Geo: Part 1 of Enforce not null merge_request_diffs.external_diff_store
What does this MR do?
Part of #229412 (closed):
When
sync_object_storage
is false,RegistryConsistencyService
calls methods which use the clauseWHERE file_store = 1
. We need to:
- index that field (which we've done).
- make it non-nullable.
So we don't have to add
OR file_store IS NULL
to ensure we process all records. That OR harms performance (see the performance improvement when we removed this clause from LFS objects).
-
Set a default external_diff_store
:-
Pre-deployment migration:
with_lock_retries do change_column_default :merge_request_diffs, :external_diff_store, 1 end
-
See !30769 (merged)
-
-
Add a partial index to support migrating NULL
s:- Post-deployment migration adding a partial index on
merge_request_diffs.id
WHERE external_diff_store IS NULL
- See !28832 (merged)
- Post-deployment migration adding a partial index on
-
Add a not null constraint ignoring existing values: - Post-deployment migration:
add_not_null_constraint(:merge_request_diffs, :external_diff_store, validate: false)
- See !31261 (diffs) but don't do the inline update. Instead, we need to do a background migration in the next MR.
- Post-deployment migration:
Follow up (tracked in the same issue)
- Add a background migration to update all the
NULL
external_diff_store
values to1
Why use a background migration (and a new partial index) to update existing rows WHERE external_diff_store IS NULL
?
Because 19M of 93M rows have external_diff_store IS NULL
.
Migration output
1: AddPartialIndexOnIdToMergeRequestDiffs
Up:
== 20200804035230 AddPartialIndexOnIdToMergeRequestDiffs: migrating ===========
-- transaction_open?()
-> 0.0000s
-- index_exists?(:merge_request_diffs, :id, {:where=>"external_diff_store IS NULL", :name=>"index_merge_request_diffs_external_diff_store_is_null", :algorithm=>:concurrently})
-> 0.0027s
-- add_index(:merge_request_diffs, :id, {:where=>"external_diff_store IS NULL", :name=>"index_merge_request_diffs_external_diff_store_is_null", :algorithm=>:concurrently})
-> 0.0096s
== 20200804035230 AddPartialIndexOnIdToMergeRequestDiffs: migrated (0.0126s) ==
Down:
== 20200804035230 AddPartialIndexOnIdToMergeRequestDiffs: reverting ===========
-- transaction_open?()
-> 0.0000s
-- indexes(:merge_request_diffs)
-> 0.0032s
-- remove_index(:merge_request_diffs, {:algorithm=>:concurrently, :name=>"index_merge_request_diffs_external_diff_store_is_null"})
-> 0.0026s
== 20200804035230 AddPartialIndexOnIdToMergeRequestDiffs: reverted (0.0062s) ==
2: AddDefaultValueForExternalDiffStoreToMergeRequestDiffs
Up:
== 20200804041018 AddDefaultValueForExternalDiffStoreToMergeRequestDiffs: migrating
-- change_column_default(:merge_request_diffs, :external_diff_store, 1)
-> 0.0045s
== 20200804041018 AddDefaultValueForExternalDiffStoreToMergeRequestDiffs: migrated (0.0109s)
Down:
== 20200804041018 AddDefaultValueForExternalDiffStoreToMergeRequestDiffs: reverting
-- change_column_default(:merge_request_diffs, :external_diff_store, nil)
-> 0.0033s
== 20200804041018 AddDefaultValueForExternalDiffStoreToMergeRequestDiffs: reverted (0.0084s)
3: AddNotNullConstraintOnExternalDiffStoreToMergeRequestDiffs
Up:
== 20200804041930 AddNotNullConstraintOnExternalDiffStoreToMergeRequestDiffs: migrating
-- transaction_open?()
-> 0.0000s
-- execute("ALTER TABLE merge_request_diffs\nADD CONSTRAINT check_93ee616ac9\nCHECK ( external_diff_store IS NOT NULL )\nNOT VALID;\n")
-> 0.0023s
== 20200804041930 AddNotNullConstraintOnExternalDiffStoreToMergeRequestDiffs: migrated (0.0171s)
Down:
== 20200804041930 AddNotNullConstraintOnExternalDiffStoreToMergeRequestDiffs: reverting
-- execute("ALTER TABLE merge_request_diffs\nDROP CONSTRAINT IF EXISTS check_93ee616ac9\n")
-> 0.0009s
== 20200804041930 AddNotNullConstraintOnExternalDiffStoreToMergeRequestDiffs: reverted (0.0062s)
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.
Edited by Michael Kozono