Skip to content

Migrate bot_type data to user_type column

Pavel Shutsin requested to merge 208897-migrate-bot-type-to-user-type into master

What does this MR do?

This is refactoring MR that moves bot_type column functionality to more generalized user_type column. See #208897 (closed) and !26068 (merged) for migration reasoning. No behavior changes are intended in scope of this MR.

Migration details

Indexes migration output 
== 20200311082301 AddUserStateIndex: migrating ================================
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:users, [:state, :user_type], {:where=>"ghost IS NOT TRUE", :name=>"index_users_on_state_and_user_type_internal", :algorithm=>:concurrently})
   -> 0.0525s
-- execute("SET statement_timeout TO 0")
   -> 0.0023s
-- add_index(:users, [:state, :user_type], {:where=>"ghost IS NOT TRUE", :name=>"index_users_on_state_and_user_type_internal", :algorithm=>:concurrently})
   -> 0.0212s
-- execute("RESET ALL")
   -> 0.0027s
-- transaction_open?()
   -> 0.0000s
-- indexes(:users)
   -> 0.0565s
-- execute("SET statement_timeout TO 0")
   -> 0.0019s
-- remove_index(:users, {:algorithm=>:concurrently, :name=>"index_users_on_state_and_internal_ee"})
   -> 0.0043s
-- execute("RESET ALL")
   -> 0.0020s
-- transaction_open?()
   -> 0.0000s
-- indexes(:users)
   -> 0.0532s
-- execute("SET statement_timeout TO 0")
   -> 0.0028s
-- remove_index(:users, {:algorithm=>:concurrently, :name=>"index_users_on_state_and_internal"})
   -> 0.0057s
-- execute("RESET ALL")
   -> 0.0025s
== 20200311082301 AddUserStateIndex: migrated (0.2087s) =======================


== 20200311082301 AddUserStateIndex: reverting ================================
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:users, :state, {:where=>"ghost IS NOT TRUE AND bot_type IS NULL", :name=>"index_users_on_state_and_internal_ee", :algorithm=>:concurrently})
   -> 0.0249s
-- execute("SET statement_timeout TO 0")
   -> 0.0011s
-- add_index(:users, :state, {:where=>"ghost IS NOT TRUE AND bot_type IS NULL", :name=>"index_users_on_state_and_internal_ee", :algorithm=>:concurrently})
   -> 0.0121s
-- execute("RESET ALL")
   -> 0.0014s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:users, :state, {:where=>"ghost IS NOT TRUE", :name=>"index_users_on_state_and_internal", :algorithm=>:concurrently})
   -> 0.0267s
-- execute("SET statement_timeout TO 0")
   -> 0.0014s
-- add_index(:users, :state, {:where=>"ghost IS NOT TRUE", :name=>"index_users_on_state_and_internal", :algorithm=>:concurrently})
   -> 0.0093s
-- execute("RESET ALL")
   -> 0.0016s
-- transaction_open?()
   -> 0.0000s
-- indexes(:users)
   -> 0.0294s
-- execute("SET statement_timeout TO 0")
   -> 0.0011s
-- remove_index(:users, {:algorithm=>:concurrently, :name=>"index_users_on_state_and_internal_ee"})
   -> 0.0028s
-- execute("RESET ALL")
   -> 0.0013s
== 20200311082301 AddUserStateIndex: reverted (0.1139s) =======================
Data migration 
== 20200311074438 MigrateBotTypeToUserType: reverting =========================
-- execute("UPDATE users SET user_type = NULL WHERE bot_type IS NOT NULL AND bot_type = user_type")
   -> 0.0057s
== 20200311074438 MigrateBotTypeToUserType: reverted (0.0057s) ================

== 20200311074438 MigrateBotTypeToUserType: migrating =========================
-- execute("UPDATE users SET user_type = bot_type WHERE bot_type IS NOT NULL AND user_type IS NULL")
   -> 0.0092s
== 20200311074438 MigrateBotTypeToUserType: migrated (0.0093s) ================
Users.active query plan 
explain SELECT "users".* FROM "users" WHERE ("users"."state" IN ('active')) AND (ghost IS NOT TRUE) AND ("users"."user_type" IS NULL OR "users"."user_type" NOT IN (2, 1, 3)) LIMIT 100

Limit  (cost=0.43..21.35 rows=100 width=1193) (actual time=9.256..12.182 rows=100 loops=1)
   Buffers: shared hit=2 read=9
   I/O Timings: read=11.698
   ->  Index Scan using index_users_on_state_and_user_type_internal on public.users  (cost=0.43..1108553.79 rows=5298432 width=1193) (actual time=9.253..12.160 rows=100 loops=1)
         Index Cond: ((users.state)::text = 'active'::text)
         Filter: ((users.user_type IS NULL) OR (users.user_type <> ALL ('{2,1,3}'::integer[])))
         Rows Removed by Filter: 3
         Buffers: shared hit=2 read=9
         I/O Timings: read=11.698

Time: 12.875 ms
  - planning: 0.539 ms
  - execution: 12.336 ms
    - I/O read: 11.698 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 2 (~16.00 KiB) from the buffer pool
  - reads: 9 (~72.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

Index changes reasoning.

  • In Sept, 2019 separate indexes for .active scope were introduced.
  • However in Feb, 2020 alert-bot was moved to CE. It means that index_users_on_state_and_internal_ee will come into play even for CE since .active scope was adjusted to exclude alert-bot.
  • #database-lab tests show that index_users_on_state_and_internal_ee isn't used at all
explain SELECT "users".* FROM "users" WHERE ("users"."state" IN ('active')) AND (ghost IS NOT TRUE) AND "users"."bot_type" IS NULL LIMIT 20

Limit  (cost=0.56..4.20 rows=20 width=1193) (actual time=0.035..0.106 rows=20 loops=1)
   Buffers: shared hit=24
   ->  Index Scan using index_users_on_bot_type on public.users  (cost=0.56..966243.68 rows=5298354 width=1193) (actual time=0.033..0.102 rows=20 loops=1)
         Index Cond: (users.bot_type IS NULL)
         Filter: ((users.ghost IS NOT TRUE) AND ((users.state)::text = 'active'::text))
         Rows Removed by Filter: 0
         Buffers: shared hit=24

So with new user_type .active query changes again so I've replaced old indexes with new index. See query plan above.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #208897 (closed)

Edited by Pavel Shutsin

Merge request reports