From 6d6b9cff490bfecd3f08e64ee1e8fdc3674b2dd2 Mon Sep 17 00:00:00 2001 From: Shayon Mukherjee Date: Tue, 15 Feb 2022 18:42:12 -0500 Subject: [PATCH] Ensure table names are unique and preset (#37) Since we are not restoring index/sequence name for now, ensuring the table names are unique so that if we run the same operation on the same table, there is no sequence conflict. The sequence will get overriden on the table anyways, but won't fail and complain when trying to create a shadow before the swap. This is temporary until we support restoring original index and sequence names. Can be a pro feature too --- lib/pg_online_schema_change/orchestrate.rb | 18 +++++-- spec/lib/orchestrate_spec.rb | 63 +++++++++++----------- 2 files changed, 43 insertions(+), 38 deletions(-) diff --git a/lib/pg_online_schema_change/orchestrate.rb b/lib/pg_online_schema_change/orchestrate.rb index d6f9f2a..4abe80f 100644 --- a/lib/pg_online_schema_change/orchestrate.rb +++ b/lib/pg_online_schema_change/orchestrate.rb @@ -1,3 +1,5 @@ +require "securerandom" + module PgOnlineSchemaChange class Orchestrate extend Helper @@ -16,6 +18,13 @@ def setup!(options) # install functions Query.run(client.connection, FUNC_FIX_SERIAL_SEQUENCE) Query.run(client.connection, FUNC_CREATE_TABLE_ALL) + + # Set this early on to ensure their creation and cleanup (unexpected) + # happens at all times. IOW, the calls from Store.get always return + # the same value. + Store.set(:old_primary_table, "pgosc_op_table_#{client.table}") + Store.set(:audit_table, "pgosc_at_#{client.table}_#{random_string}") + Store.set(:shadow_table, "pgosc_st_#{client.table}_#{random_string}") end def run!(options) @@ -69,7 +78,6 @@ def handle_signals! end def setup_audit_table! - audit_table = Store.set(:audit_table, "pgosc_audit_table_for_#{client.table}") logger.info("Setting up audit table", { audit_table: audit_table }) sql = <<~SQL @@ -120,8 +128,6 @@ def setup_trigger! end def setup_shadow_table! - shadow_table = Store.set(:shadow_table, "pgosc_shadow_table_for_#{client.table}") - logger.info("Setting up shadow table", { shadow_table: shadow_table }) Query.run(client.connection, "SELECT create_table_all('#{client.table}', '#{shadow_table}');") @@ -185,8 +191,6 @@ def replay_and_swap! def swap! logger.info("Performing swap!") - old_primary_table = Store.set(:old_primary_table, "pgosc_old_primary_table_#{client.table}") - foreign_key_statements = Query.get_foreign_keys_to_refresh(client, client.table) storage_params_reset = primary_table_storage_parameters.empty? ? "" : "ALTER TABLE #{client.table} SET (#{primary_table_storage_parameters});" @@ -243,6 +247,10 @@ def drop_and_cleanup! Query.run(client.connection, sql) end + + private def random_string + @random_string ||= SecureRandom.hex(3) + end end end end diff --git a/spec/lib/orchestrate_spec.rb b/spec/lib/orchestrate_spec.rb index ed8ccf2..becaf79 100644 --- a/spec/lib/orchestrate_spec.rb +++ b/spec/lib/orchestrate_spec.rb @@ -69,14 +69,10 @@ end it "creates the audit table with columns from parent table and additional identifiers" do - expect(client.connection).to receive(:async_exec).with("BEGIN;").and_call_original - expect(client.connection).to receive(:async_exec).with("CREATE TABLE pgosc_audit_table_for_books (operation_type text, trigger_time timestamp, LIKE books);\n").and_call_original - expect(client.connection).to receive(:async_exec).with("COMMIT;").and_call_original - described_class.setup_audit_table! RSpec::Mocks.space.reset_all - columns = PgOnlineSchemaChange::Query.table_columns(client, "pgosc_audit_table_for_books") + columns = PgOnlineSchemaChange::Query.table_columns(client, described_class.audit_table.to_s) expect(columns).to eq([ { "column_name" => "\"operation_type\"", "type" => "text", "column_position" => 1, @@ -121,13 +117,13 @@ $$ BEGIN IF ( TG_OP = 'INSERT') THEN - INSERT INTO "pgosc_audit_table_for_books" select 'INSERT', now(), NEW.* ; + INSERT INTO "#{described_class.audit_table}" select 'INSERT', now(), NEW.* ; RETURN NEW; ELSIF ( TG_OP = 'UPDATE') THEN - INSERT INTO "pgosc_audit_table_for_books" select 'UPDATE', now(), NEW.* ; + INSERT INTO "#{described_class.audit_table}" select 'UPDATE', now(), NEW.* ; RETURN NEW; ELSIF ( TG_OP = 'DELETE') THEN - INSERT INTO "pgosc_audit_table_for_books" select 'DELETE', now(), OLD.* ; + INSERT INTO "#{described_class.audit_table}" select 'DELETE', now(), OLD.* ; RETURN NEW; END IF; END; @@ -144,7 +140,7 @@ expect(client.connection).to receive(:async_exec).with("COMMIT;").twice.and_call_original described_class.setup_trigger! - expect(described_class.audit_table).to eq("pgosc_audit_table_for_books") + expect(described_class.audit_table).to eq(described_class.audit_table.to_s) end it "closes transaction when it couldn't acquire lock" do @@ -269,11 +265,6 @@ end it "creates the shadow table matching parent table" do - expect(client.connection).to receive(:async_exec).with("BEGIN;").and_call_original - expect(client.connection).to receive(:async_exec).with("SELECT fix_serial_sequence('books', 'pgosc_shadow_table_for_books');").and_call_original - expect(client.connection).to receive(:async_exec).with("SELECT create_table_all('books', 'pgosc_shadow_table_for_books');").and_call_original - expect(client.connection).to receive(:async_exec).with("COMMIT;").and_call_original - described_class.setup_shadow_table! RSpec::Mocks.space.reset_all @@ -296,22 +287,22 @@ "column_position" => 7, "column_name_regular" => "last_login" }, ]) - columns = PgOnlineSchemaChange::Query.get_indexes_for(client, "pgosc_shadow_table_for_books") - expect(columns).to eq(["CREATE UNIQUE INDEX pgosc_shadow_table_for_books_pkey ON pgosc_shadow_table_for_books USING btree (user_id)", - "CREATE UNIQUE INDEX pgosc_shadow_table_for_books_username_key ON pgosc_shadow_table_for_books USING btree (username)", - "CREATE UNIQUE INDEX pgosc_shadow_table_for_books_email_key ON pgosc_shadow_table_for_books USING btree (email)"]) + columns = PgOnlineSchemaChange::Query.get_indexes_for(client, described_class.shadow_table.to_s) + expect(columns).to eq(["CREATE UNIQUE INDEX #{described_class.shadow_table}_pkey ON #{described_class.shadow_table} USING btree (user_id)", + "CREATE UNIQUE INDEX #{described_class.shadow_table}_username_key ON #{described_class.shadow_table} USING btree (username)", + "CREATE UNIQUE INDEX #{described_class.shadow_table}_email_key ON #{described_class.shadow_table} USING btree (email)"]) foreign_keys = PgOnlineSchemaChange::Query.get_foreign_keys_for(client, - "pgosc_shadow_table_for_books") + described_class.shadow_table.to_s) expect(foreign_keys).to eq([ - { "table_on" => "pgosc_shadow_table_for_books", "table_from" => "sellers", - "constraint_type" => "f", "constraint_name" => "pgosc_shadow_table_for_books_seller_id_fkey", "constraint_validated" => "t", "definition" => "FOREIGN KEY (seller_id) REFERENCES sellers(id)" }, + { "table_on" => described_class.shadow_table.to_s, "table_from" => "sellers", + "constraint_type" => "f", "constraint_name" => "#{described_class.shadow_table}_seller_id_fkey", "constraint_validated" => "t", "definition" => "FOREIGN KEY (seller_id) REFERENCES sellers(id)" }, ]) primary_keys = PgOnlineSchemaChange::Query.get_primary_keys_for(client, - "pgosc_shadow_table_for_books") + described_class.shadow_table.to_s) expect(primary_keys).to eq([ - { "constraint_name" => "pgosc_shadow_table_for_books_pkey", "constraint_type" => "p", - "constraint_validated" => "t", "definition" => "PRIMARY KEY (user_id)", "table_from" => "-", "table_on" => "pgosc_shadow_table_for_books" }, + { "constraint_name" => "#{described_class.shadow_table}_pkey", "constraint_type" => "p", + "constraint_validated" => "t", "definition" => "PRIMARY KEY (user_id)", "table_from" => "-", "table_on" => described_class.shadow_table.to_s }, ]) end @@ -344,11 +335,11 @@ it "succesfully" do query = <<~SQL - ALTER TABLE pgosc_shadow_table_for_books SET ( + ALTER TABLE #{described_class.shadow_table} SET ( autovacuum_enabled = false, toast.autovacuum_enabled = false ); - ALTER TABLE pgosc_audit_table_for_books SET ( + ALTER TABLE #{described_class.audit_table} SET ( autovacuum_enabled = false, toast.autovacuum_enabled = false ); SQL @@ -524,7 +515,7 @@ it "succesfully" do expect(client.connection).to receive(:async_exec).with("BEGIN;").and_call_original - expect(client.connection).to receive(:async_exec).with("ALTER TABLE pgosc_shadow_table_for_books ADD COLUMN purchased boolean DEFAULT false").and_call_original + expect(client.connection).to receive(:async_exec).with("ALTER TABLE #{described_class.shadow_table} ADD COLUMN purchased boolean DEFAULT false").and_call_original expect(client.connection).to receive(:async_exec).with("COMMIT;").and_call_original described_class.run_alter_statement! @@ -576,7 +567,7 @@ it "succesfully" do expect(client.connection).to receive(:async_exec).with("BEGIN;").and_call_original - expect(client.connection).to receive(:async_exec).with("ALTER TABLE pgosc_shadow_table_for_books DROP email").and_call_original + expect(client.connection).to receive(:async_exec).with("ALTER TABLE #{described_class.shadow_table} DROP email").and_call_original expect(client.connection).to receive(:async_exec).with("COMMIT;").and_call_original described_class.run_alter_statement! @@ -623,7 +614,7 @@ it "succesfully" do expect(client.connection).to receive(:async_exec).with("BEGIN;").and_call_original - expect(client.connection).to receive(:async_exec).with("ALTER TABLE pgosc_shadow_table_for_books RENAME COLUMN email TO new_email").and_call_original + expect(client.connection).to receive(:async_exec).with("ALTER TABLE #{described_class.shadow_table} RENAME COLUMN email TO new_email").and_call_original expect(client.connection).to receive(:async_exec).with("COMMIT;").and_call_original described_class.run_alter_statement! @@ -680,12 +671,18 @@ PgOnlineSchemaChange::Replay.play!(rows) end + it "ensures helper table names are of proper length" do + expect(described_class.shadow_table.length).to eq(21) + expect(described_class.audit_table.length).to eq(21) + expect(described_class.old_primary_table.length).to eq(20) + end + it "sucessfully renames the tables" do described_class.swap! # Fetch rows from the original primary table select_query = <<~SQL - SELECT * FROM pgosc_old_primary_table_books; + SELECT * FROM #{described_class.old_primary_table}; SQL rows = [] PgOnlineSchemaChange::Query.run(client.connection, select_query) do |result| @@ -705,9 +702,9 @@ # confirm indexes on newly renamed table columns = PgOnlineSchemaChange::Query.get_indexes_for(client, "books") - expect(columns).to eq(["CREATE UNIQUE INDEX pgosc_shadow_table_for_books_pkey ON books USING btree (user_id)", - "CREATE UNIQUE INDEX pgosc_shadow_table_for_books_username_key ON books USING btree (username)", - "CREATE UNIQUE INDEX pgosc_shadow_table_for_books_email_key ON books USING btree (email)"]) + expect(columns).to eq(["CREATE UNIQUE INDEX #{described_class.shadow_table}_pkey ON books USING btree (user_id)", + "CREATE UNIQUE INDEX #{described_class.shadow_table}_username_key ON books USING btree (username)", + "CREATE UNIQUE INDEX #{described_class.shadow_table}_email_key ON books USING btree (email)"]) end it "sucessfully drops the trigger" do