Migrate bot_type data to user_type column
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
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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