Merge branch '338535-redo-loose-fk-tables' into 'master'

Replace time-range partitioned loose FK table

See merge request gitlab-org/gitlab!71718

app/models/loose_foreign_keys/deleted_record.rb

@@ -2,48 +2,4 @@
 
 class LooseForeignKeys::DeletedRecord < ApplicationRecord
   extend SuppressCompositePrimaryKeyWarning
-  include PartitionedTable
-
-  partitioned_by :created_at, strategy: :monthly, retain_for: 3.months, retain_non_empty_partitions: true
-
-  scope :ordered_by_primary_keys, -> { order(:created_at, :deleted_table_name, :deleted_table_primary_key_value) }
-
-  def self.load_batch(batch_size)
-    ordered_by_primary_keys
-      .limit(batch_size)
-      .to_a
-  end
-
-  # Because the table has composite primary keys, the delete_all or delete methods are not going to work.
-  # This method implements deletion that benefits from the primary key index, example:
-  #
-  # > DELETE
-  # > FROM "loose_foreign_keys_deleted_records"
-  # > WHERE (created_at,
-  # >        deleted_table_name,
-  # >        deleted_table_primary_key_value) IN
-  # >     (SELECT created_at::TIMESTAMP WITH TIME ZONE,
-  # >                                             deleted_table_name,
-  # >                                             deleted_table_primary_key_value
-  # >      FROM (VALUES (LIST_OF_VALUES)) AS primary_key_values (created_at, deleted_table_name, deleted_table_primary_key_value))
-  def self.delete_records(records)
-    values = records.pluck(:created_at, :deleted_table_name, :deleted_table_primary_key_value)
-
-    primary_keys = connection.primary_keys(table_name).join(', ')
-
-    primary_keys_with_type_cast = [
-      Arel.sql('created_at::timestamp with time zone'),
-      Arel.sql('deleted_table_name'),
-      Arel.sql('deleted_table_primary_key_value')
-    ]
-
-    value_list = Arel::Nodes::ValuesList.new(values)
-
-    # (SELECT primary keys FROM VALUES)
-    inner_query = Arel::SelectManager.new
-    inner_query.from("#{Arel::Nodes::Grouping.new([value_list]).as('primary_key_values').to_sql} (#{primary_keys})")
-    inner_query.projections = primary_keys_with_type_cast
-
-    where(Arel::Nodes::Grouping.new([Arel.sql(primary_keys)]).in(inner_query)).delete_all
-  end
 end

db/migrate/20211005092428_drop_time_range_partitioned_loose_fk.rb

+# frozen_string_literal: true
+
+class DropTimeRangePartitionedLooseFk < Gitlab::Database::Migration[1.0]
+  include Gitlab::Database::PartitioningMigrationHelpers
+
+  def up
+    # the table is not in use
+    drop_table :loose_foreign_keys_deleted_records # rubocop: disable Migration/DropTable
+  end
+
+  def down
+    constraint_name = check_constraint_name('loose_foreign_keys_deleted_records', 'deleted_table_name', 'max_length')
+    execute(<<~SQL)
+      CREATE TABLE loose_foreign_keys_deleted_records (
+        created_at timestamp with time zone NOT NULL DEFAULT NOW(),
+        deleted_table_name text NOT NULL,
+        deleted_table_primary_key_value bigint NOT NULL,
+        PRIMARY KEY (created_at, deleted_table_name, deleted_table_primary_key_value),
+        CONSTRAINT #{constraint_name} CHECK ((char_length(deleted_table_name) <= 63))
+      ) PARTITION BY RANGE (created_at);
+    SQL
+
+    min_date = Date.today - 1.month
+    max_date = Date.today + 3.months
+    create_daterange_partitions('loose_foreign_keys_deleted_records', 'created_at', min_date, max_date)
+  end
+end

db/migrate/20211005093558_add_range_partitioned_loose_fk_table.rb

+# frozen_string_literal: true
+
+class AddRangePartitionedLooseFkTable < Gitlab::Database::Migration[1.0]
+  include Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers
+
+  def up
+    constraint_name = check_constraint_name('loose_foreign_keys_deleted_records', 'fully_qualified_table_name', 'max_length')
+    execute(<<~SQL)
+      CREATE TABLE loose_foreign_keys_deleted_records (
+        id BIGSERIAL NOT NULL,
+        partition bigint NOT NULL,
+        primary_key_value bigint NOT NULL,
+        status smallint NOT NULL DEFAULT 1,
+        created_at timestamp with time zone NOT NULL DEFAULT NOW(),
+        fully_qualified_table_name text NOT NULL,
+        PRIMARY KEY (partition, id),
+        CONSTRAINT #{constraint_name} CHECK ((char_length(fully_qualified_table_name) <= 150))
+      ) PARTITION BY LIST (partition);
+
+      CREATE TABLE gitlab_partitions_static.loose_foreign_keys_deleted_records_1
+      PARTITION OF loose_foreign_keys_deleted_records
+      FOR VALUES IN (1);
+    SQL
+  end
+
+  def down
+    drop_table :loose_foreign_keys_deleted_records
+  end
+end

db/migrate/20211005100112_recreate_loose_fk_insert_function.rb

+# frozen_string_literal: true
+
+class RecreateLooseFkInsertFunction < Gitlab::Database::Migration[1.0]
+  include Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers
+
+  def up
+    execute(<<~SQL)
+      CREATE OR REPLACE FUNCTION #{DELETED_RECORDS_INSERT_FUNCTION_NAME}()
+      RETURNS TRIGGER AS
+      $$
+      BEGIN
+        INSERT INTO loose_foreign_keys_deleted_records
+        (partition, fully_qualified_table_name, primary_key_value)
+        SELECT 1, TG_TABLE_SCHEMA || '.' || TG_TABLE_NAME, old_table.id FROM old_table
+        ON CONFLICT DO NOTHING;
+
+        RETURN NULL;
+      END
+      $$ LANGUAGE PLPGSQL
+    SQL
+  end
+
+  def down
+    # old function
+    execute(<<~SQL)
+      CREATE OR REPLACE FUNCTION #{DELETED_RECORDS_INSERT_FUNCTION_NAME}()
+      RETURNS TRIGGER AS
+      $$
+      BEGIN
+        INSERT INTO loose_foreign_keys_deleted_records
+        (deleted_table_name, deleted_table_primary_key_value)
+        SELECT TG_TABLE_NAME, old_table.id FROM old_table
+        ON CONFLICT DO NOTHING;
+
+        RETURN NULL;
+      END
+      $$ LANGUAGE PLPGSQL
+    SQL
+  end
+end

db/schema_migrations/20211005092428

+43abb71ecc1f1b4e699af1258934884a06e4e4eb1445ec3cc7a2c6668f42f14a
\ No newline at end of file

db/schema_migrations/20211005093558

+16638e14f1920b2e615dcb14965b7ef2a16ead099e7f8b1cdad6dd75d6d45107
\ No newline at end of file

db/schema_migrations/20211005100112

+196cd1cf84babb12e92830bf2b7a0315499fdb976f825d4913a506e744b4fd53
\ No newline at end of file

db/structure.sql

@@ -27,8 +27,8 @@ CREATE FUNCTION insert_into_loose_foreign_keys_deleted_records() RETURNS trigger
     AS $$
 BEGIN
   INSERT INTO loose_foreign_keys_deleted_records
-  (deleted_table_name, deleted_table_primary_key_value)
-  SELECT TG_TABLE_NAME, old_table.id FROM old_table
+  (partition, fully_qualified_table_name, primary_key_value)
+  SELECT 1, TG_TABLE_SCHEMA || '.' || TG_TABLE_NAME, old_table.id FROM old_table
   ON CONFLICT DO NOTHING;
 
   RETURN NULL;
@@ -128,14 +128,6 @@ CREATE TABLE incident_management_pending_issue_escalations (
 )
 PARTITION BY RANGE (process_at);
 
-CREATE TABLE loose_foreign_keys_deleted_records (
-    created_at timestamp with time zone DEFAULT now() NOT NULL,
-    deleted_table_name text NOT NULL,
-    deleted_table_primary_key_value bigint NOT NULL,
-    CONSTRAINT check_7229f9527e CHECK ((char_length(deleted_table_name) <= 63))
-)
-PARTITION BY RANGE (created_at);
-
 CREATE TABLE web_hook_logs (
     id bigint NOT NULL,
     web_hook_id integer NOT NULL,
@@ -945,6 +937,37 @@ CREATE TABLE gitlab_partitions_static.analytics_cycle_analytics_merge_request_st
 );
 ALTER TABLE ONLY analytics_cycle_analytics_merge_request_stage_events ATTACH PARTITION gitlab_partitions_static.analytics_cycle_analytics_merge_request_stage_events_31 FOR VALUES WITH (modulus 32, remainder 31);
 
+CREATE TABLE loose_foreign_keys_deleted_records (
+    id bigint NOT NULL,
+    partition bigint NOT NULL,
+    primary_key_value bigint NOT NULL,
+    status smallint DEFAULT 1 NOT NULL,
+    created_at timestamp with time zone DEFAULT now() NOT NULL,
+    fully_qualified_table_name text NOT NULL,
+    CONSTRAINT check_1a541f3235 CHECK ((char_length(fully_qualified_table_name) <= 150))
+)
+PARTITION BY LIST (partition);
+
+CREATE SEQUENCE loose_foreign_keys_deleted_records_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE loose_foreign_keys_deleted_records_id_seq OWNED BY loose_foreign_keys_deleted_records.id;
+
+CREATE TABLE gitlab_partitions_static.loose_foreign_keys_deleted_records_1 (
+    id bigint DEFAULT nextval('loose_foreign_keys_deleted_records_id_seq'::regclass) NOT NULL,
+    partition bigint NOT NULL,
+    primary_key_value bigint NOT NULL,
+    status smallint DEFAULT 1 NOT NULL,
+    created_at timestamp with time zone DEFAULT now() NOT NULL,
+    fully_qualified_table_name text NOT NULL,
+    CONSTRAINT check_1a541f3235 CHECK ((char_length(fully_qualified_table_name) <= 150))
+);
+ALTER TABLE ONLY loose_foreign_keys_deleted_records ATTACH PARTITION gitlab_partitions_static.loose_foreign_keys_deleted_records_1 FOR VALUES IN ('1');
+
 CREATE TABLE product_analytics_events_experimental (
     id bigint NOT NULL,
     project_id integer NOT NULL,
@@ -21426,6 +21449,8 @@ ALTER TABLE ONLY list_user_preferences ALTER COLUMN id SET DEFAULT nextval('list
 
 ALTER TABLE ONLY lists ALTER COLUMN id SET DEFAULT nextval('lists_id_seq'::regclass);
 
+ALTER TABLE ONLY loose_foreign_keys_deleted_records ALTER COLUMN id SET DEFAULT nextval('loose_foreign_keys_deleted_records_id_seq'::regclass);
+
 ALTER TABLE ONLY members ALTER COLUMN id SET DEFAULT nextval('members_id_seq'::regclass);
 
 ALTER TABLE ONLY merge_request_assignees ALTER COLUMN id SET DEFAULT nextval('merge_request_assignees_id_seq'::regclass);
@@ -22048,6 +22073,12 @@ ALTER TABLE ONLY gitlab_partitions_static.analytics_cycle_analytics_merge_reques
 ALTER TABLE ONLY gitlab_partitions_static.analytics_cycle_analytics_merge_request_stage_events_31
     ADD CONSTRAINT analytics_cycle_analytics_merge_request_stage_events_31_pkey PRIMARY KEY (stage_event_hash_id, merge_request_id);
 
+ALTER TABLE ONLY loose_foreign_keys_deleted_records
+    ADD CONSTRAINT loose_foreign_keys_deleted_records_pkey PRIMARY KEY (partition, id);
+
+ALTER TABLE ONLY gitlab_partitions_static.loose_foreign_keys_deleted_records_1
+    ADD CONSTRAINT loose_foreign_keys_deleted_records_1_pkey PRIMARY KEY (partition, id);
+
 ALTER TABLE ONLY product_analytics_events_experimental
     ADD CONSTRAINT product_analytics_events_experimental_pkey PRIMARY KEY (id, project_id);
 
@@ -23119,9 +23150,6 @@ ALTER TABLE ONLY list_user_preferences
 ALTER TABLE ONLY lists
     ADD CONSTRAINT lists_pkey PRIMARY KEY (id);
 
-ALTER TABLE ONLY loose_foreign_keys_deleted_records
-    ADD CONSTRAINT loose_foreign_keys_deleted_records_pkey PRIMARY KEY (created_at, deleted_table_name, deleted_table_primary_key_value);
-
 ALTER TABLE ONLY members
     ADD CONSTRAINT members_pkey PRIMARY KEY (id);
 
@@ -27147,6 +27175,8 @@ ALTER INDEX analytics_cycle_analytics_merge_request_stage_events_pkey ATTACH PAR
 
 ALTER INDEX analytics_cycle_analytics_merge_request_stage_events_pkey ATTACH PARTITION gitlab_partitions_static.analytics_cycle_analytics_merge_request_stage_events_31_pkey;
 
+ALTER INDEX loose_foreign_keys_deleted_records_pkey ATTACH PARTITION gitlab_partitions_static.loose_foreign_keys_deleted_records_1_pkey;
+
 ALTER INDEX index_product_analytics_events_experimental_project_and_time ATTACH PARTITION gitlab_partitions_static.product_analytics_events_expe_project_id_collector_tstamp_idx10;
 
 ALTER INDEX index_product_analytics_events_experimental_project_and_time ATTACH PARTITION gitlab_partitions_static.product_analytics_events_expe_project_id_collector_tstamp_idx11;

spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb

@@ -19,6 +19,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do
     end
   end
 
+  after(:all) do
+    migration.drop_table :loose_fk_test_table
+  end
+
   before do
     3.times { model.create! }
   end
@@ -45,8 +49,9 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do
       expect(LooseForeignKeys::DeletedRecord.count).to eq(1)
       deleted_record = LooseForeignKeys::DeletedRecord.all.first
 
-      expect(deleted_record.deleted_table_primary_key_value).to eq(record_to_be_deleted.id)
-      expect(deleted_record.deleted_table_name).to eq('loose_fk_test_table')
+      expect(deleted_record.primary_key_value).to eq(record_to_be_deleted.id)
+      expect(deleted_record.fully_qualified_table_name).to eq('public.loose_fk_test_table')
+      expect(deleted_record.partition).to eq(1)
     end
 
     it 'stores multiple record deletions' do

spec/models/loose_foreign_keys/deleted_record_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe LooseForeignKeys::DeletedRecord do
-  let_it_be(:deleted_record_1) { described_class.create!(created_at: 1.day.ago, deleted_table_name: 'projects', deleted_table_primary_key_value: 5) }
-  let_it_be(:deleted_record_2) { described_class.create!(created_at: 3.days.ago, deleted_table_name: 'projects', deleted_table_primary_key_value: 1) }
-  let_it_be(:deleted_record_3) { described_class.create!(created_at: 5.days.ago, deleted_table_name: 'projects', deleted_table_primary_key_value: 3) }
-  let_it_be(:deleted_record_4) { described_class.create!(created_at: 10.days.ago, deleted_table_name: 'projects', deleted_table_primary_key_value: 1) } # duplicate
-
-  # skip created_at because it gets truncated after insert
-  def map_attributes(records)
-    records.pluck(:deleted_table_name, :deleted_table_primary_key_value)
-  end
-
-  describe 'partitioning strategy' do
-    it 'has retain_non_empty_partitions option' do
-      expect(described_class.partitioning_strategy.retain_non_empty_partitions).to eq(true)
-    end
-  end
-
-  describe '.load_batch' do
-    it 'loads records and orders them by creation date' do
-      records = described_class.load_batch(4)
-
-      expect(map_attributes(records)).to eq([['projects', 1], ['projects', 3], ['projects', 1], ['projects', 5]])
-    end
-
-    it 'supports configurable batch size' do
-      records = described_class.load_batch(2)
-
-      expect(map_attributes(records)).to eq([['projects', 1], ['projects', 3]])
-    end
-  end
-
-  describe '.delete_records' do
-    it 'deletes exactly one record' do
-      described_class.delete_records([deleted_record_2])
-
-      expect(described_class.count).to eq(3)
-      expect(described_class.find_by(created_at: deleted_record_2.created_at)).to eq(nil)
-    end
-
-    it 'deletes two records' do
-      described_class.delete_records([deleted_record_2, deleted_record_4])
-
-      expect(described_class.count).to eq(2)
-    end
-
-    it 'deletes all records' do
-      described_class.delete_records([deleted_record_1, deleted_record_2, deleted_record_3, deleted_record_4])
-
-      expect(described_class.count).to eq(0)
-    end
-  end
-end