Add NOT VALID strict sharding key constraint to notes

Chen Zhangrequested to merge
601451-notes-strict-sharding-key-not-valid into master

What does this MR do and why?

Adds the strict num_nonnulls(namespace_id, organization_id, project_id) = 1 check constraint to the notes table as NOT VALID.

Why NOT VALID now instead of waiting for the BBM to finish

Postgres does not scan existing rows when a constraint is added as NOT VALID. The ~2.33B legacy dual-key rows on GitLab.com (where both namespace_id and project_id are set) are therefore unaffected at add time. The constraint is enforced only for new writes:

  • INSERT paths are single-key. Note#ensure_namespace_id normalizes the key on every validation, the ExactlyOnePresentValidator added by !251888 (merged) rejects anything that slips past it, and every callback-bypassing insert path (Gitlab::GithubImport::Importer::NoteImporter, DiffNoteImporter, WorkItems::DataSync::Handlers::Notes::CopyService, the vulnerability Note.insert_all! services, RestoreIncorrectVulnerabilityStates::NoteInserter) writes project_id only. See the production verification below.
  • UPDATE paths on legacy dual-key rows are repaired in-place by the BEFORE UPDATE trigger trigger_817aa51bc4f2 added by !250291 (merged). The trigger nulls namespace_id before the CHECK runs, so the row satisfies = 1 by the time Postgres evaluates the constraint.

This lets us add the strict constraint now, while the CleanupDualShardingKeysInNotes BBM (!238033 (merged)) continues draining the legacy rows over the coming months.

Production verification of the insert path

Waiting for the BBM would not make inserts any safer, since the BBM only rewrites existing rows. Insert safety was therefore verified directly against GitLab.com data (postgres.ai, clone of 2026-09-03), using the partial index tmp_index_notes_on_id_with_namespace_and_project and an id anchor (importers preserve the source created_at, so anchoring on id also catches imported rows):

SELECT 1 FROM notes
WHERE id >= (SELECT min(id) FROM notes
             WHERE created_at >= '2026-08-05' AND created_at < '2026-08-05 00:05')
  AND namespace_id IS NOT NULL AND project_id IS NOT NULL;
-- 443 rows (Joe query 6242324198467228080)

Fingerprinting those 443 rows (Joe query 5249662474714771066 and the preceding run):

Signal Count
noteable_type = 'WikiPage::Meta' 443
namespace_id = projects.project_namespace_id 443
cached_markdown_version IS NULL (raw insert) 0
imported_from <> 0 0
system 0
created_at >= '2026-08-20' 0

All 443 are project wiki comments written by the pre-!238033 version of Note#ensure_namespace_id, which deliberately kept namespace_id for wiki page notes:

# app/models/note.rb at b5e05271 (before !238033)
self.namespace_id = if group_level_issue?
                      noteable&.namespace_id
                    elsif for_wiki_page?
                      noteable&.namespace_id || project&.project_namespace_id
                    end

!238033 (merged) (merged 2026-08-10) replaced this with the group_level_noteable? guard, which only keeps namespace_id for wikis whose noteable.project_id is blank. The last dual-key row was inserted before 2026-08-20, consistent with that change reaching production. Since then, roughly 90M notes have been inserted across every noteable type, importer, bot, and API client with zero dual-key rows. The 443 leftovers are ordinary legacy rows that the BBM and the repair trigger handle.

The same anchored query should be re-run on a fresh clone before this MR is merged, anchored at 2026-08-20, and is expected to return 0 rows.

Why an explicit constraint_name

The auto-generated name for add_multi_column_not_null_constraint on (namespace_id, organization_id, project_id) is check_82f260979e, which is the same name as the existing loose >= 1 constraint. Passing the same name would cause add_check_constraint to detect the existing constraint and silently skip adding the new one. The explicit name check_notes_sharding_key_eq_one avoids the collision and follows the descriptive style used by similar tighten-constraint migrations.

What is intentionally left in place

  • The loose constraint check_82f260979e (num_nonnulls >= 1) remains. Dropping it now would leave a window with no sharding-key constraint at all.
  • The notes entry in allowed_to_have_loose_multi_column_sharding_constraint in spec/lib/gitlab/organizations/sharding_key_spec.rb remains. The has_exactly_one_not_null_check_constraint? helper only counts convalidated = true constraints, so the NOT VALID constraint does not satisfy it yet.
  • The repair trigger trigger_817aa51bc4f2 remains. It is needed for the duration of the BBM.

All three will be cleaned up in follow-up work tracked by #618780 after the BBM finalizes and the constraint is validated.

References

Screenshots or screen recordings

Not applicable (database migration only).

How to set up and validate locally

  1. Run the migration: 
    bundle exec rails db:migrate
  2. Confirm the constraint exists and is NOT VALID: 
    SELECT conname, convalidated
FROM pg_constraint
WHERE conrelid = 'notes'::regclass
  AND contype = 'c'
  AND conname = 'check_notes_sharding_key_eq_one';
-- Expected: conname = check_notes_sharding_key_eq_one, convalidated = f
  3. Confirm the loose constraint is still present: 
    SELECT conname FROM pg_constraint
WHERE conrelid = 'notes'::regclass AND conname = 'check_82f260979e';
  4. Run the migration spec: 
    bundle exec rspec spec/migrations/20260903120000_add_strict_sharding_key_constraint_to_notes_spec.rb
  5. Run the sharding key spec to confirm the allow-list entry is still needed: 
    bundle exec rspec spec/lib/gitlab/organizations/sharding_key_spec.rb
  6. Roll back and confirm the constraint is removed: 
    bundle exec rails db:migrate:down VERSION=20260903120000

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Chen Zhang

Merge request reports