From ddd290544450711c74576ca0ec9fd7e9dfd683bf Mon Sep 17 00:00:00 2001 From: bplunkett-stripe Date: Thu, 3 Aug 2023 23:37:55 -0400 Subject: [PATCH 1/5] Add foreign key support --- README.md | 1 - .../acceptance_test.go | 4 + .../column_cases_test.go | 8 +- .../foreign_key_constraint_cases_test.go | 638 ++++++++++++++++++ .../partitioned_table_cases_test.go | 421 +++++++++--- .../schema_cases_test.go | 204 +++--- .../sequence_cases_test.go | 4 - .../table_cases_test.go | 113 +++- internal/queries/queries.sql | 33 +- internal/queries/queries.sql.go | 446 ++++++------ internal/schema/schema.go | 78 ++- internal/schema/schema_test.go | 293 +++++--- pkg/diff/plan.go | 19 +- pkg/diff/schema_migration_plan_test.go | 263 +++----- pkg/diff/sql_generator.go | 298 ++++++-- 15 files changed, 2086 insertions(+), 737 deletions(-) create mode 100644 internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go diff --git a/README.md b/README.md index ffe8dbd..fdad34f 100644 --- a/README.md +++ b/README.md @@ -130,7 +130,6 @@ Postgres v13 and below are not supported. Use at your own risk. Note, the library only currently supports diffing the *public* schema. Support for diffing other schemas is on the roadmap *Unsupported*: -- (On roadmap) Foreign key constraints - (On roadmap) Diffing schemas other than "public" - (On roadmap) Unique constraints (unique indexes are supported but not unique constraints) - (On roadmap) Adding and remove partitions from an existing partitioned table diff --git a/internal/migration_acceptance_tests/acceptance_test.go b/internal/migration_acceptance_tests/acceptance_test.go index 3329f16..77be89f 100644 --- a/internal/migration_acceptance_tests/acceptance_test.go +++ b/internal/migration_acceptance_tests/acceptance_test.go @@ -17,6 +17,10 @@ import ( "github.com/stripe/pg-schema-diff/pkg/tempdb" ) +var ( + errValidatingPlan = fmt.Errorf("validating migration plan") +) + type ( expectations struct { planErrorIs error diff --git a/internal/migration_acceptance_tests/column_cases_test.go b/internal/migration_acceptance_tests/column_cases_test.go index 38a2eb0..56caf41 100644 --- a/internal/migration_acceptance_tests/column_cases_test.go +++ b/internal/migration_acceptance_tests/column_cases_test.go @@ -543,8 +543,12 @@ var columnAcceptanceTestCases = []acceptanceTestCase{ diff.MigrationHazardTypeAcquiresAccessExclusiveLock, diff.MigrationHazardTypeImpactsDatabasePerformance, }, - vanillaExpectations: expectations{planErrorContains: "validating migration plan"}, - dataPackingExpectations: expectations{planErrorContains: "validating migration plan"}, + vanillaExpectations: expectations{ + planErrorContains: errValidatingPlan.Error(), + }, + dataPackingExpectations: expectations{ + planErrorContains: errValidatingPlan.Error(), + }, }, { name: "Change to not null", diff --git a/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go b/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go new file mode 100644 index 0000000..7f65974 --- /dev/null +++ b/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go @@ -0,0 +1,638 @@ +package migration_acceptance_tests + +import "github.com/stripe/pg-schema-diff/pkg/diff" + +var foreignKeyConstraintCases = []acceptanceTestCase{ + { + name: "No-op", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id_part_1 INT, + id_part_2 VARCHAR(255), + PRIMARY KEY (id_part_1, id_part_2) + ); + CREATE TABLE "foobar fk"( + fk_part_1 BIGINT, + fk_part_2 VARCHAR(255), + FOREIGN KEY (fk_part_1, fk_part_2) REFERENCES foobar(id_part_1, id_part_2) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id_part_1 INT, + id_part_2 VARCHAR(255), + PRIMARY KEY (id_part_1, id_part_2) + ); + CREATE TABLE "foobar fk"( + fk_part_1 BIGINT, + fk_part_2 VARCHAR(255), + FOREIGN KEY (fk_part_1, fk_part_2) REFERENCES foobar(id_part_1, id_part_2) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + vanillaExpectations: expectations{ + empty: true, + }, + dataPackingExpectations: expectations{ + empty: true, + }, + }, + { + name: "Add FK with most options", + oldSchemaDDL: []string{ + ` + CREATE TABLE "foo bar"( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE "foo bar"( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES "foo bar"(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Add FK (only referenced table is new)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Add FK (owning table is not new)", + oldSchemaDDL: []string{ + ` + CREATE TABLE "foobar fk"( + fk_id INT + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Add FK (both tables new)", + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + }, + { + name: "Add not-valid FK (neither table is new)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_foobar_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id) + NOT VALID; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{}, + }, + { + name: "Add FK (partitioned table)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + foo VARCHAR(255), + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('2'); + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + foo VARCHAR(255), + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('2'); + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT, + FOREIGN KEY (fk_foo, fk_id) REFERENCES foobar(foo, id) + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Drop FK", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + `, + }, + }, + { + name: "Drop FK (partitioned table)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + foo VARCHAR(255), + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('2'); + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT, + FOREIGN KEY (fk_foo, fk_id) REFERENCES foobar(foo, id) + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + foo VARCHAR(255), + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('2'); + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT + ); + `, + }, + }, + { + name: "Alter FK (not valid to valid)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id) + NOT VALID; + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id); + `, + }, + }, + { + name: "Alter FK (valid to not valid)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id) + NOT VALID; + `, + }, + }, + { + name: "Alter FK (columns)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + foo VARCHAR(255), + PRIMARY KEY (id) + ); + CREATE UNIQUE INDEX foobar_unique ON foobar(foo); + + CREATE TABLE "foobar fk"( + fk_id INT, + fk_foo VARCHAR(255) + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id) + NOT VALID; + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + foo VARCHAR(255), + PRIMARY KEY (id) + ); + CREATE UNIQUE INDEX foobar_unique ON foobar(foo); + + CREATE TABLE "foobar fk"( + fk_id INT, + fk_foo VARCHAR(255) + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_foo) REFERENCES foobar(foo); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Alter FK (on update)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ON UPDATE CASCADE + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Alter FK (on delete)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ON UPDATE CASCADE + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Alter castable type change", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id BIGINT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + diff.MigrationHazardTypeImpactsDatabasePerformance, + }, + }, + { + name: "Alter non-castable type change", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id BIGINT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id BIGINT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id TIMESTAMP, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id TIMESTAMP, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ); + `, + }, + vanillaExpectations: expectations{ + planErrorContains: errValidatingPlan.Error(), + }, + dataPackingExpectations: expectations{ + planErrorContains: errValidatingPlan.Error(), + }, + }, + { + name: "Switch FK owning table (to partitioned table)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id); + + CREATE TABLE "foobar fk partitioned"( + foo varchar(255), + fk_id INT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('2'); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + + CREATE TABLE "foobar fk partitioned"( + foo varchar(255), + fk_id INT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('2'); + ALTER TABLE "foobar fk partitioned" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, + { + name: "Switch FK referenced table (to partitioned table with new unique index)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + foo varchar(255), + id INT, + PRIMARY KEY (id) + ); + CREATE UNIQUE INDEX unique_idx ON foobar(foo, id); + + CREATE TABLE "foobar partitioned"( + foo varchar(255), + id INT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF "foobar partitioned" FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF "foobar partitioned" FOR VALUES IN ('2'); + + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_foo, fk_id) REFERENCES foobar(foo, id) NOT VALID ; + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + foo varchar(255), + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar partitioned"( + foo varchar(255), + id INT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF "foobar partitioned" FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF "foobar partitioned" FOR VALUES IN ('2'); + CREATE UNIQUE INDEX unique_idx ON "foobar partitioned"(foo, id); + + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_foo, fk_id) REFERENCES "foobar partitioned"(foo, id); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeIndexDropped, + diff.MigrationHazardTypeIndexBuild, + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + }, + }, +} + +func (suite *acceptanceTestSuite) TestForeignKeyConstraintTestCases() { + suite.runTestCases(foreignKeyConstraintCases) +} diff --git a/internal/migration_acceptance_tests/partitioned_table_cases_test.go b/internal/migration_acceptance_tests/partitioned_table_cases_test.go index 0d9fbb0..b889074 100644 --- a/internal/migration_acceptance_tests/partitioned_table_cases_test.go +++ b/internal/migration_acceptance_tests/partitioned_table_cases_test.go @@ -17,17 +17,31 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ CHECK ( fizz > 0 ), PRIMARY KEY (foo, id) ) PARTITION BY LIST (foo); - CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF foobar FOR VALUES IN ('foo_3'); - -- partitioned indexes - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz); - + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); -- local indexes CREATE INDEX foobar_1_local_idx ON foobar_1(foo); CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- local indexes + CREATE UNIQUE INDEX foobar_1_local_unique_idx ON foobar_fk_1(foo); + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo) REFERENCES foobar_2(foo); + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo) REFERENCES foobar_fk_1(foo); `, }, newSchemaDDL: []string{ @@ -40,17 +54,34 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ CHECK ( fizz > 0 ), PRIMARY KEY (foo, id) ) PARTITION BY LIST (foo); - + -- partitions CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF foobar FOR VALUES IN ('foo_3'); - -- partitioned indexes - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz); - + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); -- local indexes CREATE INDEX foobar_1_local_idx ON foobar_1(foo); CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + -- partitions + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- local indexes + CREATE UNIQUE INDEX foobar_1_local_unique_idx ON foobar_fk_1(foo); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo) REFERENCES foobar_2(foo); + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo) REFERENCES foobar_fk_1(foo); `, }, vanillaExpectations: expectations{ @@ -73,17 +104,15 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ CHECK ( fizz > 0 ), PRIMARY KEY (foo, id) ) PARTITION BY LIST (foo); - + -- partitions CREATE TABLE "FOOBAR_1" PARTITION OF "Foobar"( foo NOT NULL, bar NOT NULL ) FOR VALUES IN ('foo_1'); CREATE TABLE foobar_2 PARTITION OF "Foobar" FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF "Foobar" FOR VALUES IN ('foo_3'); - -- partitioned indexes CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, fizz); - -- local indexes CREATE INDEX foobar_1_local_idx ON "FOOBAR_1"(foo); CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); @@ -100,17 +129,15 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ CHECK ( fizz > 0 ), PRIMARY KEY (foo, id) ) PARTITION BY LIST (foo); - CREATE TABLE "FOOBAR_1" PARTITION OF "Foobar"( foo NOT NULL, bar NOT NULL ) FOR VALUES IN ('foo_1'); + -- partitions CREATE TABLE foobar_2 PARTITION OF "Foobar" FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF "Foobar" FOR VALUES IN ('foo_3'); - -- partitioned indexes CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, fizz); - -- local indexes CREATE INDEX foobar_1_local_idx ON "FOOBAR_1"(foo); CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); @@ -124,12 +151,13 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ newSchemaDDL: []string{ ` CREATE TABLE "Foobar"( - id INT, + id INT, + fizz SERIAL, foo VARCHAR(255), bar TEXT, - fizz SERIAL, CHECK ( fizz > 0 ) ) PARTITION BY LIST (foo); + -- partitions CREATE TABLE "FOOBAR_1" PARTITION OF "Foobar"( foo NOT NULL, bar NOT NULL, @@ -141,46 +169,31 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ CREATE TABLE foobar_3 PARTITION OF "Foobar"( PRIMARY KEY (foo, fizz) ) FOR VALUES IN ('foo_3'); - -- partitioned indexes - CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, fizz); - + CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, bar); -- local indexes CREATE INDEX foobar_1_local_idx ON "FOOBAR_1"(foo); CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); - `, - }, - dataPackingExpectations: expectations{ - outputState: []string{ - ` - CREATE TABLE "Foobar"( - id INT, - fizz SERIAL, - foo VARCHAR(255), - bar TEXT, - CHECK ( fizz > 0 ) - ) PARTITION BY LIST (foo); - CREATE TABLE "FOOBAR_1" PARTITION OF "Foobar"( - foo NOT NULL, - bar NOT NULL, - PRIMARY KEY (foo, id) - ) FOR VALUES IN ('foo_1'); - CREATE TABLE foobar_2 PARTITION OF "Foobar"( - PRIMARY KEY (foo, bar) - ) FOR VALUES IN ('foo_2'); - CREATE TABLE foobar_3 PARTITION OF "Foobar"( - PRIMARY KEY (foo, fizz) - ) FOR VALUES IN ('foo_3'); - - -- partitioned indexes - CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, fizz); - - -- local indexes - CREATE INDEX foobar_1_local_idx ON "FOOBAR_1"(foo); - CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); - `, - }, + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + -- partitions + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- local indexes + CREATE UNIQUE INDEX foobar_1_local_unique_idx ON foobar_fk_1(foo); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES "Foobar"(foo, bar); + ALTER TABLE "Foobar" ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo) REFERENCES foobar_2(foo); + ALTER TABLE "FOOBAR_1" ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo) REFERENCES foobar_fk_1(foo); + `, }, }, { @@ -200,15 +213,33 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ foo NOT NULL, bar NOT NULL ) FOR VALUES IN ('foo_1'); + -- partitions CREATE TABLE foobar_2 PARTITION OF "Foobar" FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF "Foobar" FOR VALUES IN ('foo_3'); - -- partitioned indexes - CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, fizz); - + CREATE UNIQUE INDEX foobar_unique_idx ON "Foobar"(foo, bar); -- local indexes CREATE INDEX foobar_1_local_idx ON "FOOBAR_1"(foo); CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + -- partitions + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- local indexes + CREATE UNIQUE INDEX foobar_1_local_unique_idx ON foobar_fk_1(foo); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES "Foobar"(foo, bar); + ALTER TABLE "Foobar" ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo) REFERENCES foobar_2(foo); + ALTER TABLE "FOOBAR_1" ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo) REFERENCES foobar_fk_1(foo); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -217,7 +248,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ newSchemaDDL: nil, }, { - name: "Alter table: New primary key, change column types, delete unique partitioned index index, new partitioned index, delete local index, add local index, validate check constraint", + name: "Alter table: New primary key, change column types, delete partitioned index, new partitioned index, delete local index, add local index, validate check constraint, validate FK, delete FK", oldSchemaDDL: []string{ ` CREATE TABLE foobar( @@ -227,22 +258,38 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ fizz SERIAL, PRIMARY KEY (foo, id) ) PARTITION BY LIST (foo); - + -- check constraints ALTER TABLE foobar ADD CONSTRAINT some_check_constraint CHECK ( fizz > 0 ) NOT VALID; - + -- partitions CREATE TABLE foobar_1 PARTITION OF foobar( foo NOT NULL, bar NOT NULL ) FOR VALUES IN ('foo_1'); CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF foobar FOR VALUES IN ('foo_3'); - -- partitioned indexes - CREATE INDEX foobar_some_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + -- local indexes + CREATE UNIQUE INDEX foobar_1_local_unique_idx ON foobar_1(foo); + CREATE INDEX foobar_2_local_idx ON foobar_1(foo); + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); -- local indexes - CREATE INDEX foobar_1_local_idx ON foobar_1(foo); - CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); + CREATE UNIQUE INDEX foobar_fk_1_local_unique_idx ON foobar_fk_1(foo); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo) REFERENCES foobar_1(foo) NOT VALID; + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo) REFERENCES foobar_fk_1(foo) NOT VALID; `, }, newSchemaDDL: []string{ @@ -254,20 +301,35 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ fizz TEXT, PRIMARY KEY (foo, id) ) PARTITION BY LIST (foo); + -- check constraint ALTER TABLE foobar ADD CONSTRAINT some_check_constraint CHECK ( LENGTH(fizz) > 0 ); - + -- partitions CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); CREATE TABLE foobar_2 PARTITION OF foobar( bar NOT NULL ) FOR VALUES IN ('foo_2'); CREATE TABLE foobar_3 PARTITION OF foobar FOR VALUES IN ('foo_3'); - -- partitioned indexes - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz); - + CREATE UNIQUE INDEX foobar_some_idx ON foobar(foo, fizz); -- local indexes - CREATE UNIQUE INDEX foobar_2_local_unique_idx ON foobar_2(foo); + CREATE UNIQUE INDEX foobar_1_local_unique_idx ON foobar_1(foo); CREATE INDEX foobar_3_local_idx ON foobar_3(foo, bar); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- local indexes + CREATE UNIQUE INDEX foobar_fk_1_local_unique_idx ON foobar_fk_1(foo); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo) REFERENCES foobar_1(foo); + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo) REFERENCES foobar_fk_1(foo); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -324,7 +386,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX some_idx on foobar(id); - + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE FUNCTION increment_version() RETURNS TRIGGER AS $$ BEGIN @@ -339,6 +401,17 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + `, }, newSchemaDDL: []string{ @@ -352,6 +425,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE INDEX some_idx on foobar(id); + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); @@ -367,6 +441,19 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -386,6 +473,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX some_idx on foobar(id); + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE TABLE foobar_1( id INT, @@ -416,6 +504,24 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ); + CREATE TABLE foobar_fk_1( + foo VARCHAR(255), + bar TEXT + ); + + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_foo_bar_fkey FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_foo_bar_fkey FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -429,6 +535,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE INDEX some_idx on foobar(id); + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); @@ -445,6 +552,19 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -464,10 +584,10 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE INDEX some_idx on foobar(id); + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); - CREATE FUNCTION increment_version() RETURNS TRIGGER AS $$ BEGIN NEW.version = OLD.version + 1; @@ -480,6 +600,19 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -493,7 +626,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX some_idx on foobar(id); - + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE FUNCTION increment_version() RETURNS TRIGGER AS $$ BEGIN @@ -507,6 +640,17 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -526,6 +670,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE INDEX some_idx on foobar(id); + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1'); @@ -542,6 +687,19 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -555,6 +713,7 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX some_idx on foobar(id); + CREATE UNIQUE INDEX foobar_unique_idx on foobar(foo, bar); CREATE TABLE foobar_1( id INT, @@ -585,6 +744,24 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ); + CREATE TABLE foobar_fk_1( + foo VARCHAR(255), + bar TEXT + ); + + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_foo_bar_fkey FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_foo_bar_fkey FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -607,6 +784,19 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ -- partitioned indexes CREATE UNIQUE INDEX some_partitioned_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + id INT, + foo VARCHAR(255), + bar TEXT, + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -625,6 +815,23 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ -- partitioned indexes CREATE UNIQUE INDEX some_partitioned_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + id INT, + foo VARCHAR(255), + bar TEXT, + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN ('foo_1'); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo, id) REFERENCES foobar_1(foo, id); + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo, id) REFERENCES foobar_fk_1(foo, id); `, }, }, @@ -640,9 +847,21 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ bar TEXT, CHECK ( fizz > 0 ) ) PARTITION BY LIST (foo); - -- partitioned indexes CREATE UNIQUE INDEX some_partitioned_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + id INT, + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -655,13 +874,29 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ bar TEXT, CHECK ( fizz > 0 ) ) PARTITION BY LIST (foo); - + -- partitions CREATE TABLE foobar_1 PARTITION OF foobar( PRIMARY KEY (foo, bar) ) FOR VALUES IN ('foo_1'); - -- partitioned indexes CREATE UNIQUE INDEX some_partitioned_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + id INT, + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + -- partitions + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk( + PRIMARY KEY (foo, bar) + ) FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, }, @@ -743,13 +978,30 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE TABLE foobar_1 PARTITION OF foobar( - PRIMARY KEY (foo, fizz) + PRIMARY KEY (foo, bar) ) FOR VALUES IN ('foo_1'); -- partitioned indexes - CREATE INDEX some_partitioned_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo); -- local indexes CREATE INDEX some_local_idx ON foobar_1(foo, bar); + + CREATE TABLE foobar_fk( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk ( + PRIMARY KEY (foo, bar) + ) FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo); + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo) REFERENCES foobar(foo); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo) REFERENCES foobar_fk(foo); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo, bar) REFERENCES foobar_1(foo, bar); + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk_1(foo, bar); `, }, newSchemaDDL: []string{ @@ -762,13 +1014,30 @@ var partitionedTableAcceptanceTestCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE TABLE foobar_1 PARTITION OF foobar_new( - PRIMARY KEY (foo, fizz) + PRIMARY KEY (foo, bar) ) FOR VALUES IN ('foo_1'); -- partitioned indexes - CREATE INDEX some_partitioned_idx ON foobar_new(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar_new(foo); -- local indexes CREATE INDEX some_local_idx ON foobar_1(foo, bar); + + CREATE TABLE foobar_fk_new( + foo VARCHAR(255), + bar TEXT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk_new ( + PRIMARY KEY (foo, bar) + ) FOR VALUES IN ('foo_1'); + -- partitioned indexes + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk_new(foo); + -- foreign keys + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk_new ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo) REFERENCES foobar_new(foo); + ALTER TABLE foobar_new ADD CONSTRAINT foobar_fk FOREIGN KEY (foo) REFERENCES foobar_fk_new(foo); + -- local local foreign keys + ALTER TABLE foobar_fk_1 ADD CONSTRAINT foobar_fk_1_fk FOREIGN KEY (foo, bar) REFERENCES foobar_1(foo, bar); + ALTER TABLE foobar_1 ADD CONSTRAINT foobar_1_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk_1(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff --git a/internal/migration_acceptance_tests/schema_cases_test.go b/internal/migration_acceptance_tests/schema_cases_test.go index 63d0203..9132cc2 100644 --- a/internal/migration_acceptance_tests/schema_cases_test.go +++ b/internal/migration_acceptance_tests/schema_cases_test.go @@ -42,9 +42,9 @@ var schemaAcceptanceTests = []acceptanceTestCase{ CREATE TABLE foobar( id INT, - fizz SERIAL NOT NULL, foo VARCHAR(255) DEFAULT 'some default' NOT NULL CHECK (LENGTH(foo) > 0), - bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + bar SERIAL NOT NULL, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (foo, id) ) PARTITION BY LIST(foo); @@ -53,22 +53,23 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ) FOR VALUES IN ('foobar_1_val_1', 'foobar_1_val_2'); -- partitioned indexes - CREATE INDEX foobar_normal_idx ON foobar(foo DESC, fizz); + CREATE INDEX foobar_normal_idx ON foobar(foo DESC, bar); CREATE INDEX foobar_hash_idx ON foobar USING hash (foo); - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz); -- local indexes CREATE INDEX foobar_1_local_idx ON foobar_1(foo, fizz); CREATE table bar( - id VARCHAR(255) PRIMARY KEY, - foo INT DEFAULT 0 CHECK (foo > 0 AND foo > bar), + id INT PRIMARY KEY, + foo VARCHAR(255), bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL CHECK (buzz IS NOT NULL) + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + buzz REAL NOT NULL CHECK (buzz IS NOT NULL), + FOREIGN KEY (foo, fizz) REFERENCES foobar (foo, fizz) ); CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar, fizz); - CREATE UNIQUE INDEX bar_unique_idx on bar(fizz, buzz); + CREATE UNIQUE INDEX bar_unique_idx on bar(foo, buzz); `, }, newSchemaDDL: []string{ @@ -104,10 +105,10 @@ var schemaAcceptanceTests = []acceptanceTestCase{ RETURN add(a, b) + increment(a); CREATE TABLE foobar( - id INT, - fizz SERIAL NOT NULL, + id INT, foo VARCHAR(255) DEFAULT 'some default' NOT NULL CHECK (LENGTH(foo) > 0), - bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + bar SERIAL NOT NULL, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (foo, id) ) PARTITION BY LIST(foo); @@ -116,22 +117,23 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ) FOR VALUES IN ('foobar_1_val_1', 'foobar_1_val_2'); -- partitioned indexes - CREATE INDEX foobar_normal_idx ON foobar(foo DESC, fizz); + CREATE INDEX foobar_normal_idx ON foobar(foo DESC, bar); CREATE INDEX foobar_hash_idx ON foobar USING hash (foo); - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz); -- local indexes CREATE INDEX foobar_1_local_idx ON foobar_1(foo, fizz); CREATE table bar( - id VARCHAR(255) PRIMARY KEY, - foo INT DEFAULT 0 CHECK (foo > 0 AND foo > bar), + id INT PRIMARY KEY, + foo VARCHAR(255), bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL CHECK (buzz IS NOT NULL) + buzz REAL NOT NULL CHECK (buzz IS NOT NULL), + FOREIGN KEY (foo, fizz) REFERENCES foobar (foo, fizz) ); CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar, fizz); - CREATE UNIQUE INDEX bar_unique_idx on bar(fizz, buzz); + CREATE UNIQUE INDEX bar_unique_idx on bar(foo, buzz); `, }, vanillaExpectations: expectations{ @@ -184,12 +186,12 @@ var schemaAcceptanceTests = []acceptanceTestCase{ CREATE TABLE foobar( id INT PRIMARY KEY, - fizz SERIAL NOT NULL, + bar SERIAL NOT NULL, foo VARCHAR(255) DEFAULT 'some default' NOT NULL, - bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP ); CREATE INDEX foobar_normal_idx ON foobar USING hash (fizz); - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar DESC); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz DESC); CREATE TRIGGER "some trigger" BEFORE UPDATE ON foobar @@ -199,12 +201,13 @@ var schemaAcceptanceTests = []acceptanceTestCase{ CREATE table bar( id VARCHAR(255) PRIMARY KEY, - foo INT DEFAULT 0, + foo VARCHAR(255), bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + buzz REAL NOT NULL, + FOREIGN KEY (foo, fizz) REFERENCES foobar (foo, fizz) ); - ALTER TABLE bar ADD CONSTRAINT "FOO_CHECK" CHECK (foo < bar) NOT VALID; + ALTER TABLE bar ADD CONSTRAINT "FOO_CHECK" CHECK (LENGTH(foo) < bar) NOT VALID; CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar DESC, fizz DESC); CREATE UNIQUE INDEX bar_unique_idx on bar(fizz, buzz); @@ -251,13 +254,13 @@ var schemaAcceptanceTests = []acceptanceTestCase{ CREATE TABLE "New_table"( id INT PRIMARY KEY, - new_fizz SMALLSERIAL NOT NULL, + new_bar SMALLSERIAL NOT NULL, new_foo VARCHAR(255) DEFAULT '' NOT NULL CHECK ( new_foo IS NOT NULL), - new_bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + new_fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, version INT NOT NULL DEFAULT 0 ); - ALTER TABLE "New_table" ADD CONSTRAINT "new_bar_check" CHECK ( new_bar < CURRENT_TIMESTAMP - interval '1 month' ) NO INHERIT NOT VALID; - CREATE UNIQUE INDEX foobar_unique_idx ON "New_table"(new_foo, new_bar); + ALTER TABLE "New_table" ADD CONSTRAINT "new_fzz_check" CHECK ( new_fizz < CURRENT_TIMESTAMP - interval '1 month' ) NO INHERIT NOT VALID; + CREATE UNIQUE INDEX foobar_unique_idx ON "New_table"(new_foo, new_fizz); CREATE TRIGGER "some trigger" BEFORE UPDATE ON "New_table" @@ -267,13 +270,14 @@ var schemaAcceptanceTests = []acceptanceTestCase{ CREATE TABLE bar( id VARCHAR(255) PRIMARY KEY, - foo INT, + foo VARCHAR(255), bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, buzz REAL NOT NULL, - quux TEXT + quux TEXT, + FOREIGN KEY (foo, fizz) REFERENCES "New_table" (new_foo, new_fizz) ); - ALTER TABLE bar ADD CONSTRAINT "FOO_CHECK" CHECK ( foo < bar ); + ALTER TABLE bar ADD CONSTRAINT "FOO_CHECK" CHECK ( LENGTH(foo) < bar ); CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar DESC, fizz DESC); CREATE UNIQUE INDEX bar_unique_idx ON bar(fizz, buzz); @@ -294,6 +298,7 @@ var schemaAcceptanceTests = []acceptanceTestCase{ `, }, expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeDeletesData, diff.MigrationHazardTypeHasUntrackableDependencies, diff.MigrationHazardTypeIndexBuild, @@ -307,7 +312,7 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ); CREATE SEQUENCE new_foobar_sequence - AS SMALLINT + AS SMALLINT INCREMENT BY 4 MINVALUE 10 MAXVALUE 200 START WITH 20 CACHE 10 NO CYCLE @@ -339,14 +344,14 @@ var schemaAcceptanceTests = []acceptanceTestCase{ $$ language 'plpgsql'; CREATE TABLE "New_table"( - new_bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP, - id INT PRIMARY KEY, - version INT NOT NULL DEFAULT 0, - new_fizz SMALLSERIAL NOT NULL, - new_foo VARCHAR(255) DEFAULT '' NOT NULL CHECK (new_foo IS NOT NULL) + new_fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + id INT PRIMARY KEY, + version INT NOT NULL DEFAULT 0, + new_bar SMALLSERIAL NOT NULL, + new_foo VARCHAR(255) DEFAULT '' NOT NULL CHECK ( new_foo IS NOT NULL) ); - ALTER TABLE "New_table" ADD CONSTRAINT "new_bar_check" CHECK ( new_bar < CURRENT_TIMESTAMP - interval '1 month' ) NO INHERIT NOT VALID; - CREATE UNIQUE INDEX foobar_unique_idx ON "New_table"(new_foo, new_bar); + ALTER TABLE "New_table" ADD CONSTRAINT "new_fzz_check" CHECK ( new_fizz < CURRENT_TIMESTAMP - interval '1 month' ) NO INHERIT NOT VALID; + CREATE UNIQUE INDEX foobar_unique_idx ON "New_table"(new_foo, new_fizz); CREATE TRIGGER "some trigger" BEFORE UPDATE ON "New_table" @@ -355,17 +360,18 @@ var schemaAcceptanceTests = []acceptanceTestCase{ EXECUTE PROCEDURE "increment version"(); CREATE TABLE bar( - id VARCHAR(255) PRIMARY KEY, - foo INT, - bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL, - quux TEXT + id VARCHAR(255) PRIMARY KEY, + foo VARCHAR(255), + bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + buzz REAL NOT NULL, + quux TEXT, + FOREIGN KEY (foo, fizz) REFERENCES "New_table" (new_foo, new_fizz) ); - ALTER TABLE bar ADD CONSTRAINT "FOO_CHECK" CHECK ( foo < bar ); + ALTER TABLE bar ADD CONSTRAINT "FOO_CHECK" CHECK ( LENGTH(foo) < bar ); CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar DESC, fizz DESC); - CREATE UNIQUE INDEX bar_unique_idx on bar(fizz, buzz); + CREATE UNIQUE INDEX bar_unique_idx ON bar(fizz, buzz); CREATE INDEX gin_index ON bar USING gin (quux gin_trgm_ops); CREATE FUNCTION check_content() RETURNS TRIGGER AS $$ @@ -390,9 +396,9 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ` CREATE TABLE foobar( id INT, - fizz SERIAL NOT NULL, + bar SERIAL NOT NULL, foo VARCHAR(255) DEFAULT 'some default' NOT NULL CHECK (LENGTH(foo) > 0), - bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (foo, id) ) PARTITION BY LIST(foo); @@ -401,17 +407,18 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ) FOR VALUES IN ('foobar_1_val_1', 'foobar_1_val_2'); -- partitioned indexes - CREATE INDEX foobar_normal_idx ON foobar(foo, fizz); - CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + CREATE INDEX foobar_normal_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, fizz); -- local indexes - CREATE INDEX foobar_1_local_idx ON foobar_1(foo, fizz); + CREATE INDEX foobar_1_local_idx ON foobar_1(foo, bar); CREATE table bar( id VARCHAR(255) PRIMARY KEY, - foo INT DEFAULT 0 CHECK (foo > 0 AND foo > bar), + foo VARCHAR(255), bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL CHECK (buzz IS NOT NULL) + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + buzz REAL NOT NULL CHECK (buzz IS NOT NULL), + FOREIGN KEY (foo, fizz) REFERENCES foobar (foo, fizz) ); CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar, fizz); @@ -425,9 +432,9 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ` CREATE TABLE new_foobar( id INT, - fizz SERIAL NOT NULL, + bar TIMESTAMPTZ NOT NULL, foo VARCHAR(255) DEFAULT 'some default' NOT NULL CHECK (LENGTH(foo) > 0), - bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP ) PARTITION BY LIST(foo); CREATE TABLE foobar_1 PARTITION of new_foobar( @@ -436,17 +443,18 @@ var schemaAcceptanceTests = []acceptanceTestCase{ ) FOR VALUES IN ('foobar_1_val_1', 'foobar_1_val_2'); -- local indexes - CREATE INDEX foobar_1_local_idx ON foobar_1(foo, fizz); + CREATE INDEX foobar_1_local_idx ON foobar_1(foo, bar); -- partitioned indexes - CREATE INDEX foobar_normal_idx ON new_foobar(foo, fizz); - CREATE UNIQUE INDEX foobar_unique_idx ON new_foobar(foo, bar); + CREATE INDEX foobar_normal_idx ON new_foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON new_foobar(foo, fizz); CREATE table bar( id VARCHAR(255) PRIMARY KEY, - foo INT DEFAULT 0 CHECK (foo > 0 AND foo > bar), + foo VARCHAR(255), bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL CHECK (buzz IS NOT NULL) + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + buzz REAL NOT NULL CHECK (buzz IS NOT NULL), + FOREIGN KEY (foo, fizz) REFERENCES new_foobar (foo, fizz) ); CREATE INDEX bar_normal_idx ON bar(bar); CREATE INDEX bar_another_normal_id ON bar(bar, fizz); @@ -457,43 +465,45 @@ var schemaAcceptanceTests = []acceptanceTestCase{ `, }, expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeDeletesData, }, dataPackingExpectations: expectations{ outputState: []string{ ` - CREATE TABLE new_foobar( - bar TIMESTAMP DEFAULT CURRENT_TIMESTAMP, - id INT, - fizz SERIAL NOT NULL, - foo VARCHAR(255) DEFAULT 'some default' NOT NULL CHECK (LENGTH(foo) > 0) - ) PARTITION BY LIST(foo); - - CREATE TABLE foobar_1 PARTITION of new_foobar( - fizz NOT NULL, - PRIMARY KEY (foo, bar) - ) FOR VALUES IN ('foobar_1_val_1', 'foobar_1_val_2'); - - -- local indexes - CREATE INDEX foobar_1_local_idx ON foobar_1(foo, fizz); - -- partitioned indexes - CREATE INDEX foobar_normal_idx ON new_foobar(foo, fizz); - CREATE UNIQUE INDEX foobar_unique_idx ON new_foobar(foo, bar); - - CREATE table bar( - id VARCHAR(255) PRIMARY KEY, - foo INT DEFAULT 0 CHECK (foo > 0 AND foo > bar), - bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, - fizz timestamptz DEFAULT CURRENT_TIMESTAMP, - buzz REAL NOT NULL CHECK (buzz IS NOT NULL) - ); - CREATE INDEX bar_normal_idx ON bar(bar); - CREATE INDEX bar_another_normal_id ON bar(bar, fizz); - CREATE UNIQUE INDEX bar_unique_idx on bar(fizz, buzz); - - CREATE TABLE fizz( - ); - `, + CREATE TABLE new_foobar( + bar TIMESTAMPTZ NOT NULL, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + id INT, + foo VARCHAR(255) DEFAULT 'some default' NOT NULL CHECK (LENGTH(foo) > 0) + ) PARTITION BY LIST(foo); + + CREATE TABLE foobar_1 PARTITION of new_foobar( + fizz NOT NULL, + PRIMARY KEY (foo, bar) + ) FOR VALUES IN ('foobar_1_val_1', 'foobar_1_val_2'); + + -- local indexes + CREATE INDEX foobar_1_local_idx ON foobar_1(foo, bar); + -- partitioned indexes + CREATE INDEX foobar_normal_idx ON new_foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON new_foobar(foo, fizz); + + CREATE table bar( + id VARCHAR(255) PRIMARY KEY, + foo VARCHAR(255), + bar DOUBLE PRECISION NOT NULL DEFAULT 8.8, + fizz TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, + buzz REAL NOT NULL CHECK (buzz IS NOT NULL), + FOREIGN KEY (foo, fizz) REFERENCES new_foobar (foo, fizz) + ); + CREATE INDEX bar_normal_idx ON bar(bar); + CREATE INDEX bar_another_normal_id ON bar(bar, fizz); + CREATE UNIQUE INDEX bar_unique_idx on bar(fizz, buzz); + + CREATE TABLE fizz( + ); + `, }, }, }, diff --git a/internal/migration_acceptance_tests/sequence_cases_test.go b/internal/migration_acceptance_tests/sequence_cases_test.go index d258acc..8400564 100644 --- a/internal/migration_acceptance_tests/sequence_cases_test.go +++ b/internal/migration_acceptance_tests/sequence_cases_test.go @@ -3,10 +3,6 @@ package migration_acceptance_tests import "github.com/stripe/pg-schema-diff/pkg/diff" var sequenceAcceptanceTests = []acceptanceTestCase{ - // Write test case for ownership change between two separate tables and from none to something - - // write acceptance test cases for sequences - // Write noop test case { name: "No-op", oldSchemaDDL: []string{ diff --git a/internal/migration_acceptance_tests/table_cases_test.go b/internal/migration_acceptance_tests/table_cases_test.go index af883a5..e520c28 100644 --- a/internal/migration_acceptance_tests/table_cases_test.go +++ b/internal/migration_acceptance_tests/table_cases_test.go @@ -18,6 +18,15 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX normal_idx ON foobar(fizz); CREATE UNIQUE INDEX unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -31,6 +40,15 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX normal_idx ON foobar(fizz); CREATE UNIQUE INDEX unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, vanillaExpectations: expectations{ @@ -53,7 +71,16 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ buzz REAL CHECK (buzz IS NOT NULL) ); CREATE INDEX normal_idx ON foobar(fizz); - CREATE UNIQUE INDEX unique_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, dataPackingExpectations: expectations{ @@ -66,8 +93,18 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ foo VARCHAR(255) COLLATE "POSIX" DEFAULT '' NOT NULL ); CREATE INDEX normal_idx ON foobar(fizz); - CREATE UNIQUE INDEX unique_idx ON foobar(foo, bar); - `}, + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); + `, + }, }, }, { @@ -112,7 +149,16 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ buzz REAL CHECK (buzz IS NOT NULL) ); CREATE INDEX normal_idx ON foobar(fizz); - CREATE UNIQUE INDEX unique_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -140,7 +186,7 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ }, }, { - name: "Alter table: New primary key, change column types, delete unique index, new index, validate check constraint", + name: "Alter table: New primary key, change column types, delete unique index, delete FK's, new index, validate check constraint", oldSchemaDDL: []string{ ` CREATE TABLE foobar( @@ -153,7 +199,16 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ ); ALTER TABLE foobar ADD CONSTRAINT buzz_check CHECK (buzz IS NOT NULL) NOT VALID; CREATE INDEX normal_idx ON foobar(fizz); - CREATE UNIQUE INDEX unique_idx ON foobar(foo, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -169,6 +224,11 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ ALTER TABLE foobar ADD CONSTRAINT buzz_check CHECK (buzz IS NOT NULL); CREATE INDEX normal_idx ON foobar(fizz); CREATE INDEX other_idx ON foobar(bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); `, }, expectedHazardTypes: []diff.MigrationHazardType{ @@ -179,7 +239,7 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ }, }, { - name: "Alter table: New column, new primary key, alter column to nullable, alter column types, drop column, drop index, drop check constraints", + name: "Alter table: New column, new primary key, new FK, drop FK, alter column to nullable, alter column types, drop column, drop index, drop check constraints", oldSchemaDDL: []string{ ` CREATE TABLE foobar( @@ -191,6 +251,13 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ ); CREATE INDEX normal_idx ON foobar(fizz); CREATE UNIQUE INDEX unique_idx ON foobar(foo DESC, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); `, }, newSchemaDDL: []string{ @@ -202,10 +269,19 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ new_fizz DECIMAL(65, 10) DEFAULT 5.25 NOT NULL PRIMARY KEY ); CREATE INDEX other_idx ON foobar(bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo CHAR + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeImpactsDatabasePerformance, diff.MigrationHazardTypeDeletesData, diff.MigrationHazardTypeIndexDropped, @@ -224,7 +300,16 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ buzz REAL CHECK (buzz IS NOT NULL) ); CREATE INDEX normal_idx ON foobar USING hash (fizz); - CREATE UNIQUE INDEX unique_idx ON foobar(foo DESC, bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(foo, bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(foo, bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (foo, bar) REFERENCES foobar_fk(foo, bar); `, }, newSchemaDDL: []string{ @@ -237,11 +322,21 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ new_buzz REAL CHECK (new_buzz IS NOT NULL) ); CREATE INDEX normal_idx ON foobar USING hash (new_fizz); - CREATE UNIQUE INDEX unique_idx ON foobar(new_foo DESC, new_bar); + CREATE UNIQUE INDEX foobar_unique_idx ON foobar(new_foo, new_bar); + + CREATE TABLE foobar_fk( + bar TIMESTAMP, + foo VARCHAR(255) + ); + CREATE UNIQUE INDEX foobar_fk_unique_idx ON foobar_fk(foo, bar); + -- create a circular dependency of foreign keys (this is allowed) + ALTER TABLE foobar_fk ADD CONSTRAINT foobar_fk_fk FOREIGN KEY (foo, bar) REFERENCES foobar(new_foo, new_bar); + ALTER TABLE foobar ADD CONSTRAINT foobar_fk FOREIGN KEY (new_foo, new_bar) REFERENCES foobar_fk(foo, bar); `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeDeletesData, diff.MigrationHazardTypeIndexDropped, diff.MigrationHazardTypeIndexBuild, diff --git a/internal/queries/queries.sql b/internal/queries/queries.sql index 15f2523..337fe54 100644 --- a/internal/queries/queries.sql +++ b/internal/queries/queries.sql @@ -68,7 +68,9 @@ SELECT FROM pg_catalog.pg_class AS c INNER JOIN pg_catalog.pg_index AS i ON (i.indexrelid = c.oid) INNER JOIN pg_catalog.pg_class AS table_c ON (table_c.oid = i.indrelid) -LEFT JOIN pg_catalog.pg_constraint AS con ON (con.conindid = c.oid) +LEFT JOIN + pg_catalog.pg_constraint AS con + ON (con.conindid = c.oid AND con.contype IN ('p', 'u', NULL)) LEFT JOIN pg_catalog.pg_inherits AS idx_inherits ON (c.oid = idx_inherits.inhrelid) @@ -109,6 +111,35 @@ WHERE AND pg_constraint.contype = 'c' AND pg_constraint.conislocal; +-- name: GetForeignKeyConstraints :many +SELECT + pg_constraint.conname::TEXT AS constraint_name, + constraint_c.relname::TEXT AS owning_table_name, + constraint_namespace.nspname::TEXT AS owning_table_schema_name, + foreign_table_c.relname::TEXT AS foreign_table_name, + foreign_table_namespace.nspname::TEXT AS foreign_table_schema_name, + pg_constraint.convalidated AS is_valid, + pg_catalog.pg_get_constraintdef(pg_constraint.oid) AS constraint_def +FROM pg_catalog.pg_constraint +INNER JOIN + pg_catalog.pg_class AS constraint_c + ON pg_constraint.conrelid = constraint_c.oid +INNER JOIN pg_catalog.pg_namespace AS constraint_namespace + ON pg_constraint.connamespace = constraint_namespace.oid +INNER JOIN + pg_catalog.pg_class AS foreign_table_c + ON pg_constraint.confrelid = foreign_table_c.oid +INNER JOIN pg_catalog.pg_namespace AS foreign_table_namespace + ON + foreign_table_c.relnamespace = foreign_table_namespace.oid +INNER JOIN + pg_catalog.pg_class AS index_c + ON pg_constraint.conindid = index_c.oid +WHERE + constraint_namespace.nspname = 'public' + AND pg_constraint.contype = 'f' + AND pg_constraint.conislocal; + -- name: GetFunctions :many SELECT pg_proc.oid, diff --git a/internal/queries/queries.sql.go b/internal/queries/queries.sql.go index d0aee76..b859d7d 100644 --- a/internal/queries/queries.sql.go +++ b/internal/queries/queries.sql.go @@ -10,22 +10,20 @@ import ( ) const getCheckConstraints = `-- name: GetCheckConstraints :many -SELECT - pg_constraint.oid, - pg_constraint.conname::TEXT AS constraint_name, - pg_class.relname::TEXT AS table_name, - pg_constraint.convalidated AS is_valid, - pg_constraint.connoinherit AS is_not_inheritable, - pg_catalog.pg_get_expr( - pg_constraint.conbin, pg_constraint.conrelid - ) AS constraint_expression +SELECT pg_constraint.oid, + pg_constraint.conname::TEXT AS constraint_name, + pg_class.relname::TEXT AS table_name, + pg_constraint.convalidated AS is_valid, + pg_constraint.connoinherit AS is_not_inheritable, + pg_catalog.pg_get_expr( + pg_constraint.conbin, pg_constraint.conrelid + ) AS constraint_expression FROM pg_catalog.pg_constraint -INNER JOIN pg_catalog.pg_class ON pg_constraint.conrelid = pg_class.oid -WHERE - pg_class.relnamespace + INNER JOIN pg_catalog.pg_class ON pg_constraint.conrelid = pg_class.oid +WHERE pg_class.relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'public') - AND pg_constraint.contype = 'c' - AND pg_constraint.conislocal + AND pg_constraint.contype = 'c' + AND pg_constraint.conislocal ` type GetCheckConstraintsRow struct { @@ -70,9 +68,8 @@ func (q *Queries) GetCheckConstraints(ctx context.Context) ([]GetCheckConstraint const getColumnsForIndex = `-- name: GetColumnsForIndex :many SELECT a.attname::TEXT AS column_name FROM pg_catalog.pg_attribute AS a -WHERE - a.attrelid = $1 - AND a.attnum > 0 +WHERE a.attrelid = $1 + AND a.attnum > 0 ORDER BY a.attnum ` @@ -100,28 +97,26 @@ func (q *Queries) GetColumnsForIndex(ctx context.Context, attrelid interface{}) } const getColumnsForTable = `-- name: GetColumnsForTable :many -SELECT - a.attname::TEXT AS column_name, - COALESCE(coll.collname, '')::TEXT AS collation_name, - COALESCE(collation_namespace.nspname, '')::TEXT AS collation_schema_name, - COALESCE( - pg_catalog.pg_get_expr(d.adbin, d.adrelid), '' - )::TEXT AS default_value, - a.attnotnull AS is_not_null, - a.attlen AS column_size, - pg_catalog.format_type(a.atttypid, a.atttypmod) AS column_type +SELECT a.attname::TEXT AS column_name, + COALESCE(coll.collname, '')::TEXT AS collation_name, + COALESCE(collation_namespace.nspname, '')::TEXT AS collation_schema_name, + COALESCE( + pg_catalog.pg_get_expr(d.adbin, d.adrelid), '' + )::TEXT AS default_value, + a.attnotnull AS is_not_null, + a.attlen AS column_size, + pg_catalog.format_type(a.atttypid, a.atttypmod) AS column_type FROM pg_catalog.pg_attribute AS a -LEFT JOIN - pg_catalog.pg_attrdef AS d - ON (d.adrelid = a.attrelid AND d.adnum = a.attnum) -LEFT JOIN pg_catalog.pg_collation AS coll ON coll.oid = a.attcollation -LEFT JOIN - pg_catalog.pg_namespace AS collation_namespace - ON collation_namespace.oid = coll.collnamespace -WHERE - a.attrelid = $1 - AND a.attnum > 0 - AND NOT a.attisdropped + LEFT JOIN + pg_catalog.pg_attrdef AS d + ON (d.adrelid = a.attrelid AND d.adnum = a.attnum) + LEFT JOIN pg_catalog.pg_collation AS coll ON coll.oid = a.attcollation + LEFT JOIN + pg_catalog.pg_namespace AS collation_namespace + ON collation_namespace.oid = coll.collnamespace +WHERE a.attrelid = $1 + AND a.attnum > 0 + AND NOT a.attisdropped ORDER BY a.attnum ` @@ -167,24 +162,22 @@ func (q *Queries) GetColumnsForTable(ctx context.Context, attrelid interface{}) } const getDependsOnFunctions = `-- name: GetDependsOnFunctions :many -SELECT - pg_proc.proname::TEXT AS func_name, - proc_namespace.nspname::TEXT AS func_schema_name, - pg_catalog.pg_get_function_identity_arguments( - pg_proc.oid - ) AS func_identity_arguments +SELECT pg_proc.proname::TEXT AS func_name, + proc_namespace.nspname::TEXT AS func_schema_name, + pg_catalog.pg_get_function_identity_arguments( + pg_proc.oid + ) AS func_identity_arguments FROM pg_catalog.pg_depend AS depend -INNER JOIN pg_catalog.pg_proc AS pg_proc - ON - depend.refclassid = 'pg_proc'::REGCLASS - AND depend.refobjid = pg_proc.oid -INNER JOIN - pg_catalog.pg_namespace AS proc_namespace - ON pg_proc.pronamespace = proc_namespace.oid -WHERE - depend.classid = $1::REGCLASS - AND depend.objid = $2 - AND depend.deptype = 'n' + INNER JOIN pg_catalog.pg_proc AS pg_proc + ON + depend.refclassid = 'pg_proc'::REGCLASS + AND depend.refobjid = pg_proc.oid + INNER JOIN + pg_catalog.pg_namespace AS proc_namespace + ON pg_proc.pronamespace = proc_namespace.oid +WHERE depend.classid = $1::REGCLASS + AND depend.objid = $2 + AND depend.deptype = 'n' ` type GetDependsOnFunctionsParams struct { @@ -222,15 +215,14 @@ func (q *Queries) GetDependsOnFunctions(ctx context.Context, arg GetDependsOnFun } const getExtensions = `-- name: GetExtensions :many -SELECT - ext.oid, - ext.extname::TEXT AS extension_name, - ext.extversion AS extension_version, - extension_namespace.nspname::TEXT AS schema_name +SELECT ext.oid, + ext.extname::TEXT AS extension_name, + ext.extversion AS extension_version, + extension_namespace.nspname::TEXT AS schema_name FROM pg_catalog.pg_namespace AS extension_namespace -INNER JOIN - pg_catalog.pg_extension AS ext - ON ext.extnamespace = extension_namespace.oid + INNER JOIN + pg_catalog.pg_extension AS ext + ON ext.extnamespace = extension_namespace.oid WHERE extension_namespace.nspname = 'public' ` @@ -269,34 +261,98 @@ func (q *Queries) GetExtensions(ctx context.Context) ([]GetExtensionsRow, error) return items, nil } +const getForeignKeyConstraints = `-- name: GetForeignKeyConstraints :many +SELECT pg_constraint.conname::TEXT AS constraint_name, + constraint_c.relname::TEXT AS owning_table_name, + constraint_namespace.nspname::TEXT AS owning_table_schema_name, + foreign_table_c.relname::TEXT AS foreign_table_name, + foreign_table_namespace.nspname::TEXT AS foreign_table_schema_name, + pg_constraint.convalidated AS is_valid, + pg_catalog.pg_get_constraintdef(pg_constraint.oid) AS constraint_def +FROM pg_catalog.pg_constraint + INNER JOIN + pg_catalog.pg_class AS constraint_c + ON pg_constraint.conrelid = constraint_c.oid + INNER JOIN pg_catalog.pg_namespace AS constraint_namespace + ON pg_constraint.connamespace = constraint_namespace.oid + INNER JOIN + pg_catalog.pg_class AS foreign_table_c + ON pg_constraint.confrelid = foreign_table_c.oid + INNER JOIN pg_catalog.pg_namespace AS foreign_table_namespace + ON + foreign_table_c.relnamespace = foreign_table_namespace.oid + INNER JOIN pg_catalog.pg_class as index_c ON pg_constraint.conindid = index_c.oid +WHERE constraint_namespace.nspname = 'public' + AND pg_constraint.contype = 'f' + AND pg_constraint.conislocal +` + +type GetForeignKeyConstraintsRow struct { + ConstraintName string + OwningTableName string + OwningTableSchemaName string + ForeignTableName string + ForeignTableSchemaName string + IsValid bool + ConstraintDef string +} + +func (q *Queries) GetForeignKeyConstraints(ctx context.Context) ([]GetForeignKeyConstraintsRow, error) { + rows, err := q.db.QueryContext(ctx, getForeignKeyConstraints) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetForeignKeyConstraintsRow + for rows.Next() { + var i GetForeignKeyConstraintsRow + if err := rows.Scan( + &i.ConstraintName, + &i.OwningTableName, + &i.OwningTableSchemaName, + &i.ForeignTableName, + &i.ForeignTableSchemaName, + &i.IsValid, + &i.ConstraintDef, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getFunctions = `-- name: GetFunctions :many -SELECT - pg_proc.oid, - pg_proc.proname::TEXT AS func_name, - proc_namespace.nspname::TEXT AS func_schema_name, - proc_lang.lanname::TEXT AS func_lang, - pg_catalog.pg_get_function_identity_arguments( - pg_proc.oid - ) AS func_identity_arguments, - pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def +SELECT pg_proc.oid, + pg_proc.proname::TEXT AS func_name, + proc_namespace.nspname::TEXT AS func_schema_name, + proc_lang.lanname::TEXT AS func_lang, + pg_catalog.pg_get_function_identity_arguments( + pg_proc.oid + ) AS func_identity_arguments, + pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def FROM pg_catalog.pg_proc -INNER JOIN - pg_catalog.pg_namespace AS proc_namespace - ON pg_proc.pronamespace = proc_namespace.oid -INNER JOIN - pg_catalog.pg_language AS proc_lang - ON proc_lang.oid = pg_proc.prolang -WHERE - proc_namespace.nspname = 'public' - AND pg_proc.prokind = 'f' - -- Exclude functions belonging to extensions - AND NOT EXISTS ( + INNER JOIN + pg_catalog.pg_namespace AS proc_namespace + ON pg_proc.pronamespace = proc_namespace.oid + INNER JOIN + pg_catalog.pg_language AS proc_lang + ON proc_lang.oid = pg_proc.prolang +WHERE proc_namespace.nspname = 'public' + AND pg_proc.prokind = 'f' + -- Exclude functions belonging to extensions + AND NOT EXISTS( SELECT depend.objid FROM pg_catalog.pg_depend AS depend - WHERE - depend.classid = 'pg_proc'::REGCLASS - AND depend.objid = pg_proc.oid - AND depend.deptype = 'e' + WHERE depend.classid = 'pg_proc'::REGCLASS + AND depend.objid = pg_proc.oid + AND depend.deptype = 'e' ) ` @@ -340,34 +396,32 @@ func (q *Queries) GetFunctions(ctx context.Context) ([]GetFunctionsRow, error) { } const getIndexes = `-- name: GetIndexes :many -SELECT - c.oid AS oid, - c.relname::TEXT AS index_name, - table_c.relname::TEXT AS table_name, - pg_catalog.pg_get_indexdef(c.oid)::TEXT AS def_stmt, - COALESCE(con.conname, '')::TEXT AS constraint_name, - i.indisvalid AS index_is_valid, - i.indisprimary AS index_is_pk, - i.indisunique AS index_is_unique, - COALESCE(parent_c.relname, '')::TEXT AS parent_index_name, - COALESCE(parent_namespace.nspname, '')::TEXT AS parent_index_schema_name +SELECT c.oid AS oid, + c.relname::TEXT AS index_name, + table_c.relname::TEXT AS table_name, + pg_catalog.pg_get_indexdef(c.oid)::TEXT AS def_stmt, + COALESCE(con.conname, '')::TEXT AS constraint_name, + i.indisvalid AS index_is_valid, + i.indisprimary AS index_is_pk, + i.indisunique AS index_is_unique, + COALESCE(parent_c.relname, '')::TEXT AS parent_index_name, + COALESCE(parent_namespace.nspname, '')::TEXT AS parent_index_schema_name FROM pg_catalog.pg_class AS c -INNER JOIN pg_catalog.pg_index AS i ON (i.indexrelid = c.oid) -INNER JOIN pg_catalog.pg_class AS table_c ON (table_c.oid = i.indrelid) -LEFT JOIN pg_catalog.pg_constraint AS con ON (con.conindid = c.oid) -LEFT JOIN - pg_catalog.pg_inherits AS idx_inherits - ON (c.oid = idx_inherits.inhrelid) -LEFT JOIN - pg_catalog.pg_class AS parent_c - ON (idx_inherits.inhparent = parent_c.oid) -LEFT JOIN - pg_catalog.pg_namespace AS parent_namespace - ON parent_c.relnamespace = parent_namespace.oid -WHERE - c.relnamespace + INNER JOIN pg_catalog.pg_index AS i ON (i.indexrelid = c.oid) + INNER JOIN pg_catalog.pg_class AS table_c ON (table_c.oid = i.indrelid) + LEFT JOIN pg_catalog.pg_constraint AS con ON (con.conindid = c.oid AND con.contype IN ('p', 'u', NULL)) + LEFT JOIN + pg_catalog.pg_inherits AS idx_inherits + ON (c.oid = idx_inherits.inhrelid) + LEFT JOIN + pg_catalog.pg_class AS parent_c + ON (idx_inherits.inhparent = parent_c.oid) + LEFT JOIN + pg_catalog.pg_namespace AS parent_namespace + ON parent_c.relnamespace = parent_namespace.oid +WHERE c.relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'public') - AND (c.relkind = 'i' OR c.relkind = 'I') + AND (c.relkind = 'i' OR c.relkind = 'I') ` type GetIndexesRow struct { @@ -418,45 +472,43 @@ func (q *Queries) GetIndexes(ctx context.Context) ([]GetIndexesRow, error) { } const getSequences = `-- name: GetSequences :many -SELECT - seq_c.relname::TEXT AS sequence_name, - seq_ns.nspname::TEXT AS sequence_schema_name, - COALESCE(owner_attr.attname, '')::TEXT AS owner_column_name, - COALESCE(owner_ns.nspname, '')::TEXT AS owner_schema_name, - COALESCE(owner_c.relname, '')::TEXT AS owner_table_name, - pg_seq.seqstart AS start_value, - pg_seq.seqincrement AS increment_value, - pg_seq.seqmax AS max_value, - pg_seq.seqmin AS min_value, - pg_seq.seqcache AS cache_size, - pg_seq.seqcycle AS is_cycle, - FORMAT_TYPE(pg_seq.seqtypid, NULL) AS data_type +SELECT seq_c.relname::TEXT AS sequence_name, + seq_ns.nspname::TEXT AS sequence_schema_name, + COALESCE(owner_attr.attname, '')::TEXT AS owner_column_name, + COALESCE(owner_ns.nspname, '')::TEXT AS owner_schema_name, + COALESCE(owner_c.relname, '')::TEXT AS owner_table_name, + pg_seq.seqstart AS start_value, + pg_seq.seqincrement AS increment_value, + pg_seq.seqmax AS max_value, + pg_seq.seqmin AS min_value, + pg_seq.seqcache AS cache_size, + pg_seq.seqcycle AS is_cycle, + FORMAT_TYPE(pg_seq.seqtypid, NULL) AS data_type FROM pg_catalog.pg_sequence AS pg_seq -INNER JOIN pg_catalog.pg_class AS seq_c ON pg_seq.seqrelid = seq_c.oid -INNER JOIN pg_catalog.pg_namespace AS seq_ns ON seq_c.relnamespace = seq_ns.oid -LEFT JOIN pg_catalog.pg_depend AS depend - ON - depend.classid = 'pg_class'::REGCLASS - AND pg_seq.seqrelid = depend.objid - AND depend.refclassid = 'pg_class'::REGCLASS - AND depend.deptype = 'a' -LEFT JOIN pg_catalog.pg_attribute AS owner_attr - ON - depend.refobjid = owner_attr.attrelid - AND depend.refobjsubid = owner_attr.attnum -LEFT JOIN pg_catalog.pg_class AS owner_c ON depend.refobjid = owner_c.oid -LEFT JOIN - pg_catalog.pg_namespace AS owner_ns - ON owner_c.relnamespace = owner_ns.oid + INNER JOIN pg_catalog.pg_class AS seq_c ON pg_seq.seqrelid = seq_c.oid + INNER JOIN pg_catalog.pg_namespace AS seq_ns ON seq_c.relnamespace = seq_ns.oid + LEFT JOIN pg_catalog.pg_depend AS depend + ON + depend.classid = 'pg_class'::REGCLASS + AND pg_seq.seqrelid = depend.objid + AND depend.refclassid = 'pg_class'::REGCLASS + AND depend.deptype = 'a' + LEFT JOIN pg_catalog.pg_attribute AS owner_attr + ON + depend.refobjid = owner_attr.attrelid + AND depend.refobjsubid = owner_attr.attnum + LEFT JOIN pg_catalog.pg_class AS owner_c ON depend.refobjid = owner_c.oid + LEFT JOIN + pg_catalog.pg_namespace AS owner_ns + ON owner_c.relnamespace = owner_ns.oid WHERE seq_ns.nspname = 'public' -AND NOT EXISTS ( - SELECT ext_depend.objid - FROM pg_catalog.pg_depend AS ext_depend - WHERE - ext_depend.classid = 'pg_class'::REGCLASS - AND ext_depend.objid = pg_seq.seqrelid - AND ext_depend.deptype = 'e' -) + AND NOT EXISTS( + SELECT ext_depend.objid + FROM pg_catalog.pg_depend AS ext_depend + WHERE ext_depend.classid = 'pg_class'::REGCLASS + AND ext_depend.objid = pg_seq.seqrelid + AND ext_depend.deptype = 'e' + ) ` type GetSequencesRow struct { @@ -512,34 +564,32 @@ func (q *Queries) GetSequences(ctx context.Context) ([]GetSequencesRow, error) { } const getTables = `-- name: GetTables :many -SELECT - c.oid AS oid, - c.relname::TEXT AS table_name, - COALESCE(parent_c.relname, '')::TEXT AS parent_table_name, - COALESCE(parent_namespace.nspname, '')::TEXT AS parent_table_schema_name, - (CASE - WHEN c.relkind = 'p' THEN pg_catalog.pg_get_partkeydef(c.oid) - ELSE '' - END)::TEXT - AS partition_key_def, - (CASE - WHEN c.relispartition THEN pg_catalog.pg_get_expr(c.relpartbound, c.oid) - ELSE '' - END)::TEXT AS partition_for_values +SELECT c.oid AS oid, + c.relname::TEXT AS table_name, + COALESCE(parent_c.relname, '')::TEXT AS parent_table_name, + COALESCE(parent_namespace.nspname, '')::TEXT AS parent_table_schema_name, + (CASE + WHEN c.relkind = 'p' THEN pg_catalog.pg_get_partkeydef(c.oid) + ELSE '' + END)::TEXT + AS partition_key_def, + (CASE + WHEN c.relispartition THEN pg_catalog.pg_get_expr(c.relpartbound, c.oid) + ELSE '' + END)::TEXT AS partition_for_values FROM pg_catalog.pg_class AS c -LEFT JOIN - pg_catalog.pg_inherits AS table_inherits - ON table_inherits.inhrelid = c.oid -LEFT JOIN - pg_catalog.pg_class AS parent_c - ON table_inherits.inhparent = parent_c.oid -LEFT JOIN - pg_catalog.pg_namespace AS parent_namespace - ON parent_c.relnamespace = parent_namespace.oid -WHERE - c.relnamespace + LEFT JOIN + pg_catalog.pg_inherits AS table_inherits + ON table_inherits.inhrelid = c.oid + LEFT JOIN + pg_catalog.pg_class AS parent_c + ON table_inherits.inhparent = parent_c.oid + LEFT JOIN + pg_catalog.pg_namespace AS parent_namespace + ON parent_c.relnamespace = parent_namespace.oid +WHERE c.relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'public') - AND (c.relkind = 'r' OR c.relkind = 'p') + AND (c.relkind = 'r' OR c.relkind = 'p') ` type GetTablesRow struct { @@ -582,30 +632,28 @@ func (q *Queries) GetTables(ctx context.Context) ([]GetTablesRow, error) { } const getTriggers = `-- name: GetTriggers :many -SELECT - trig.tgname::TEXT AS trigger_name, - owning_c.relname::TEXT AS owning_table_name, - owning_c_namespace.nspname::TEXT AS owning_table_schema_name, - pg_proc.proname::TEXT AS func_name, - proc_namespace.nspname::TEXT AS func_schema_name, - pg_catalog.pg_get_function_identity_arguments( - pg_proc.oid - ) AS func_identity_arguments, - pg_catalog.pg_get_triggerdef(trig.oid) AS trigger_def +SELECT trig.tgname::TEXT AS trigger_name, + owning_c.relname::TEXT AS owning_table_name, + owning_c_namespace.nspname::TEXT AS owning_table_schema_name, + pg_proc.proname::TEXT AS func_name, + proc_namespace.nspname::TEXT AS func_schema_name, + pg_catalog.pg_get_function_identity_arguments( + pg_proc.oid + ) AS func_identity_arguments, + pg_catalog.pg_get_triggerdef(trig.oid) AS trigger_def FROM pg_catalog.pg_trigger AS trig -INNER JOIN pg_catalog.pg_class AS owning_c ON trig.tgrelid = owning_c.oid -INNER JOIN - pg_catalog.pg_namespace AS owning_c_namespace - ON owning_c.relnamespace = owning_c_namespace.oid -INNER JOIN pg_catalog.pg_proc AS pg_proc ON trig.tgfoid = pg_proc.oid -INNER JOIN - pg_catalog.pg_namespace AS proc_namespace - ON pg_proc.pronamespace = proc_namespace.oid -WHERE - proc_namespace.nspname = 'public' - AND owning_c_namespace.nspname = 'public' - AND trig.tgparentid = 0 - AND NOT trig.tgisinternal + INNER JOIN pg_catalog.pg_class AS owning_c ON trig.tgrelid = owning_c.oid + INNER JOIN + pg_catalog.pg_namespace AS owning_c_namespace + ON owning_c.relnamespace = owning_c_namespace.oid + INNER JOIN pg_catalog.pg_proc AS pg_proc ON trig.tgfoid = pg_proc.oid + INNER JOIN + pg_catalog.pg_namespace AS proc_namespace + ON pg_proc.pronamespace = proc_namespace.oid +WHERE proc_namespace.nspname = 'public' + AND owning_c_namespace.nspname = 'public' + AND trig.tgparentid = 0 + AND NOT trig.tgisinternal ` type GetTriggersRow struct { diff --git a/internal/schema/schema.go b/internal/schema/schema.go index df82b1e..cb0c5c1 100644 --- a/internal/schema/schema.go +++ b/internal/schema/schema.go @@ -44,13 +44,13 @@ func (o SchemaQualifiedName) IsEmpty() bool { } type Schema struct { - Extensions []Extension - Tables []Table - Indexes []Index - - Sequences []Sequence - Functions []Function - Triggers []Trigger + Extensions []Extension + Tables []Table + Indexes []Index + ForeignKeyConstraints []ForeignKeyConstraint + Sequences []Sequence + Functions []Function + Triggers []Trigger } // Normalize normalizes the schema (alphabetically sorts tables and columns in tables) @@ -71,6 +71,7 @@ func (s Schema) Normalize() Schema { s.Tables = normTables s.Indexes = sortSchemaObjectsByName(s.Indexes) + s.ForeignKeyConstraints = sortSchemaObjectsByName(s.ForeignKeyConstraints) s.Sequences = sortSchemaObjectsByName(s.Sequences) s.Extensions = sortSchemaObjectsByName(s.Extensions) @@ -221,12 +222,28 @@ func (c CheckConstraint) GetName() string { return c.Name } +type ForeignKeyConstraint struct { + EscapedName string + OwningTable SchemaQualifiedName + // TableUnescapedName is a hackaround until we switch over Tables to use fully-qualified, escaped names + OwningTableUnescapedName string + ForeignTable SchemaQualifiedName + // ForeignTableUnescapedName is hackaround for the same as above + ForeignTableUnescapedName string + ConstraintDef string + IsValid bool +} + +func (f ForeignKeyConstraint) GetName() string { + return f.OwningTable.GetFQEscapedName() + "_" + f.EscapedName +} + type ( // SequenceOwner represents the owner of a sequence. Once we remove TableUnescapedName, we can replace it with // ColumnIdentifier struct that can also be used in the Column struct SequenceOwner struct { TableName SchemaQualifiedName - // TableUnescapedName is a hackaround until we switch over Tables ot use fully-qualified, escaped names + // TableUnescapedName is a hackaround until we switch over Tables to use fully-qualified, escaped names TableUnescapedName string ColumnName string } @@ -308,6 +325,11 @@ func GetPublicSchema(ctx context.Context, db queries.DBTX) (Schema, error) { return Schema{}, fmt.Errorf("fetchIndexes: %w", err) } + fkCons, err := fetchForeignKeyCons(ctx, q) + if err != nil { + return Schema{}, fmt.Errorf("fetchForeignKeyCons: %w", err) + } + sequences, err := fetchSequences(ctx, q) if err != nil { return Schema{}, fmt.Errorf("fetchSequences: %w", err) @@ -324,12 +346,13 @@ func GetPublicSchema(ctx context.Context, db queries.DBTX) (Schema, error) { } return Schema{ - Extensions: extensions, - Tables: tables, - Indexes: indexes, - Sequences: sequences, - Functions: functions, - Triggers: triggers, + Extensions: extensions, + Tables: tables, + Indexes: indexes, + ForeignKeyConstraints: fkCons, + Sequences: sequences, + Functions: functions, + Triggers: triggers, }, nil } @@ -476,6 +499,33 @@ func fetchIndexes(ctx context.Context, q *queries.Queries) ([]Index, error) { return indexes, nil } +func fetchForeignKeyCons(ctx context.Context, q *queries.Queries) ([]ForeignKeyConstraint, error) { + rawFkCons, err := q.GetForeignKeyConstraints(ctx) + if err != nil { + return nil, fmt.Errorf("GetForeignKeyConstraints: %w", err) + } + + var fkCons []ForeignKeyConstraint + for _, rawFkCon := range rawFkCons { + fkCons = append(fkCons, ForeignKeyConstraint{ + EscapedName: EscapeIdentifier(rawFkCon.ConstraintName), + OwningTable: SchemaQualifiedName{ + SchemaName: rawFkCon.OwningTableSchemaName, + EscapedName: EscapeIdentifier(rawFkCon.OwningTableName), + }, + OwningTableUnescapedName: rawFkCon.OwningTableName, + ForeignTable: SchemaQualifiedName{ + SchemaName: rawFkCon.ForeignTableSchemaName, + EscapedName: EscapeIdentifier(rawFkCon.ForeignTableName), + }, + ForeignTableUnescapedName: rawFkCon.ForeignTableName, + ConstraintDef: rawFkCon.ConstraintDef, + IsValid: rawFkCon.IsValid, + }) + } + return fkCons, nil +} + func fetchSequences(ctx context.Context, q *queries.Queries) ([]Sequence, error) { rawSeqs, err := q.GetSequences(ctx) if err != nil { diff --git a/internal/schema/schema_test.go b/internal/schema/schema_test.go index 76d62fe..c0de6ca 100644 --- a/internal/schema/schema_test.go +++ b/internal/schema/schema_test.go @@ -63,11 +63,12 @@ var ( RETURN add(a, b) + increment(a); CREATE TABLE foo ( - id SERIAL PRIMARY KEY, + id SERIAL, author TEXT COLLATE "C", content TEXT NOT NULL DEFAULT '', created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP CHECK (created_at > CURRENT_TIMESTAMP - interval '1 month') NO INHERIT, version INT NOT NULL DEFAULT 0, + PRIMARY KEY(id, version), CHECK ( function_with_dependencies(id, id) > 0) ); @@ -88,8 +89,17 @@ var ( FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foo_fk( + id INT, + version INT + ); + ALTER TABLE foo_fk ADD CONSTRAINT foo_fk_fk FOREIGN KEY (id, version) REFERENCES foo (id, version) + ON UPDATE CASCADE + ON DELETE CASCADE + NOT VALID; `}, - expectedHash: "59c2e1ea7460fbbb", + expectedHash: "32a7d0fd6fb7d9f1", expectedSchema: schema.Schema{ Extensions: []schema.Extension{ { @@ -124,6 +134,48 @@ var ( }, }, }, + { + Name: "foo_fk", + Columns: []schema.Column{ + { + Name: "id", + Type: "integer", + Collation: schema.SchemaQualifiedName{}, + Default: "", + IsNullable: true, + Size: 4, + }, + { + Name: "version", + Type: "integer", + Collation: schema.SchemaQualifiedName{}, + Default: "", + IsNullable: true, + Size: 4, + }, + }, + CheckConstraints: nil, + PartitionKeyDef: "", + ParentTableName: "", + ForValues: "", + }, + }, + ForeignKeyConstraints: []schema.ForeignKeyConstraint{ + { + EscapedName: "\"foo_fk_fk\"", + OwningTable: schema.SchemaQualifiedName{ + SchemaName: "public", + EscapedName: "\"foo_fk\"", + }, + OwningTableUnescapedName: "foo_fk", + ForeignTable: schema.SchemaQualifiedName{ + SchemaName: "public", + EscapedName: "\"foo\"", + }, + ForeignTableUnescapedName: "foo", + ConstraintDef: "FOREIGN KEY (id, version) REFERENCES foo(id, version) ON UPDATE CASCADE ON DELETE CASCADE NOT VALID", + IsValid: false, + }, }, Sequences: []schema.Sequence{ { @@ -156,11 +208,11 @@ var ( { TableName: "foo", Name: "foo_pkey", - Columns: []string{"id"}, + Columns: []string{"id", "version"}, IsPk: true, IsUnique: true, ConstraintName: "foo_pkey", - GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foo USING btree (id)", + GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foo USING btree (id, version)", }, { TableName: "foo", @@ -227,6 +279,7 @@ var ( content TEXT DEFAULT '', genre VARCHAR(256) NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP CHECK (created_at > CURRENT_TIMESTAMP - interval '1 month'), + version INT NOT NULL DEFAULT 0, PRIMARY KEY (author, id) ) PARTITION BY LIST (author); ALTER TABLE foo ADD CONSTRAINT author_check CHECK (author IS NOT NULL AND LENGTH(author) > 0) NOT VALID; @@ -256,8 +309,8 @@ var ( CREATE INDEX some_invalid_idx ON ONLY foo(author, genre); -- local indexes - CREATE UNIQUE INDEX foo_1_local_idx ON foo_1(author DESC, id); - CREATE UNIQUE INDEX foo_2_local_idx ON foo_2(author, content); + CREATE UNIQUE INDEX foo_1_local_idx ON foo_1(author, content); + CREATE UNIQUE INDEX foo_2_local_idx ON foo_2(author DESC, id); CREATE UNIQUE INDEX foo_3_local_idx ON foo_3(author, created_at); CREATE TRIGGER some_partition_trigger @@ -266,8 +319,20 @@ var ( WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE increment_version(); + + CREATE TABLE foo_fk( + author TEXT, + id INT, + content TEXT DEFAULT '' + ) PARTITION BY LIST (author); + CREATE TABLE foo_fk_1 PARTITION OF foo_fk FOR VALUES IN ('some author 1'); + ALTER TABLE foo_fk ADD CONSTRAINT foo_fk_fk FOREIGN KEY (author, id) REFERENCES foo (author, id) + ON UPDATE CASCADE + ON DELETE CASCADE; + ALTER TABLE foo_fk_1 ADD CONSTRAINT foo_fk_1_fk FOREIGN KEY (author, content) REFERENCES foo_1 (author, content) + NOT VALID; `}, - expectedHash: "58b60e7d949ba226", + expectedHash: "1aa0036e467663ce", expectedSchema: schema.Schema{ Tables: []schema.Table{ { @@ -278,6 +343,7 @@ var ( {Name: "content", Type: "text", Default: "''::text", IsNullable: true, Size: -1, Collation: defaultCollation}, {Name: "genre", Type: "character varying(256)", Size: -1, Collation: defaultCollation}, {Name: "created_at", Type: "timestamp without time zone", Default: "CURRENT_TIMESTAMP", Size: 8}, + {Name: "version", Type: "integer", Default: "0", Size: 4}, }, CheckConstraints: []schema.CheckConstraint{ {Name: "author_check", Expression: "((author IS NOT NULL) AND (length(author) > 0))", IsInheritable: true}, @@ -295,6 +361,7 @@ var ( {Name: "content", Type: "text", Default: "''::text", Size: -1, Collation: defaultCollation}, {Name: "genre", Type: "character varying(256)", Size: -1, Collation: defaultCollation}, {Name: "created_at", Type: "timestamp without time zone", Default: "CURRENT_TIMESTAMP", Size: 8}, + {Name: "version", Type: "integer", Default: "0", Size: 4}, }, CheckConstraints: nil, ForValues: "FOR VALUES IN ('some author 1')", @@ -308,6 +375,7 @@ var ( {Name: "content", Type: "text", Default: "''::text", IsNullable: true, Size: -1, Collation: defaultCollation}, {Name: "genre", Type: "character varying(256)", Size: -1, Collation: defaultCollation}, {Name: "created_at", Type: "timestamp without time zone", Default: "CURRENT_TIMESTAMP", Size: 8}, + {Name: "version", Type: "integer", Default: "0", Size: 4}, }, CheckConstraints: nil, ForValues: "FOR VALUES IN ('some author 2')", @@ -321,10 +389,77 @@ var ( {Name: "content", Type: "text", Default: "''::text", IsNullable: true, Size: -1, Collation: defaultCollation}, {Name: "genre", Type: "character varying(256)", Size: -1, Collation: defaultCollation}, {Name: "created_at", Type: "timestamp without time zone", Default: "CURRENT_TIMESTAMP", Size: 8}, + {Name: "version", Type: "integer", Default: "0", Size: 4}, }, CheckConstraints: nil, ForValues: "FOR VALUES IN ('some author 3')", }, + { + Name: "foo_fk", + Columns: []schema.Column{ + { + Name: "author", + Type: "text", + Collation: schema.SchemaQualifiedName{SchemaName: "pg_catalog", EscapedName: "\"default\""}, + Default: "", + IsNullable: true, + Size: -1, + }, + { + Name: "id", + Type: "integer", + Collation: schema.SchemaQualifiedName{}, + Default: "", + IsNullable: true, + Size: 4, + }, + { + Name: "content", + Type: "text", + Collation: schema.SchemaQualifiedName{SchemaName: "pg_catalog", EscapedName: "\"default\""}, + Default: "''::text", + IsNullable: true, + Size: -1, + }, + }, + CheckConstraints: nil, + PartitionKeyDef: "LIST (author)", + ParentTableName: "", + ForValues: "", + }, + { + Name: "foo_fk_1", + Columns: []schema.Column{ + { + Name: "author", + Type: "text", + Collation: schema.SchemaQualifiedName{SchemaName: "pg_catalog", EscapedName: "\"default\""}, + Default: "", + IsNullable: true, + Size: -1, + }, + { + Name: "id", + Type: "integer", + Collation: schema.SchemaQualifiedName{}, + Default: "", + IsNullable: true, + Size: 4, + }, + { + Name: "content", + Type: "text", + Collation: schema.SchemaQualifiedName{SchemaName: "pg_catalog", EscapedName: "\"default\""}, + Default: "''::text", + IsNullable: true, + Size: -1, + }, + }, + CheckConstraints: nil, + PartitionKeyDef: "", + ParentTableName: "foo_fk", + ForValues: "FOR VALUES IN ('some author 1')", + }, }, Indexes: []schema.Index{ { @@ -360,8 +495,8 @@ var ( }, { TableName: "foo_1", - Name: "foo_1_local_idx", Columns: []string{"author", "id"}, IsUnique: true, - GetIndexDefStmt: "CREATE UNIQUE INDEX foo_1_local_idx ON public.foo_1 USING btree (author DESC, id)", + Name: "foo_1_local_idx", Columns: []string{"author", "content"}, IsUnique: true, + GetIndexDefStmt: "CREATE UNIQUE INDEX foo_1_local_idx ON public.foo_1 USING btree (author, content)", }, { TableName: "foo_1", @@ -381,8 +516,8 @@ var ( }, { TableName: "foo_2", - Name: "foo_2_local_idx", Columns: []string{"author", "content"}, IsUnique: true, - GetIndexDefStmt: "CREATE UNIQUE INDEX foo_2_local_idx ON public.foo_2 USING btree (author, content)", + Name: "foo_2_local_idx", Columns: []string{"author", "id"}, IsUnique: true, + GetIndexDefStmt: "CREATE UNIQUE INDEX foo_2_local_idx ON public.foo_2 USING btree (author DESC, id)", }, { TableName: "foo_2", @@ -411,6 +546,26 @@ var ( GetIndexDefStmt: "CREATE UNIQUE INDEX foo_3_pkey ON public.foo_3 USING btree (author, id)", }, }, + ForeignKeyConstraints: []schema.ForeignKeyConstraint{ + { + EscapedName: "\"foo_fk_fk\"", + OwningTable: schema.SchemaQualifiedName{SchemaName: "public", EscapedName: "\"foo_fk\""}, + OwningTableUnescapedName: "foo_fk", + ForeignTable: schema.SchemaQualifiedName{SchemaName: "public", EscapedName: "\"foo\""}, + ForeignTableUnescapedName: "foo", + ConstraintDef: "FOREIGN KEY (author, id) REFERENCES foo(author, id) ON UPDATE CASCADE ON DELETE CASCADE", + IsValid: true, + }, + { + EscapedName: "\"foo_fk_1_fk\"", + OwningTable: schema.SchemaQualifiedName{SchemaName: "public", EscapedName: "\"foo_fk_1\""}, + OwningTableUnescapedName: "foo_fk_1", + ForeignTable: schema.SchemaQualifiedName{SchemaName: "public", EscapedName: "\"foo_1\""}, + ForeignTableUnescapedName: "foo_1", + ConstraintDef: "FOREIGN KEY (author, content) REFERENCES foo_1(author, content) NOT VALID", + IsValid: false, + }, + }, Sequences: []schema.Sequence{ { SchemaQualifiedName: schema.SchemaQualifiedName{SchemaName: "public", EscapedName: "\"foo_id_seq\""}, @@ -465,7 +620,7 @@ var ( PRIMARY KEY (author, id) ) FOR VALUES IN ('some author 1'); `}, - expectedHash: "6b678f2eae7b824d", + expectedHash: "bd547bb10b1d6ae", expectedSchema: schema.Schema{ Tables: []schema.Table{ { @@ -514,7 +669,7 @@ var ( "serial" SERIAL NOT NULL ); `}, - expectedHash: "fe72d1b3a50d54b9", + expectedHash: "c107f4517aef1d0d", expectedSchema: schema.Schema{ Tables: []schema.Table{ { @@ -554,97 +709,6 @@ var ( }, }, }, - { - name: "Multi-Table", - ddl: []string{` - CREATE TABLE foo ( - id INTEGER PRIMARY KEY CHECK (id > 0) NO INHERIT, - content TEXT DEFAULT 'some default' - ); - CREATE INDEX foo_idx ON foo(id, content); - CREATE TABLE bar( - id INTEGER PRIMARY KEY CHECK (id > 0), - content TEXT NOT NULL - ); - CREATE INDEX bar_idx ON bar(content, id); - CREATE TABLE foobar( - id INTEGER PRIMARY KEY, - content BIGINT NOT NULL - ); - ALTER TABLE foobar ADD CONSTRAINT foobar_id_check CHECK (id > 0) NOT VALID; - CREATE UNIQUE INDEX foobar_idx ON foobar(content); - `}, - expectedHash: "ef43a41b2ac96e18", - expectedSchema: schema.Schema{ - Tables: []schema.Table{ - { - Name: "foo", - Columns: []schema.Column{ - {Name: "id", Type: "integer", Size: 4}, - {Name: "content", Type: "text", IsNullable: true, Default: "'some default'::text", Size: -1, Collation: defaultCollation}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "foo_id_check", Expression: "(id > 0)", IsValid: true}, - }, - }, - { - Name: "bar", - Columns: []schema.Column{ - {Name: "id", Type: "integer", Size: 4}, - {Name: "content", Type: "text", Size: -1, Collation: defaultCollation}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "bar_id_check", Expression: "(id > 0)", IsValid: true, IsInheritable: true}, - }, - }, - { - Name: "foobar", - Columns: []schema.Column{ - {Name: "id", Type: "integer", Size: 4}, - {Name: "content", Type: "bigint", Size: 8}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "foobar_id_check", Expression: "(id > 0)", IsInheritable: true}, - }, - }, - }, - Indexes: []schema.Index{ - // foo indexes - { - TableName: "foo", - Name: "foo_pkey", Columns: []string{"id"}, IsPk: true, IsUnique: true, ConstraintName: "foo_pkey", - GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foo USING btree (id)", - }, - { - TableName: "foo", - Name: "foo_idx", Columns: []string{"id", "content"}, - GetIndexDefStmt: "CREATE INDEX foo_idx ON public.foo USING btree (id, content)", - }, - // bar indexes - { - TableName: "bar", - Name: "bar_pkey", Columns: []string{"id"}, IsPk: true, IsUnique: true, ConstraintName: "bar_pkey", - GetIndexDefStmt: "CREATE UNIQUE INDEX bar_pkey ON public.bar USING btree (id)", - }, - { - TableName: "bar", - Name: "bar_idx", Columns: []string{"content", "id"}, - GetIndexDefStmt: "CREATE INDEX bar_idx ON public.bar USING btree (content, id)", - }, - // foobar indexes - { - TableName: "foobar", - Name: "foobar_pkey", Columns: []string{"id"}, IsPk: true, IsUnique: true, ConstraintName: "foobar_pkey", - GetIndexDefStmt: "CREATE UNIQUE INDEX foobar_pkey ON public.foobar USING btree (id)", - }, - { - TableName: "foobar", - Name: "foobar_idx", Columns: []string{"content"}, IsUnique: true, - GetIndexDefStmt: "CREATE UNIQUE INDEX foobar_idx ON public.foobar USING btree (content)", - }, - }, - }, - }, { name: "Multi-Schema", ddl: []string{` @@ -721,8 +785,12 @@ var ( FOR EACH ROW WHEN (OLD.* IS DISTINCT FROM NEW.*) EXECUTE PROCEDURE test.increment_version(); + + -- create foreign keys + ALTER TABLE foo ADD CONSTRAINT foo_fk FOREIGN KEY (id) REFERENCES test.foo(test_schema_id); + ALTER TABLE test.foo ADD CONSTRAINT foo_fk FOREIGN KEY (test_schema_id) REFERENCES foo(id); `}, - expectedHash: "72c5e264fb96ed86", + expectedHash: "255f5a621ca70b6b", expectedSchema: schema.Schema{ Tables: []schema.Table{ { @@ -788,6 +856,17 @@ var ( GetIndexDefStmt: "CREATE UNIQUE INDEX bar_1_pkey ON public.bar_1 USING btree (author, id)", }, }, + ForeignKeyConstraints: []schema.ForeignKeyConstraint{ + { + EscapedName: "\"foo_fk\"", + OwningTable: schema.SchemaQualifiedName{SchemaName: "public", EscapedName: "\"foo\""}, + OwningTableUnescapedName: "foo", + ForeignTable: schema.SchemaQualifiedName{SchemaName: "test", EscapedName: "\"foo\""}, + ForeignTableUnescapedName: "foo", + ConstraintDef: "FOREIGN KEY (id) REFERENCES test.foo(test_schema_id)", + IsValid: true, + }, + }, Functions: []schema.Function{ { SchemaQualifiedName: schema.SchemaQualifiedName{EscapedName: "\"dup\"(integer, OUT f1 integer, OUT f2 text)", SchemaName: "public"}, @@ -814,7 +893,7 @@ var ( { name: "Empty Schema", ddl: nil, - expectedHash: "4cc7f4f2dd81ec29", + expectedHash: "aebac19ebacc31e8", expectedSchema: schema.Schema{ Tables: nil, }, @@ -826,7 +905,7 @@ var ( value TEXT ); `}, - expectedHash: "24b2cd8d56d1cedd", + expectedHash: "d79ce7136996f2a7", expectedSchema: schema.Schema{ Tables: []schema.Table{ { diff --git a/pkg/diff/plan.go b/pkg/diff/plan.go index 77025fc..8b29c06 100644 --- a/pkg/diff/plan.go +++ b/pkg/diff/plan.go @@ -9,15 +9,16 @@ import ( type MigrationHazardType = string const ( - MigrationHazardTypeAcquiresAccessExclusiveLock MigrationHazardType = "ACQUIRES_ACCESS_EXCLUSIVE_LOCK" - MigrationHazardTypeAcquiresShareLock MigrationHazardType = "ACQUIRES_SHARE_LOCK" - MigrationHazardTypeDeletesData MigrationHazardType = "DELETES_DATA" - MigrationHazardTypeHasUntrackableDependencies MigrationHazardType = "HAS_UNTRACKABLE_DEPENDENCIES" - MigrationHazardTypeIndexBuild MigrationHazardType = "INDEX_BUILD" - MigrationHazardTypeIndexDropped MigrationHazardType = "INDEX_DROPPED" - MigrationHazardTypeImpactsDatabasePerformance MigrationHazardType = "IMPACTS_DATABASE_PERFORMANCE" - MigrationHazardTypeIsUserGenerated MigrationHazardType = "IS_USER_GENERATED" - MigrationHazardTypeExtensionVersionUpgrade MigrationHazardType = "UPGRADING_EXTENSION_VERSION" + MigrationHazardTypeAcquiresAccessExclusiveLock MigrationHazardType = "ACQUIRES_ACCESS_EXCLUSIVE_LOCK" + MigrationHazardTypeAcquiresShareLock MigrationHazardType = "ACQUIRES_SHARE_LOCK" + MigrationHazardTypeAcquiresShareRowExclusiveLock MigrationHazardType = "ACQUIRES_SHARE_ROW_EXCLUSIVE_LOCK" + MigrationHazardTypeDeletesData MigrationHazardType = "DELETES_DATA" + MigrationHazardTypeHasUntrackableDependencies MigrationHazardType = "HAS_UNTRACKABLE_DEPENDENCIES" + MigrationHazardTypeIndexBuild MigrationHazardType = "INDEX_BUILD" + MigrationHazardTypeIndexDropped MigrationHazardType = "INDEX_DROPPED" + MigrationHazardTypeImpactsDatabasePerformance MigrationHazardType = "IMPACTS_DATABASE_PERFORMANCE" + MigrationHazardTypeIsUserGenerated MigrationHazardType = "IS_USER_GENERATED" + MigrationHazardTypeExtensionVersionUpgrade MigrationHazardType = "UPGRADING_EXTENSION_VERSION" ) // MigrationHazard represents a hazard that a statement poses to a database diff --git a/pkg/diff/schema_migration_plan_test.go b/pkg/diff/schema_migration_plan_test.go index ba474a9..c8b1c85 100644 --- a/pkg/diff/schema_migration_plan_test.go +++ b/pkg/diff/schema_migration_plan_test.go @@ -42,150 +42,6 @@ var ( } schemaMigrationPlanTestCases = []schemaMigrationPlanTestCase{ - { - name: "No-op", - oldSchema: schema.Schema{ - Tables: []schema.Table{ - { - Name: "foobar", - Columns: []schema.Column{ - {Name: "id", Type: "integer"}, - {Name: "foo", Type: "character varying(255)", Default: "''::character varying", Collation: defaultCollation}, - {Name: "bar", Type: "timestamp without time zone", IsNullable: true, Default: "CURRENT_TIMESTAMP"}, - {Name: "fizz", Type: "boolean", Default: "CURRENT_TIMESTAMP"}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "id_check", Expression: "(id > 0)", IsInheritable: true}, - {Name: "bar_check", Expression: "(id > LENGTH(foo))", IsValid: true}, - }, - }, - { - Name: "bar", - Columns: []schema.Column{ - {Name: "id", Type: "character varying(255)", Default: "", IsNullable: false, Collation: defaultCollation}, - {Name: "foo", Type: "integer", Default: "", IsNullable: true}, - {Name: "bar", Type: "double precision", Default: "8.8", IsNullable: false}, - {Name: "fizz", Type: "timestamp with time zone", Default: "CURRENT_TIMESTAMP", IsNullable: true}, - {Name: "buzz", Type: "real", Default: "", IsNullable: false}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "id_check", Expression: "(id + 10 > 0 )", IsValid: true}, - {Name: "bar_check", Expression: "(foo > buzz)", IsInheritable: true}, - }, - }, - { - Name: "fizz", - Columns: nil, - CheckConstraints: nil, - }, - }, - Indexes: []schema.Index{ - // bar indexes - { - TableName: "bar", - Name: "bar_pkey", Columns: []string{"bar"}, IsPk: true, IsUnique: true, ConstraintName: "bar_pkey_non_default_name", - GetIndexDefStmt: "CREATE UNIQUE INDEX bar_pkey ON public.bar USING btree (bar)", - }, - { - TableName: "bar", - Name: "bar_normal_idx", Columns: []string{"fizz"}, - GetIndexDefStmt: "CREATE INDEX bar_normal_idx ON public.bar USING btree (fizz)", - }, - { - TableName: "bar", - Name: "bar_unique_idx", IsUnique: true, Columns: []string{"fizz", "buzz"}, - GetIndexDefStmt: "CREATE UNIQUE INDEX bar_unique_idx ON public.bar USING btree (fizz, buzz)", - }, - // foobar indexes - { - TableName: "foobar", - Name: "foobar_pkey", Columns: []string{"id"}, IsPk: true, IsUnique: true, ConstraintName: "foobar_pkey", - GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foobar USING btree (id)", - }, - { - TableName: "foobar", - Name: "foobar_normal_idx", Columns: []string{"fizz"}, - GetIndexDefStmt: "CREATE INDEX foobar_normal_idx ON public.foobar USING btree (fizz)", - }, - { - TableName: "foobar", - Name: "foobar_unique_idx", IsUnique: true, Columns: []string{"foo", "bar"}, - GetIndexDefStmt: "CREATE UNIQUE INDEX foobar_unique_idx ON public.foobar USING btree (foo, bar)", - }, - }, - }, - newSchema: schema.Schema{ - Tables: []schema.Table{ - { - Name: "foobar", - Columns: []schema.Column{ - {Name: "id", Type: "integer"}, - {Name: "foo", Type: "character varying(255)", Default: "''::character varying", Collation: defaultCollation}, - {Name: "bar", Type: "timestamp without time zone", IsNullable: true, Default: "CURRENT_TIMESTAMP"}, - {Name: "fizz", Type: "boolean", Default: "CURRENT_TIMESTAMP"}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "id_check", Expression: "(id > 0)", IsInheritable: true}, - {Name: "bar_check", Expression: "(id > LENGTH(foo))", IsValid: true}, - }, - }, - { - Name: "bar", - Columns: []schema.Column{ - {Name: "id", Type: "character varying(255)", Default: "", IsNullable: false, Collation: defaultCollation}, - {Name: "foo", Type: "integer", Default: "", IsNullable: true}, - {Name: "bar", Type: "double precision", Default: "8.8", IsNullable: false}, - {Name: "fizz", Type: "timestamp with time zone", Default: "CURRENT_TIMESTAMP", IsNullable: true}, - {Name: "buzz", Type: "real", Default: "", IsNullable: false}, - }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "id_check", Expression: "(id + 10 > 0 )", IsValid: true}, - {Name: "bar_check", Expression: "(foo > buzz)", IsInheritable: true}, - }, - }, - { - Name: "fizz", - Columns: nil, - CheckConstraints: nil, - }, - }, - Indexes: []schema.Index{ - // bar indexes - { - TableName: "bar", - Name: "bar_pkey", Columns: []string{"bar"}, IsPk: true, IsUnique: true, ConstraintName: "bar_pkey_non_default_name", - GetIndexDefStmt: "CREATE UNIQUE INDEX bar_pkey ON public.bar USING btree (bar)", - }, - { - TableName: "bar", - Name: "bar_normal_idx", Columns: []string{"fizz"}, - GetIndexDefStmt: "CREATE INDEX bar_normal_idx ON public.bar USING btree (fizz)", - }, - { - TableName: "bar", - Name: "bar_unique_idx", IsUnique: true, Columns: []string{"fizz", "buzz"}, - GetIndexDefStmt: "CREATE UNIQUE INDEX bar_unique_idx ON public.bar USING btree (fizz, buzz)", - }, - // foobar indexes - { - TableName: "foobar", - Name: "foobar_pkey", Columns: []string{"id"}, IsPk: true, IsUnique: true, ConstraintName: "foobar_pkey", - GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foobar USING btree (id)", - }, - { - TableName: "foobar", - Name: "foobar_normal_idx", Columns: []string{"fizz"}, - GetIndexDefStmt: "CREATE INDEX foobar_normal_idx ON public.foobar USING btree (fizz)", - }, - { - TableName: "foobar", - Name: "foobar_unique_idx", IsUnique: true, Columns: []string{"foo", "bar"}, - GetIndexDefStmt: "CREATE UNIQUE INDEX foobar_unique_idx ON public.foobar USING btree (foo, bar)", - }, - }, - }, - expectedStatements: nil, - }, { name: "Index replacement", oldSchema: schema.Schema{ @@ -322,12 +178,12 @@ var ( Hazards: []MigrationHazard{buildIndexDroppedQueryPerfHazard()}, }, { - DDL: "ALTER TABLE \"foobar\" DROP COLUMN \"bar\"", + DDL: "ALTER TABLE \"public\".\"foobar\" DROP COLUMN \"bar\"", Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{buildColumnDataDeletionHazard()}, }, { - DDL: "ALTER TABLE \"foobar\" DROP COLUMN \"foo\"", + DDL: "ALTER TABLE \"public\".\"foobar\" DROP COLUMN \"foo\"", Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{buildColumnDataDeletionHazard()}, }, @@ -429,7 +285,7 @@ var ( Timeout: statementTimeoutDefault, }, { - DDL: "ALTER TABLE \"foobar\" ATTACH PARTITION \"foobar_1\" FOR VALUES IN ('some_val')", + DDL: "ALTER TABLE \"public\".\"foobar\" ATTACH PARTITION \"foobar_1\" FOR VALUES IN ('some_val')", Timeout: statementTimeoutDefault, }, }, @@ -799,7 +655,7 @@ var ( }, }, { - DDL: "ALTER TABLE \"foobar\" DROP COLUMN \"bar\"", + DDL: "ALTER TABLE \"public\".\"foobar\" DROP COLUMN \"bar\"", Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{ buildColumnDataDeletionHazard(), @@ -973,48 +829,121 @@ var ( }, expectedStatements: []Statement{ { - DDL: "ALTER TABLE \"foobar\" VALIDATE CONSTRAINT \"id_check\"", + DDL: "ALTER TABLE \"public\".\"foobar\" VALIDATE CONSTRAINT \"id_check\"", Timeout: statementTimeoutDefault, Hazards: nil, }, }, }, { - name: "Invalid check constraint re-created if expression changes", + name: "Invalid foreign key constraint constraint made valid", oldSchema: schema.Schema{ + Extensions: []schema.Extension{ + { + SchemaQualifiedName: schema.SchemaQualifiedName{ + EscapedName: schema.EscapeIdentifier("pg_trgm"), + SchemaName: "public", + }, + Version: "1.6", + }, + }, Tables: []schema.Table{ { - Name: "foobar", + Name: "foo", Columns: []schema.Column{ - {Name: "id", Type: "integer"}, + {Name: "id", Type: "integer", Size: 4}, }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "id_check", Expression: "(id > 0)", IsInheritable: true}, + }, + { + Name: "foo_fk", + Columns: []schema.Column{{Name: "id", Type: "integer", Size: 4}}, + }, + }, + ForeignKeyConstraints: []schema.ForeignKeyConstraint{ + { + EscapedName: "\"foo_fk_fk\"", + OwningTable: schema.SchemaQualifiedName{ + SchemaName: "public", + EscapedName: "\"foo_fk\"", }, + OwningTableUnescapedName: "foo_fk", + ForeignTable: schema.SchemaQualifiedName{ + SchemaName: "public", + EscapedName: "\"foo\"", + }, + ForeignTableUnescapedName: "foo", + ConstraintDef: "FOREIGN KEY (id) REFERENCES foo(id) ON UPDATE CASCADE ON DELETE CASCADE NOT VALID", + IsValid: false, + }, + }, + Indexes: []schema.Index{ + { + TableName: "foo", + Name: "foo_pkey", + Columns: []string{"id"}, + IsPk: true, + IsUnique: true, + ConstraintName: "foo_pkey", + GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foo USING btree (id)", }, }, }, newSchema: schema.Schema{ + Extensions: []schema.Extension{ + { + SchemaQualifiedName: schema.SchemaQualifiedName{ + EscapedName: schema.EscapeIdentifier("pg_trgm"), + SchemaName: "public", + }, + Version: "1.6", + }, + }, Tables: []schema.Table{ { - Name: "foobar", + Name: "foo", Columns: []schema.Column{ - {Name: "id", Type: "integer"}, + {Name: "id", Type: "integer", Size: 4}, }, - CheckConstraints: []schema.CheckConstraint{ - {Name: "id_check", Expression: "(id < 0)", IsInheritable: true, IsValid: true}, + }, + { + Name: "foo_fk", + Columns: []schema.Column{{Name: "id", Type: "integer", Size: 4}}, + }, + }, + ForeignKeyConstraints: []schema.ForeignKeyConstraint{ + { + EscapedName: "\"foo_fk_fk\"", + OwningTable: schema.SchemaQualifiedName{ + SchemaName: "public", + EscapedName: "\"foo_fk\"", }, + OwningTableUnescapedName: "foo_fk", + ForeignTable: schema.SchemaQualifiedName{ + SchemaName: "public", + EscapedName: "\"foo\"", + }, + ForeignTableUnescapedName: "foo", + ConstraintDef: "FOREIGN KEY (id) REFERENCES foo(id) ON UPDATE CASCADE ON DELETE CASCADE", + IsValid: true, + }, + }, + Indexes: []schema.Index{ + { + TableName: "foo", + Name: "foo_pkey", + Columns: []string{"id"}, + IsPk: true, + IsUnique: true, + ConstraintName: "foo_pkey", + GetIndexDefStmt: "CREATE UNIQUE INDEX foo_pkey ON public.foo USING btree (id)", }, }, }, expectedStatements: []Statement{ { - DDL: "ALTER TABLE \"foobar\" DROP CONSTRAINT \"id_check\"", - Timeout: statementTimeoutDefault, - }, - { - DDL: "ALTER TABLE \"foobar\" ADD CONSTRAINT \"id_check\" CHECK((id < 0))", + DDL: "ALTER TABLE \"public\".\"foo_fk\" VALIDATE CONSTRAINT \"foo_fk_fk\"", Timeout: statementTimeoutDefault, + Hazards: nil, }, }, }, @@ -1046,7 +975,7 @@ var ( }, expectedStatements: []Statement{ { - DDL: "ALTER TABLE \"foobar\" ALTER COLUMN \"baz\" SET DATA TYPE timestamp without time zone using to_timestamp(\"baz\" / 1000)", + DDL: "ALTER TABLE \"public\".\"foobar\" ALTER COLUMN \"baz\" SET DATA TYPE timestamp without time zone using to_timestamp(\"baz\" / 1000)", Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{{ Type: MigrationHazardTypeAcquiresAccessExclusiveLock, @@ -1064,7 +993,7 @@ var ( Hazards: []MigrationHazard{buildAnalyzeColumnMigrationHazard()}, }, { - DDL: "ALTER TABLE \"foobar\" ALTER COLUMN \"baz\" SET DEFAULT current_timestamp", + DDL: "ALTER TABLE \"public\".\"foobar\" ALTER COLUMN \"baz\" SET DEFAULT current_timestamp", Timeout: statementTimeoutDefault, }, }, @@ -1097,7 +1026,7 @@ var ( }, expectedStatements: []Statement{ { - DDL: "ALTER TABLE \"foobar\" ALTER COLUMN \"migrate_to_c_coll\" SET DATA TYPE text COLLATE \"pg_catalog\".\"C\" using \"migrate_to_c_coll\"::text", + DDL: "ALTER TABLE \"public\".\"foobar\" ALTER COLUMN \"migrate_to_c_coll\" SET DATA TYPE text COLLATE \"pg_catalog\".\"C\" using \"migrate_to_c_coll\"::text", Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{buildColumnTypeChangeHazard()}, }, @@ -1107,7 +1036,7 @@ var ( Hazards: []MigrationHazard{buildAnalyzeColumnMigrationHazard()}, }, { - DDL: "ALTER TABLE \"foobar\" ALTER COLUMN \"migrate_type\" SET DATA TYPE character varying(255) COLLATE \"pg_catalog\".\"default\" using \"migrate_type\"::character varying(255)", + DDL: "ALTER TABLE \"public\".\"foobar\" ALTER COLUMN \"migrate_type\" SET DATA TYPE character varying(255) COLLATE \"pg_catalog\".\"default\" using \"migrate_type\"::character varying(255)", Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{buildColumnTypeChangeHazard()}, }, diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index d7e867d..9c780a9 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -97,6 +97,10 @@ type ( oldAndNew[schema.Index] } + foreignKeyConstraintDiff struct { + oldAndNew[schema.ForeignKeyConstraint] + } + sequenceDiff struct { oldAndNew[schema.Sequence] } @@ -116,12 +120,13 @@ type ( type schemaDiff struct { oldAndNew[schema.Schema] - extensionDiffs listDiff[schema.Extension, extensionDiff] - tableDiffs listDiff[schema.Table, tableDiff] - indexDiffs listDiff[schema.Index, indexDiff] - sequenceDiffs listDiff[schema.Sequence, sequenceDiff] - functionDiffs listDiff[schema.Function, functionDiff] - triggerDiffs listDiff[schema.Trigger, triggerDiff] + extensionDiffs listDiff[schema.Extension, extensionDiff] + tableDiffs listDiff[schema.Table, tableDiff] + indexDiffs listDiff[schema.Index, indexDiff] + foreignKeyConstraintDiffs listDiff[schema.ForeignKeyConstraint, foreignKeyConstraintDiff] + sequenceDiffs listDiff[schema.Sequence, sequenceDiff] + functionDiffs listDiff[schema.Function, functionDiff] + triggerDiffs listDiff[schema.Trigger, triggerDiff] } func (sd schemaDiff) resolveToSQL() ([]Statement, error) { @@ -188,6 +193,41 @@ func buildSchemaDiff(old, new schema.Schema) (schemaDiff, bool, error) { return schemaDiff{}, false, fmt.Errorf("diffing indexes: %w", err) } + foreignKeyConstraintDiffs, err := diffLists(old.ForeignKeyConstraints, new.ForeignKeyConstraints, func(old, new schema.ForeignKeyConstraint, _, _ int) (foreignKeyConstraintDiff, bool, error) { + if _, isOnNewTable := addedTablesByName[new.OwningTableUnescapedName]; isOnNewTable { + // If the owning table is new, then it must be re-created (this occurs if the base table has been + // re-created). In other words, a foreign key constraint must be re-created if the owning table or referenced + // table is re-created + return foreignKeyConstraintDiff{}, true, nil + } else if _, isReferencingNewTable := addedTablesByName[new.ForeignTableUnescapedName]; isReferencingNewTable { + // Same as above, but for the referenced table + return foreignKeyConstraintDiff{}, true, nil + } + + // Set the new clone to be equal to the old for all fields that can actually be altered + newClone := new + if !old.IsValid && new.IsValid { + // We only support alter from NOT VALID to VALID and no other alterations. + // Instead of checking that each individual property is equal (that's a lot of parsing), we will just + // assert that the constraint definitions are equal if we append "NOT VALID" to the new constraint def + newClone.IsValid = old.IsValid + newClone.ConstraintDef = fmt.Sprintf("%s NOT VALID", newClone.ConstraintDef) + } + if !cmp.Equal(old, newClone) { + return foreignKeyConstraintDiff{}, true, nil + } + + return foreignKeyConstraintDiff{ + oldAndNew[schema.ForeignKeyConstraint]{ + old: old, + new: new, + }, + }, false, nil + }) + if err != nil { + return schemaDiff{}, false, fmt.Errorf("diffing foreign key constraints: %w", err) + } + sequencesDiffs, err := diffLists(old.Sequences, new.Sequences, func(old, new schema.Sequence, oldIndex, newIndex int) (diff sequenceDiff, requiresRecreation bool, error error) { seqDiff := sequenceDiff{ oldAndNew[schema.Sequence]{ @@ -243,12 +283,13 @@ func buildSchemaDiff(old, new schema.Schema) (schemaDiff, bool, error) { old: old, new: new, }, - extensionDiffs: extensionDiffs, - tableDiffs: tableDiffs, - indexDiffs: indexesDiff, - sequenceDiffs: sequencesDiffs, - functionDiffs: functionDiffs, - triggerDiffs: triggerDiffs, + extensionDiffs: extensionDiffs, + tableDiffs: tableDiffs, + indexDiffs: indexesDiff, + foreignKeyConstraintDiffs: foreignKeyConstraintDiffs, + sequenceDiffs: sequencesDiffs, + functionDiffs: functionDiffs, + triggerDiffs: triggerDiffs, }, false, nil } @@ -369,12 +410,21 @@ type schemaSQLGenerator struct{} func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { tablesInNewSchemaByName := buildSchemaObjByNameMap(diff.new.Tables) deletedTablesByName := buildSchemaObjByNameMap(diff.tableDiffs.deletes) + addedTablesByName := buildSchemaObjByNameMap(diff.tableDiffs.adds) - tableSQLVertexGenerator := tableSQLVertexGenerator{ + indexesOldSchemaByTableName := make(map[string][]schema.Index) + for _, idx := range diff.old.Indexes { + indexesOldSchemaByTableName[idx.TableName] = append(indexesOldSchemaByTableName[idx.TableName], idx) + } + indexesInNewSchemaByTableName := make(map[string][]schema.Index) + for _, idx := range diff.new.Indexes { + indexesInNewSchemaByTableName[idx.TableName] = append(indexesInNewSchemaByTableName[idx.TableName], idx) + } + + tableGraphs, err := diff.tableDiffs.resolveToSQLGraph(&tableSQLVertexGenerator{ deletedTablesByName: deletedTablesByName, tablesInNewSchemaByName: tablesInNewSchemaByName, - } - tableGraphs, err := diff.tableDiffs.resolveToSQLGraph(&tableSQLVertexGenerator) + }) if err != nil { return nil, fmt.Errorf("resolving table sql graphs: %w", err) } @@ -384,14 +434,9 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { return nil, fmt.Errorf("resolving extension sql graphs: %w", err) } - indexesInNewSchemaByTableName := make(map[string][]schema.Index) - for _, idx := range diff.new.Indexes { - indexesInNewSchemaByTableName[idx.TableName] = append(indexesInNewSchemaByTableName[idx.TableName], idx) - } - attachPartitionSQLVertexGenerator := attachPartitionSQLVertexGenerator{ + attachPartitionGraphs, err := diff.tableDiffs.resolveToSQLGraph(&attachPartitionSQLVertexGenerator{ indexesInNewSchemaByTableName: indexesInNewSchemaByTableName, - } - attachPartitionGraphs, err := diff.tableDiffs.resolveToSQLGraph(&attachPartitionSQLVertexGenerator) + }) if err != nil { return nil, fmt.Errorf("resolving attach partition sql graphs: %w", err) } @@ -402,18 +447,27 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { return nil, fmt.Errorf("resolving renaming conflicting indexes: %w", err) } - indexSQLVertexGenerator := indexSQLVertexGenerator{ + indexGraphs, err := diff.indexDiffs.resolveToSQLGraph(&indexSQLVertexGenerator{ deletedTablesByName: deletedTablesByName, - addedTablesByName: buildSchemaObjByNameMap(diff.tableDiffs.adds), + addedTablesByName: addedTablesByName, tablesInNewSchemaByName: tablesInNewSchemaByName, indexesInNewSchemaByName: buildSchemaObjByNameMap(diff.new.Indexes), indexRenamesByOldName: renameConflictingIndexSQLVertexGenerator.getRenames(), - } - indexGraphs, err := diff.indexDiffs.resolveToSQLGraph(&indexSQLVertexGenerator) + }) if err != nil { return nil, fmt.Errorf("resolving index sql graphs: %w", err) } + fkConsGraphs, err := diff.foreignKeyConstraintDiffs.resolveToSQLGraph(&foreignKeyConstraintSQLVertexGenerator{ + deletedTablesByName: deletedTablesByName, + addedTablesByName: addedTablesByName, + indexInOldSchemaByTableName: indexesInNewSchemaByTableName, + indexesInNewSchemaByTableName: indexesInNewSchemaByTableName, + }) + if err != nil { + return nil, fmt.Errorf("resolving foreign key constraint sql graphs: %w", err) + } + sequenceGraphs, err := diff.sequenceDiffs.resolveToSQLGraph(&sequenceSQLVertexGenerator{ deletedTablesByName: deletedTablesByName, tableDiffsByName: buildDiffByNameMap[schema.Table, tableDiff](diff.tableDiffs.alters), @@ -427,7 +481,6 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { } functionsInNewSchemaByName := buildSchemaObjByNameMap(diff.new.Functions) - functionGraphs, err := diff.functionDiffs.resolveToSQLGraph(&functionSQLVertexGenerator{ functionsInNewSchemaByName: functionsInNewSchemaByName, }) @@ -448,15 +501,18 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { if err := tableGraphs.union(indexGraphs); err != nil { return nil, fmt.Errorf("unioning table and index graphs: %w", err) } + if err := tableGraphs.union(renameConflictingIndexGraphs); err != nil { + return nil, fmt.Errorf("unioning table and rename conflicting index graphs: %w", err) + } + if err := tableGraphs.union(fkConsGraphs); err != nil { + return nil, fmt.Errorf("unioning table and foreign key constraint graphs: %w", err) + } if err := tableGraphs.union(sequenceGraphs); err != nil { return nil, fmt.Errorf("unioning table and sequence graphs: %w", err) } if err := tableGraphs.union(sequenceOwnershipGraphs); err != nil { return nil, fmt.Errorf("unioning table and sequence ownership graphs: %w", err) } - if err := tableGraphs.union(renameConflictingIndexGraphs); err != nil { - return nil, fmt.Errorf("unioning table and rename conflicting index graphs: %w", err) - } if err := tableGraphs.union(functionGraphs); err != nil { return nil, fmt.Errorf("unioning table and function graphs: %w", err) } @@ -624,7 +680,7 @@ func (t *tableSQLVertexGenerator) alterPartition(diff tableDiff) ([]Statement, e if colDiff.old.IsNullable == colDiff.new.IsNullable { continue } - alterColumnPrefix := fmt.Sprintf("%s ALTER COLUMN %s", alterTablePrefix(diff.new.Name), schema.EscapeIdentifier(colDiff.new.Name)) + alterColumnPrefix := fmt.Sprintf("%s ALTER COLUMN %s", publicTableAlterPrefix(diff.new.Name), schema.EscapeIdentifier(colDiff.new.Name)) if colDiff.new.IsNullable { stmts = append(stmts, Statement{ DDL: fmt.Sprintf("%s DROP NOT NULL", alterColumnPrefix), @@ -681,14 +737,14 @@ type columnSQLGenerator struct { func (csg *columnSQLGenerator) Add(column schema.Column) ([]Statement, error) { return []Statement{{ - DDL: fmt.Sprintf("%s ADD COLUMN %s", alterTablePrefix(csg.tableName), buildColumnDefinition(column)), + DDL: fmt.Sprintf("%s ADD COLUMN %s", publicTableAlterPrefix(csg.tableName), buildColumnDefinition(column)), Timeout: statementTimeoutDefault, }}, nil } func (csg *columnSQLGenerator) Delete(column schema.Column) ([]Statement, error) { return []Statement{{ - DDL: fmt.Sprintf("%s DROP COLUMN %s", alterTablePrefix(csg.tableName), schema.EscapeIdentifier(column.Name)), + DDL: fmt.Sprintf("%s DROP COLUMN %s", publicTableAlterPrefix(csg.tableName), schema.EscapeIdentifier(column.Name)), Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{ { @@ -705,7 +761,7 @@ func (csg *columnSQLGenerator) Alter(diff columnDiff) ([]Statement, error) { } oldColumn, newColumn := diff.old, diff.new var stmts []Statement - alterColumnPrefix := fmt.Sprintf("%s ALTER COLUMN %s", alterTablePrefix(csg.tableName), schema.EscapeIdentifier(newColumn.Name)) + alterColumnPrefix := fmt.Sprintf("%s ALTER COLUMN %s", publicTableAlterPrefix(csg.tableName), schema.EscapeIdentifier(newColumn.Name)) if oldColumn.IsNullable != newColumn.IsNullable { if newColumn.IsNullable { @@ -966,9 +1022,12 @@ func (isg *indexSQLVertexGenerator) addIdxStmtsWithHazards(index schema.Index) ( // with a similar strategy to adding indexes to a partitioned table return []Statement{ { - DDL: fmt.Sprintf("%s ADD CONSTRAINT %s PRIMARY KEY (%s)", - alterTablePrefix(index.TableName), - schema.EscapeIdentifier(index.Name), + DDL: fmt.Sprintf("%s PRIMARY KEY (%s)", + addConstraintPrefix(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(index.TableName), + }, schema.EscapeIdentifier(index.ConstraintName)), strings.Join(formattedNamesForSQL(index.Columns), ", "), ), Timeout: statementTimeoutDefault, @@ -1052,7 +1111,12 @@ func (isg *indexSQLVertexGenerator) Delete(index schema.Index) ([]Statement, err // the constraint without dropping the index return []Statement{ { - DDL: dropConstraintDDL(index.TableName, constraintName), + DDL: dropConstraintDDL(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(index.TableName), + }, schema.EscapeIdentifier(constraintName)), + Timeout: statementTimeoutDefault, Hazards: []MigrationHazard{ migrationHazardIndexDroppedAcquiresLock, @@ -1133,7 +1197,13 @@ func isOnPartitionedTable(tablesInNewSchemaByName map[string]schema.Table, index func (isg *indexSQLVertexGenerator) addPkConstraintUsingIdx(index schema.Index) Statement { return Statement{ - DDL: fmt.Sprintf("%s ADD CONSTRAINT %s PRIMARY KEY USING INDEX %s", alterTablePrefix(index.TableName), schema.EscapeIdentifier(index.ConstraintName), schema.EscapeIdentifier(index.Name)), + DDL: fmt.Sprintf("%s PRIMARY KEY USING INDEX %s", + addConstraintPrefix(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(index.TableName), + }, schema.EscapeIdentifier(index.ConstraintName)), + schema.EscapeIdentifier(index.Name)), Timeout: statementTimeoutDefault, } } @@ -1236,11 +1306,17 @@ func (csg *checkConstraintSQLGenerator) Add(con schema.CheckConstraint) ([]State } sb := strings.Builder{} - sb.WriteString(fmt.Sprintf("%s ADD CONSTRAINT %s CHECK(%s)", alterTablePrefix(csg.tableName), schema.EscapeIdentifier(con.Name), con.Expression)) + sb.WriteString(fmt.Sprintf("%s CHECK(%s)", + addConstraintPrefix(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(csg.tableName), + }, schema.EscapeIdentifier(con.Name)), con.Expression)) if !con.IsInheritable { sb.WriteString(" NO INHERIT") } + // TODO: We should have a hazard here when adding a check constraint that is valid if !con.IsValid { sb.WriteString(" NOT VALID") } @@ -1259,7 +1335,11 @@ func (csg *checkConstraintSQLGenerator) Delete(con schema.CheckConstraint) ([]St } return []Statement{{ - DDL: dropConstraintDDL(csg.tableName, con.Name), + DDL: dropConstraintDDL(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(csg.tableName), + }, schema.EscapeIdentifier(con.Name)), Timeout: statementTimeoutDefault, }}, nil } @@ -1273,7 +1353,7 @@ func (csg *checkConstraintSQLGenerator) Alter(diff checkConstraintDiff) ([]State oldCopy.IsValid = diff.new.IsValid if !cmp.Equal(oldCopy, diff.new) { // Technically, we could support altering expression, but I don't see the use case for it. it would require more test - // cases than forceReadding it, and I'm not convinced it unlocks any functionality + // cases than force readding it, and I'm not convinced it unlocks any functionality return nil, fmt.Errorf("altering check constraint to resolve the following diff %s: %w", cmp.Diff(oldCopy, diff.new), ErrNotImplemented) } else if diff.old.IsValid && !diff.new.IsValid { return nil, fmt.Errorf("check constraint can't go from invalid to valid") @@ -1281,10 +1361,11 @@ func (csg *checkConstraintSQLGenerator) Alter(diff checkConstraintDiff) ([]State return nil, fmt.Errorf("check constraints that depend on UDFs: %w", ErrNotImplemented) } - return []Statement{{ - DDL: fmt.Sprintf("%s VALIDATE CONSTRAINT %s", alterTablePrefix(csg.tableName), schema.EscapeIdentifier(diff.old.Name)), - Timeout: statementTimeoutDefault, - }}, nil + return []Statement{validateConstraintStatement(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(csg.tableName), + }, schema.EscapeIdentifier(diff.new.Name))}, nil } type attachPartitionSQLVertexGenerator struct { @@ -1304,7 +1385,7 @@ func (*attachPartitionSQLVertexGenerator) Alter(_ tableDiff) ([]Statement, error func buildAttachPartitionStatement(table schema.Table) Statement { return Statement{ - DDL: fmt.Sprintf("%s ATTACH PARTITION %s %s", alterTablePrefix(table.ParentTableName), schema.EscapeIdentifier(table.Name), table.ForValues), + DDL: fmt.Sprintf("%s ATTACH PARTITION %s %s", publicTableAlterPrefix(table.ParentTableName), schema.EscapeIdentifier(table.Name), table.ForValues), Timeout: statementTimeoutDefault, } } @@ -1332,6 +1413,102 @@ func (a *attachPartitionSQLVertexGenerator) GetDeleteDependencies(_ schema.Table return nil } +type foreignKeyConstraintSQLVertexGenerator struct { + // deletedTablesByName is a map of table name to tables (and partitions) that are deleted + // This is used to identify if the owning table is being dropped (meaning + // the foreign key can be implicitly dropped) + deletedTablesByName map[string]schema.Table + // addedTablesByName is a map of table name to the added tables + // This is used to identify if hazards are necessary + addedTablesByName map[string]schema.Table + // indexInOldSchemaByTableName is a map of index name to the index in the old schema + // This is used to force the foreign key constraint to be dropped before the index it depends on is dropped + indexInOldSchemaByTableName map[string][]schema.Index + // indexesInNewSchemaByTableName is a map of index name to the index + // Same as above but for adds and after + indexesInNewSchemaByTableName map[string][]schema.Index +} + +func (f *foreignKeyConstraintSQLVertexGenerator) Add(con schema.ForeignKeyConstraint) ([]Statement, error) { + var hazards []MigrationHazard + _, isOnNewTable := f.addedTablesByName[con.OwningTableUnescapedName] + _, isReferencedTableNew := f.addedTablesByName[con.ForeignTableUnescapedName] + if con.IsValid && (!isOnNewTable || !isReferencedTableNew) { + hazards = append(hazards, MigrationHazard{ + Type: MigrationHazardTypeAcquiresShareRowExclusiveLock, + Message: "This will lock writes to the owning table and referenced table while the constraint is being added. " + + "Instead, consider adding the constraint as NOT VALID and validating it later.", + }) + } + return []Statement{{ + DDL: fmt.Sprintf("%s %s", addConstraintPrefix(con.OwningTable, con.EscapedName), con.ConstraintDef), + Timeout: statementTimeoutDefault, + Hazards: hazards, + }}, nil +} + +func (f *foreignKeyConstraintSQLVertexGenerator) Delete(con schema.ForeignKeyConstraint) ([]Statement, error) { + // Always generate a drop statement even if the owning table is being deleted. This simplifies the logic a bit because + // if the owning table has a circular FK dependency with another table being dropped, we will need to explicitly drop + // one of the FK's first + return []Statement{{ + DDL: dropConstraintDDL(con.OwningTable, con.EscapedName), + Timeout: statementTimeoutDefault, + }}, nil +} + +func (f *foreignKeyConstraintSQLVertexGenerator) Alter(diff foreignKeyConstraintDiff) ([]Statement, error) { + var stmts []Statement + if !diff.old.IsValid && diff.new.IsValid { + diff.old.IsValid = diff.new.IsValid + // We're not keeping track of the other FK attributes, so it's easiest to ensure all other properties are equal + // by just modifying the old constraint diff to exclude "NOT VALID", which should make the diffs equal if no + // other properties have changed + if !strings.HasSuffix(diff.old.ConstraintDef, " NOT VALID") { + return nil, fmt.Errorf("expected the old constraint def to be suffixed with NOT VALID: %q", diff.old.ConstraintDef) + } + diff.old.ConstraintDef = strings.TrimSuffix(diff.old.ConstraintDef, " NOT VALID") + stmts = append(stmts, validateConstraintStatement(diff.new.OwningTable, diff.new.EscapedName)) + } + if !cmp.Equal(diff.old, diff.new) { + return nil, fmt.Errorf("altering foreign key constraint to resolve the following diff %s: %w", cmp.Diff(diff.old, diff.new), ErrNotImplemented) + } + + return stmts, nil +} + +func (*foreignKeyConstraintSQLVertexGenerator) GetSQLVertexId(con schema.ForeignKeyConstraint) string { + return buildVertexId("fkconstraint", con.GetName()) +} + +func (f *foreignKeyConstraintSQLVertexGenerator) GetAddAlterDependencies(con, _ schema.ForeignKeyConstraint) []dependency { + deps := []dependency{ + mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(f.GetSQLVertexId(con), diffTypeDelete), + mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildTableVertexId(con.OwningTableUnescapedName), diffTypeAddAlter), + mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildTableVertexId(con.ForeignTableUnescapedName), diffTypeAddAlter), + } + // This is the slightly lazy way of ensuring the foreign key constraint is added after the requisite index is build. + // We __could__ due this just for the index fk depends on, but that's slightly more wiring than we need right now + for _, i := range f.indexesInNewSchemaByTableName[con.ForeignTableUnescapedName] { + deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildIndexVertexId(i.Name), diffTypeAddAlter)) + } + + return deps +} + +func (f *foreignKeyConstraintSQLVertexGenerator) GetDeleteDependencies(con schema.ForeignKeyConstraint) []dependency { + deps := []dependency{ + mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildTableVertexId(con.OwningTableUnescapedName), diffTypeDelete), + mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildTableVertexId(con.ForeignTableUnescapedName), diffTypeDelete), + } + // This is the slightly lazy way of ensuring the foreign key constraint is dropped before the requisite index is dropped. + // We __could__ due this just for the index the fk depends on, but that's slightly more wiring than we need right now + for _, i := range f.indexInOldSchemaByTableName[con.ForeignTableUnescapedName] { + deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildIndexVertexId(i.Name), diffTypeDelete)) + } + return deps +} + type sequenceSQLVertexGenerator struct { deletedTablesByName map[string]schema.Table tableDiffsByName map[string]tableDiff @@ -1733,12 +1910,31 @@ func stripMigrationHazards(stmts []Statement) []Statement { return noHazardsStmts } -func dropConstraintDDL(tableName, constraintName string) string { - return fmt.Sprintf("%s DROP CONSTRAINT %s", alterTablePrefix(tableName), schema.EscapeIdentifier(constraintName)) +func addConstraintPrefix(table schema.SchemaQualifiedName, escapedConstraintName string) string { + return fmt.Sprintf("%s ADD CONSTRAINT %s", alterTablePrefix(table), escapedConstraintName) +} + +func dropConstraintDDL(table schema.SchemaQualifiedName, escapedConstraintName string) string { + return fmt.Sprintf("%s DROP CONSTRAINT %s", alterTablePrefix(table), escapedConstraintName) +} + +func validateConstraintStatement(owningTable schema.SchemaQualifiedName, escapedConstraintName string) Statement { + return Statement{ + DDL: fmt.Sprintf("%s VALIDATE CONSTRAINT %s", alterTablePrefix(owningTable), escapedConstraintName), + Timeout: statementTimeoutDefault, + } +} + +func publicTableAlterPrefix(tableName string) string { + return alterTablePrefix(schema.SchemaQualifiedName{ + // Assumes public + SchemaName: "public", + EscapedName: schema.EscapeIdentifier(tableName), + }) } -func alterTablePrefix(tableName string) string { - return fmt.Sprintf("ALTER TABLE %s", schema.EscapeIdentifier(tableName)) +func alterTablePrefix(table schema.SchemaQualifiedName) string { + return fmt.Sprintf("ALTER TABLE %s", table.GetFQEscapedName()) } func buildColumnDefinition(column schema.Column) string { From 53f2b5dc67d2323ddf9f66a8df89c768476179dc Mon Sep 17 00:00:00 2001 From: bplunkett-stripe Date: Mon, 7 Aug 2023 19:47:27 -0400 Subject: [PATCH 2/5] Add locking hazard for new check constraints --- .../check_constraint_cases_test.go | 31 +++++++++++++++++-- pkg/diff/sql_generator.go | 17 +++++++--- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/internal/migration_acceptance_tests/check_constraint_cases_test.go b/internal/migration_acceptance_tests/check_constraint_cases_test.go index 26372ae..6c6b396 100644 --- a/internal/migration_acceptance_tests/check_constraint_cases_test.go +++ b/internal/migration_acceptance_tests/check_constraint_cases_test.go @@ -52,6 +52,9 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Add check constraint with UDF dependency should error", @@ -106,6 +109,9 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Add multiple check constraints", @@ -128,6 +134,9 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Add check constraints to new column", @@ -148,6 +157,9 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Add check constraint with quoted identifiers", @@ -170,6 +182,9 @@ var checkConstraintCases = []acceptanceTestCase{ ALTER TABLE foobar ADD CONSTRAINT "BAR_CHECK" CHECK ( "Bar" < "ID" ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Add no inherit check constraint", @@ -192,6 +207,9 @@ var checkConstraintCases = []acceptanceTestCase{ ALTER TABLE foobar ADD CONSTRAINT bar_check CHECK ( bar > id ) NO INHERIT; `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Add No-Inherit, Not-Valid check constraint", @@ -381,7 +399,7 @@ var checkConstraintCases = []acceptanceTestCase{ }, }, { - name: "Alter a No-Inherit check constraint to be Inheritable", + name: "Alter a no-Inherit check constraint to be Inheritable", oldSchemaDDL: []string{ ` CREATE TABLE foobar( @@ -402,9 +420,12 @@ var checkConstraintCases = []acceptanceTestCase{ ALTER TABLE foobar ADD CONSTRAINT bar_check CHECK ( bar > id ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { - name: "Alter an Inheritable check constraint to be No-Inherit", + name: "Alter an Inheritable check constraint to be no-inherit", oldSchemaDDL: []string{ ` CREATE TABLE foobar( @@ -425,6 +446,9 @@ var checkConstraintCases = []acceptanceTestCase{ ALTER TABLE foobar ADD CONSTRAINT bar_check CHECK ( bar > id ) NO INHERIT; `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Alter a check constraint expression", @@ -446,6 +470,9 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + }, }, { name: "Alter check constraint with UDF dependency should error", diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index 9c780a9..ed30c85 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -594,7 +594,7 @@ func (t *tableSQLVertexGenerator) Add(table schema.Table) ([]Statement, error) { Timeout: statementTimeoutDefault, }) - csg := checkConstraintSQLGenerator{tableName: table.Name} + csg := checkConstraintSQLGenerator{tableName: table.Name, isNewTable: true} for _, checkCon := range table.CheckConstraints { addConStmts, err := csg.Add(checkCon) if err != nil { @@ -649,7 +649,7 @@ func (t *tableSQLVertexGenerator) Alter(diff tableDiff) ([]Statement, error) { return nil, fmt.Errorf("resolving index diff: %w", err) } - checkConSQLGenerator := checkConstraintSQLGenerator{tableName: diff.new.Name} + checkConSQLGenerator := checkConstraintSQLGenerator{tableName: diff.new.Name, isNewTable: false} checkConGeneratedSQL, err := diff.checkConstraintDiff.resolveToSQLGroupedByEffect(&checkConSQLGenerator) if err != nil { return nil, fmt.Errorf("resolving check constraints diff: %w", err) @@ -1295,10 +1295,13 @@ func (isg *indexSQLVertexGenerator) addDepsOnTableAddAlterIfNecessary(index sche } type checkConstraintSQLGenerator struct { - tableName string + tableName string + isNewTable bool } func (csg *checkConstraintSQLGenerator) Add(con schema.CheckConstraint) ([]Statement, error) { + var hazards []MigrationHazard + // UDF's in check constraints are a bad idea. Check constraints are not re-validated // if the UDF changes, so it's not really a safe practice. We won't support it for now if len(con.DependsOnFunctions) > 0 { @@ -1316,14 +1319,20 @@ func (csg *checkConstraintSQLGenerator) Add(con schema.CheckConstraint) ([]State sb.WriteString(" NO INHERIT") } - // TODO: We should have a hazard here when adding a check constraint that is valid if !con.IsValid { sb.WriteString(" NOT VALID") + } else if !csg.isNewTable { + hazards = append(hazards, MigrationHazard{ + Type: MigrationHazardTypeAcquiresAccessExclusiveLock, + Message: "This will lock reads and writes to the owning table while the constraint is being added. " + + "Instead, consider adding the constraint as NOT VALID and validating it later.", + }) } return []Statement{{ DDL: sb.String(), Timeout: statementTimeoutDefault, + Hazards: hazards, }}, nil } From 37e5f1bff2fc45191dfdf2a7445308353fdcbab8 Mon Sep 17 00:00:00 2001 From: bplunkett-stripe Date: Mon, 7 Aug 2023 19:54:44 -0400 Subject: [PATCH 3/5] Small changes --- pkg/diff/sql_generator.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index ed30c85..c00b140 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -1362,7 +1362,7 @@ func (csg *checkConstraintSQLGenerator) Alter(diff checkConstraintDiff) ([]State oldCopy.IsValid = diff.new.IsValid if !cmp.Equal(oldCopy, diff.new) { // Technically, we could support altering expression, but I don't see the use case for it. it would require more test - // cases than force readding it, and I'm not convinced it unlocks any functionality + // cases than force re-adding it, and I'm not convinced it unlocks any functionality return nil, fmt.Errorf("altering check constraint to resolve the following diff %s: %w", cmp.Diff(oldCopy, diff.new), ErrNotImplemented) } else if diff.old.IsValid && !diff.new.IsValid { return nil, fmt.Errorf("check constraint can't go from invalid to valid") @@ -1496,8 +1496,10 @@ func (f *foreignKeyConstraintSQLVertexGenerator) GetAddAlterDependencies(con, _ mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildTableVertexId(con.OwningTableUnescapedName), diffTypeAddAlter), mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildTableVertexId(con.ForeignTableUnescapedName), diffTypeAddAlter), } - // This is the slightly lazy way of ensuring the foreign key constraint is added after the requisite index is build. - // We __could__ due this just for the index fk depends on, but that's slightly more wiring than we need right now + // This is the slightly lazy way of ensuring the foreign key constraint is added after the requisite index is + // built and marked as valid. + // We __could__ do this just for the index the fk depends on, but that's slightly more wiring than we need right now + // because of partitioned indexes, which are only valid when all child indexes have been built for _, i := range f.indexesInNewSchemaByTableName[con.ForeignTableUnescapedName] { deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildIndexVertexId(i.Name), diffTypeAddAlter)) } @@ -1510,8 +1512,10 @@ func (f *foreignKeyConstraintSQLVertexGenerator) GetDeleteDependencies(con schem mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildTableVertexId(con.OwningTableUnescapedName), diffTypeDelete), mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildTableVertexId(con.ForeignTableUnescapedName), diffTypeDelete), } - // This is the slightly lazy way of ensuring the foreign key constraint is dropped before the requisite index is dropped. - // We __could__ due this just for the index the fk depends on, but that's slightly more wiring than we need right now + // This is the slightly lazy way of ensuring the foreign key constraint is added after the requisite index is + // built and marked as valid. + // We __could__ do this just for the index the fk depends on, but that's slightly more wiring than we need right now + // because of partitioned indexes, which are only valid when all child indexes have been built for _, i := range f.indexInOldSchemaByTableName[con.ForeignTableUnescapedName] { deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildIndexVertexId(i.Name), diffTypeDelete)) } @@ -1569,7 +1573,7 @@ func (s *sequenceSQLVertexGenerator) Alter(diff sequenceDiff) ([]Statement, erro diff.old.Owner = diff.new.Owner if !cmp.Equal(diff.old, diff.new) { // Technically, we could support altering expression, but I don't see the use case for it. it would require more test - // cases than forceReadding it, and I'm not convinced it unlocks any functionality + // cases than force re-adding it, and I'm not convinced it unlocks any functionality return nil, fmt.Errorf("altering sequence to resolve the following diff %s: %w", cmp.Diff(diff.old, diff.new), ErrNotImplemented) } From 1475640d4744c261f1747a3bf627bfdcba5e6f19 Mon Sep 17 00:00:00 2001 From: bplunkett-stripe Date: Mon, 7 Aug 2023 19:55:53 -0400 Subject: [PATCH 4/5] SQL generation formatting --- internal/queries/queries.sql.go | 429 +++++++++++++++++--------------- pkg/schema/schema_test.go | 2 +- 2 files changed, 228 insertions(+), 203 deletions(-) diff --git a/internal/queries/queries.sql.go b/internal/queries/queries.sql.go index b859d7d..4a24016 100644 --- a/internal/queries/queries.sql.go +++ b/internal/queries/queries.sql.go @@ -10,20 +10,22 @@ import ( ) const getCheckConstraints = `-- name: GetCheckConstraints :many -SELECT pg_constraint.oid, - pg_constraint.conname::TEXT AS constraint_name, - pg_class.relname::TEXT AS table_name, - pg_constraint.convalidated AS is_valid, - pg_constraint.connoinherit AS is_not_inheritable, - pg_catalog.pg_get_expr( - pg_constraint.conbin, pg_constraint.conrelid - ) AS constraint_expression +SELECT + pg_constraint.oid, + pg_constraint.conname::TEXT AS constraint_name, + pg_class.relname::TEXT AS table_name, + pg_constraint.convalidated AS is_valid, + pg_constraint.connoinherit AS is_not_inheritable, + pg_catalog.pg_get_expr( + pg_constraint.conbin, pg_constraint.conrelid + ) AS constraint_expression FROM pg_catalog.pg_constraint - INNER JOIN pg_catalog.pg_class ON pg_constraint.conrelid = pg_class.oid -WHERE pg_class.relnamespace +INNER JOIN pg_catalog.pg_class ON pg_constraint.conrelid = pg_class.oid +WHERE + pg_class.relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'public') - AND pg_constraint.contype = 'c' - AND pg_constraint.conislocal + AND pg_constraint.contype = 'c' + AND pg_constraint.conislocal ` type GetCheckConstraintsRow struct { @@ -68,8 +70,9 @@ func (q *Queries) GetCheckConstraints(ctx context.Context) ([]GetCheckConstraint const getColumnsForIndex = `-- name: GetColumnsForIndex :many SELECT a.attname::TEXT AS column_name FROM pg_catalog.pg_attribute AS a -WHERE a.attrelid = $1 - AND a.attnum > 0 +WHERE + a.attrelid = $1 + AND a.attnum > 0 ORDER BY a.attnum ` @@ -97,26 +100,28 @@ func (q *Queries) GetColumnsForIndex(ctx context.Context, attrelid interface{}) } const getColumnsForTable = `-- name: GetColumnsForTable :many -SELECT a.attname::TEXT AS column_name, - COALESCE(coll.collname, '')::TEXT AS collation_name, - COALESCE(collation_namespace.nspname, '')::TEXT AS collation_schema_name, - COALESCE( - pg_catalog.pg_get_expr(d.adbin, d.adrelid), '' - )::TEXT AS default_value, - a.attnotnull AS is_not_null, - a.attlen AS column_size, - pg_catalog.format_type(a.atttypid, a.atttypmod) AS column_type +SELECT + a.attname::TEXT AS column_name, + COALESCE(coll.collname, '')::TEXT AS collation_name, + COALESCE(collation_namespace.nspname, '')::TEXT AS collation_schema_name, + COALESCE( + pg_catalog.pg_get_expr(d.adbin, d.adrelid), '' + )::TEXT AS default_value, + a.attnotnull AS is_not_null, + a.attlen AS column_size, + pg_catalog.format_type(a.atttypid, a.atttypmod) AS column_type FROM pg_catalog.pg_attribute AS a - LEFT JOIN - pg_catalog.pg_attrdef AS d - ON (d.adrelid = a.attrelid AND d.adnum = a.attnum) - LEFT JOIN pg_catalog.pg_collation AS coll ON coll.oid = a.attcollation - LEFT JOIN - pg_catalog.pg_namespace AS collation_namespace - ON collation_namespace.oid = coll.collnamespace -WHERE a.attrelid = $1 - AND a.attnum > 0 - AND NOT a.attisdropped +LEFT JOIN + pg_catalog.pg_attrdef AS d + ON (d.adrelid = a.attrelid AND d.adnum = a.attnum) +LEFT JOIN pg_catalog.pg_collation AS coll ON coll.oid = a.attcollation +LEFT JOIN + pg_catalog.pg_namespace AS collation_namespace + ON collation_namespace.oid = coll.collnamespace +WHERE + a.attrelid = $1 + AND a.attnum > 0 + AND NOT a.attisdropped ORDER BY a.attnum ` @@ -162,22 +167,24 @@ func (q *Queries) GetColumnsForTable(ctx context.Context, attrelid interface{}) } const getDependsOnFunctions = `-- name: GetDependsOnFunctions :many -SELECT pg_proc.proname::TEXT AS func_name, - proc_namespace.nspname::TEXT AS func_schema_name, - pg_catalog.pg_get_function_identity_arguments( - pg_proc.oid - ) AS func_identity_arguments +SELECT + pg_proc.proname::TEXT AS func_name, + proc_namespace.nspname::TEXT AS func_schema_name, + pg_catalog.pg_get_function_identity_arguments( + pg_proc.oid + ) AS func_identity_arguments FROM pg_catalog.pg_depend AS depend - INNER JOIN pg_catalog.pg_proc AS pg_proc - ON - depend.refclassid = 'pg_proc'::REGCLASS - AND depend.refobjid = pg_proc.oid - INNER JOIN - pg_catalog.pg_namespace AS proc_namespace - ON pg_proc.pronamespace = proc_namespace.oid -WHERE depend.classid = $1::REGCLASS - AND depend.objid = $2 - AND depend.deptype = 'n' +INNER JOIN pg_catalog.pg_proc AS pg_proc + ON + depend.refclassid = 'pg_proc'::REGCLASS + AND depend.refobjid = pg_proc.oid +INNER JOIN + pg_catalog.pg_namespace AS proc_namespace + ON pg_proc.pronamespace = proc_namespace.oid +WHERE + depend.classid = $1::REGCLASS + AND depend.objid = $2 + AND depend.deptype = 'n' ` type GetDependsOnFunctionsParams struct { @@ -215,14 +222,15 @@ func (q *Queries) GetDependsOnFunctions(ctx context.Context, arg GetDependsOnFun } const getExtensions = `-- name: GetExtensions :many -SELECT ext.oid, - ext.extname::TEXT AS extension_name, - ext.extversion AS extension_version, - extension_namespace.nspname::TEXT AS schema_name +SELECT + ext.oid, + ext.extname::TEXT AS extension_name, + ext.extversion AS extension_version, + extension_namespace.nspname::TEXT AS schema_name FROM pg_catalog.pg_namespace AS extension_namespace - INNER JOIN - pg_catalog.pg_extension AS ext - ON ext.extnamespace = extension_namespace.oid +INNER JOIN + pg_catalog.pg_extension AS ext + ON ext.extnamespace = extension_namespace.oid WHERE extension_namespace.nspname = 'public' ` @@ -262,29 +270,33 @@ func (q *Queries) GetExtensions(ctx context.Context) ([]GetExtensionsRow, error) } const getForeignKeyConstraints = `-- name: GetForeignKeyConstraints :many -SELECT pg_constraint.conname::TEXT AS constraint_name, - constraint_c.relname::TEXT AS owning_table_name, - constraint_namespace.nspname::TEXT AS owning_table_schema_name, - foreign_table_c.relname::TEXT AS foreign_table_name, - foreign_table_namespace.nspname::TEXT AS foreign_table_schema_name, - pg_constraint.convalidated AS is_valid, - pg_catalog.pg_get_constraintdef(pg_constraint.oid) AS constraint_def +SELECT + pg_constraint.conname::TEXT AS constraint_name, + constraint_c.relname::TEXT AS owning_table_name, + constraint_namespace.nspname::TEXT AS owning_table_schema_name, + foreign_table_c.relname::TEXT AS foreign_table_name, + foreign_table_namespace.nspname::TEXT AS foreign_table_schema_name, + pg_constraint.convalidated AS is_valid, + pg_catalog.pg_get_constraintdef(pg_constraint.oid) AS constraint_def FROM pg_catalog.pg_constraint - INNER JOIN - pg_catalog.pg_class AS constraint_c - ON pg_constraint.conrelid = constraint_c.oid - INNER JOIN pg_catalog.pg_namespace AS constraint_namespace - ON pg_constraint.connamespace = constraint_namespace.oid - INNER JOIN - pg_catalog.pg_class AS foreign_table_c - ON pg_constraint.confrelid = foreign_table_c.oid - INNER JOIN pg_catalog.pg_namespace AS foreign_table_namespace - ON - foreign_table_c.relnamespace = foreign_table_namespace.oid - INNER JOIN pg_catalog.pg_class as index_c ON pg_constraint.conindid = index_c.oid -WHERE constraint_namespace.nspname = 'public' - AND pg_constraint.contype = 'f' - AND pg_constraint.conislocal +INNER JOIN + pg_catalog.pg_class AS constraint_c + ON pg_constraint.conrelid = constraint_c.oid +INNER JOIN pg_catalog.pg_namespace AS constraint_namespace + ON pg_constraint.connamespace = constraint_namespace.oid +INNER JOIN + pg_catalog.pg_class AS foreign_table_c + ON pg_constraint.confrelid = foreign_table_c.oid +INNER JOIN pg_catalog.pg_namespace AS foreign_table_namespace + ON + foreign_table_c.relnamespace = foreign_table_namespace.oid +INNER JOIN + pg_catalog.pg_class AS index_c + ON pg_constraint.conindid = index_c.oid +WHERE + constraint_namespace.nspname = 'public' + AND pg_constraint.contype = 'f' + AND pg_constraint.conislocal ` type GetForeignKeyConstraintsRow struct { @@ -329,30 +341,33 @@ func (q *Queries) GetForeignKeyConstraints(ctx context.Context) ([]GetForeignKey } const getFunctions = `-- name: GetFunctions :many -SELECT pg_proc.oid, - pg_proc.proname::TEXT AS func_name, - proc_namespace.nspname::TEXT AS func_schema_name, - proc_lang.lanname::TEXT AS func_lang, - pg_catalog.pg_get_function_identity_arguments( - pg_proc.oid - ) AS func_identity_arguments, - pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def +SELECT + pg_proc.oid, + pg_proc.proname::TEXT AS func_name, + proc_namespace.nspname::TEXT AS func_schema_name, + proc_lang.lanname::TEXT AS func_lang, + pg_catalog.pg_get_function_identity_arguments( + pg_proc.oid + ) AS func_identity_arguments, + pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def FROM pg_catalog.pg_proc - INNER JOIN - pg_catalog.pg_namespace AS proc_namespace - ON pg_proc.pronamespace = proc_namespace.oid - INNER JOIN - pg_catalog.pg_language AS proc_lang - ON proc_lang.oid = pg_proc.prolang -WHERE proc_namespace.nspname = 'public' - AND pg_proc.prokind = 'f' - -- Exclude functions belonging to extensions - AND NOT EXISTS( +INNER JOIN + pg_catalog.pg_namespace AS proc_namespace + ON pg_proc.pronamespace = proc_namespace.oid +INNER JOIN + pg_catalog.pg_language AS proc_lang + ON proc_lang.oid = pg_proc.prolang +WHERE + proc_namespace.nspname = 'public' + AND pg_proc.prokind = 'f' + -- Exclude functions belonging to extensions + AND NOT EXISTS ( SELECT depend.objid FROM pg_catalog.pg_depend AS depend - WHERE depend.classid = 'pg_proc'::REGCLASS - AND depend.objid = pg_proc.oid - AND depend.deptype = 'e' + WHERE + depend.classid = 'pg_proc'::REGCLASS + AND depend.objid = pg_proc.oid + AND depend.deptype = 'e' ) ` @@ -396,32 +411,36 @@ func (q *Queries) GetFunctions(ctx context.Context) ([]GetFunctionsRow, error) { } const getIndexes = `-- name: GetIndexes :many -SELECT c.oid AS oid, - c.relname::TEXT AS index_name, - table_c.relname::TEXT AS table_name, - pg_catalog.pg_get_indexdef(c.oid)::TEXT AS def_stmt, - COALESCE(con.conname, '')::TEXT AS constraint_name, - i.indisvalid AS index_is_valid, - i.indisprimary AS index_is_pk, - i.indisunique AS index_is_unique, - COALESCE(parent_c.relname, '')::TEXT AS parent_index_name, - COALESCE(parent_namespace.nspname, '')::TEXT AS parent_index_schema_name +SELECT + c.oid AS oid, + c.relname::TEXT AS index_name, + table_c.relname::TEXT AS table_name, + pg_catalog.pg_get_indexdef(c.oid)::TEXT AS def_stmt, + COALESCE(con.conname, '')::TEXT AS constraint_name, + i.indisvalid AS index_is_valid, + i.indisprimary AS index_is_pk, + i.indisunique AS index_is_unique, + COALESCE(parent_c.relname, '')::TEXT AS parent_index_name, + COALESCE(parent_namespace.nspname, '')::TEXT AS parent_index_schema_name FROM pg_catalog.pg_class AS c - INNER JOIN pg_catalog.pg_index AS i ON (i.indexrelid = c.oid) - INNER JOIN pg_catalog.pg_class AS table_c ON (table_c.oid = i.indrelid) - LEFT JOIN pg_catalog.pg_constraint AS con ON (con.conindid = c.oid AND con.contype IN ('p', 'u', NULL)) - LEFT JOIN - pg_catalog.pg_inherits AS idx_inherits - ON (c.oid = idx_inherits.inhrelid) - LEFT JOIN - pg_catalog.pg_class AS parent_c - ON (idx_inherits.inhparent = parent_c.oid) - LEFT JOIN - pg_catalog.pg_namespace AS parent_namespace - ON parent_c.relnamespace = parent_namespace.oid -WHERE c.relnamespace +INNER JOIN pg_catalog.pg_index AS i ON (i.indexrelid = c.oid) +INNER JOIN pg_catalog.pg_class AS table_c ON (table_c.oid = i.indrelid) +LEFT JOIN + pg_catalog.pg_constraint AS con + ON (con.conindid = c.oid AND con.contype IN ('p', 'u', NULL)) +LEFT JOIN + pg_catalog.pg_inherits AS idx_inherits + ON (c.oid = idx_inherits.inhrelid) +LEFT JOIN + pg_catalog.pg_class AS parent_c + ON (idx_inherits.inhparent = parent_c.oid) +LEFT JOIN + pg_catalog.pg_namespace AS parent_namespace + ON parent_c.relnamespace = parent_namespace.oid +WHERE + c.relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'public') - AND (c.relkind = 'i' OR c.relkind = 'I') + AND (c.relkind = 'i' OR c.relkind = 'I') ` type GetIndexesRow struct { @@ -472,43 +491,45 @@ func (q *Queries) GetIndexes(ctx context.Context) ([]GetIndexesRow, error) { } const getSequences = `-- name: GetSequences :many -SELECT seq_c.relname::TEXT AS sequence_name, - seq_ns.nspname::TEXT AS sequence_schema_name, - COALESCE(owner_attr.attname, '')::TEXT AS owner_column_name, - COALESCE(owner_ns.nspname, '')::TEXT AS owner_schema_name, - COALESCE(owner_c.relname, '')::TEXT AS owner_table_name, - pg_seq.seqstart AS start_value, - pg_seq.seqincrement AS increment_value, - pg_seq.seqmax AS max_value, - pg_seq.seqmin AS min_value, - pg_seq.seqcache AS cache_size, - pg_seq.seqcycle AS is_cycle, - FORMAT_TYPE(pg_seq.seqtypid, NULL) AS data_type +SELECT + seq_c.relname::TEXT AS sequence_name, + seq_ns.nspname::TEXT AS sequence_schema_name, + COALESCE(owner_attr.attname, '')::TEXT AS owner_column_name, + COALESCE(owner_ns.nspname, '')::TEXT AS owner_schema_name, + COALESCE(owner_c.relname, '')::TEXT AS owner_table_name, + pg_seq.seqstart AS start_value, + pg_seq.seqincrement AS increment_value, + pg_seq.seqmax AS max_value, + pg_seq.seqmin AS min_value, + pg_seq.seqcache AS cache_size, + pg_seq.seqcycle AS is_cycle, + FORMAT_TYPE(pg_seq.seqtypid, NULL) AS data_type FROM pg_catalog.pg_sequence AS pg_seq - INNER JOIN pg_catalog.pg_class AS seq_c ON pg_seq.seqrelid = seq_c.oid - INNER JOIN pg_catalog.pg_namespace AS seq_ns ON seq_c.relnamespace = seq_ns.oid - LEFT JOIN pg_catalog.pg_depend AS depend - ON - depend.classid = 'pg_class'::REGCLASS - AND pg_seq.seqrelid = depend.objid - AND depend.refclassid = 'pg_class'::REGCLASS - AND depend.deptype = 'a' - LEFT JOIN pg_catalog.pg_attribute AS owner_attr - ON - depend.refobjid = owner_attr.attrelid - AND depend.refobjsubid = owner_attr.attnum - LEFT JOIN pg_catalog.pg_class AS owner_c ON depend.refobjid = owner_c.oid - LEFT JOIN - pg_catalog.pg_namespace AS owner_ns - ON owner_c.relnamespace = owner_ns.oid +INNER JOIN pg_catalog.pg_class AS seq_c ON pg_seq.seqrelid = seq_c.oid +INNER JOIN pg_catalog.pg_namespace AS seq_ns ON seq_c.relnamespace = seq_ns.oid +LEFT JOIN pg_catalog.pg_depend AS depend + ON + depend.classid = 'pg_class'::REGCLASS + AND pg_seq.seqrelid = depend.objid + AND depend.refclassid = 'pg_class'::REGCLASS + AND depend.deptype = 'a' +LEFT JOIN pg_catalog.pg_attribute AS owner_attr + ON + depend.refobjid = owner_attr.attrelid + AND depend.refobjsubid = owner_attr.attnum +LEFT JOIN pg_catalog.pg_class AS owner_c ON depend.refobjid = owner_c.oid +LEFT JOIN + pg_catalog.pg_namespace AS owner_ns + ON owner_c.relnamespace = owner_ns.oid WHERE seq_ns.nspname = 'public' - AND NOT EXISTS( - SELECT ext_depend.objid - FROM pg_catalog.pg_depend AS ext_depend - WHERE ext_depend.classid = 'pg_class'::REGCLASS - AND ext_depend.objid = pg_seq.seqrelid - AND ext_depend.deptype = 'e' - ) +AND NOT EXISTS ( + SELECT ext_depend.objid + FROM pg_catalog.pg_depend AS ext_depend + WHERE + ext_depend.classid = 'pg_class'::REGCLASS + AND ext_depend.objid = pg_seq.seqrelid + AND ext_depend.deptype = 'e' +) ` type GetSequencesRow struct { @@ -564,32 +585,34 @@ func (q *Queries) GetSequences(ctx context.Context) ([]GetSequencesRow, error) { } const getTables = `-- name: GetTables :many -SELECT c.oid AS oid, - c.relname::TEXT AS table_name, - COALESCE(parent_c.relname, '')::TEXT AS parent_table_name, - COALESCE(parent_namespace.nspname, '')::TEXT AS parent_table_schema_name, - (CASE - WHEN c.relkind = 'p' THEN pg_catalog.pg_get_partkeydef(c.oid) - ELSE '' - END)::TEXT - AS partition_key_def, - (CASE - WHEN c.relispartition THEN pg_catalog.pg_get_expr(c.relpartbound, c.oid) - ELSE '' - END)::TEXT AS partition_for_values +SELECT + c.oid AS oid, + c.relname::TEXT AS table_name, + COALESCE(parent_c.relname, '')::TEXT AS parent_table_name, + COALESCE(parent_namespace.nspname, '')::TEXT AS parent_table_schema_name, + (CASE + WHEN c.relkind = 'p' THEN pg_catalog.pg_get_partkeydef(c.oid) + ELSE '' + END)::TEXT + AS partition_key_def, + (CASE + WHEN c.relispartition THEN pg_catalog.pg_get_expr(c.relpartbound, c.oid) + ELSE '' + END)::TEXT AS partition_for_values FROM pg_catalog.pg_class AS c - LEFT JOIN - pg_catalog.pg_inherits AS table_inherits - ON table_inherits.inhrelid = c.oid - LEFT JOIN - pg_catalog.pg_class AS parent_c - ON table_inherits.inhparent = parent_c.oid - LEFT JOIN - pg_catalog.pg_namespace AS parent_namespace - ON parent_c.relnamespace = parent_namespace.oid -WHERE c.relnamespace +LEFT JOIN + pg_catalog.pg_inherits AS table_inherits + ON table_inherits.inhrelid = c.oid +LEFT JOIN + pg_catalog.pg_class AS parent_c + ON table_inherits.inhparent = parent_c.oid +LEFT JOIN + pg_catalog.pg_namespace AS parent_namespace + ON parent_c.relnamespace = parent_namespace.oid +WHERE + c.relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE nspname = 'public') - AND (c.relkind = 'r' OR c.relkind = 'p') + AND (c.relkind = 'r' OR c.relkind = 'p') ` type GetTablesRow struct { @@ -632,28 +655,30 @@ func (q *Queries) GetTables(ctx context.Context) ([]GetTablesRow, error) { } const getTriggers = `-- name: GetTriggers :many -SELECT trig.tgname::TEXT AS trigger_name, - owning_c.relname::TEXT AS owning_table_name, - owning_c_namespace.nspname::TEXT AS owning_table_schema_name, - pg_proc.proname::TEXT AS func_name, - proc_namespace.nspname::TEXT AS func_schema_name, - pg_catalog.pg_get_function_identity_arguments( - pg_proc.oid - ) AS func_identity_arguments, - pg_catalog.pg_get_triggerdef(trig.oid) AS trigger_def +SELECT + trig.tgname::TEXT AS trigger_name, + owning_c.relname::TEXT AS owning_table_name, + owning_c_namespace.nspname::TEXT AS owning_table_schema_name, + pg_proc.proname::TEXT AS func_name, + proc_namespace.nspname::TEXT AS func_schema_name, + pg_catalog.pg_get_function_identity_arguments( + pg_proc.oid + ) AS func_identity_arguments, + pg_catalog.pg_get_triggerdef(trig.oid) AS trigger_def FROM pg_catalog.pg_trigger AS trig - INNER JOIN pg_catalog.pg_class AS owning_c ON trig.tgrelid = owning_c.oid - INNER JOIN - pg_catalog.pg_namespace AS owning_c_namespace - ON owning_c.relnamespace = owning_c_namespace.oid - INNER JOIN pg_catalog.pg_proc AS pg_proc ON trig.tgfoid = pg_proc.oid - INNER JOIN - pg_catalog.pg_namespace AS proc_namespace - ON pg_proc.pronamespace = proc_namespace.oid -WHERE proc_namespace.nspname = 'public' - AND owning_c_namespace.nspname = 'public' - AND trig.tgparentid = 0 - AND NOT trig.tgisinternal +INNER JOIN pg_catalog.pg_class AS owning_c ON trig.tgrelid = owning_c.oid +INNER JOIN + pg_catalog.pg_namespace AS owning_c_namespace + ON owning_c.relnamespace = owning_c_namespace.oid +INNER JOIN pg_catalog.pg_proc AS pg_proc ON trig.tgfoid = pg_proc.oid +INNER JOIN + pg_catalog.pg_namespace AS proc_namespace + ON pg_proc.pronamespace = proc_namespace.oid +WHERE + proc_namespace.nspname = 'public' + AND owning_c_namespace.nspname = 'public' + AND trig.tgparentid = 0 + AND NOT trig.tgisinternal ` type GetTriggersRow struct { diff --git a/pkg/schema/schema_test.go b/pkg/schema/schema_test.go index 8864fd0..d356406 100644 --- a/pkg/schema/schema_test.go +++ b/pkg/schema/schema_test.go @@ -75,7 +75,7 @@ func (suite *schemaTestSuite) TestGetPublicSchemaHash() { EXECUTE PROCEDURE increment_version(); ` - expectedHash = "7c9c30dde1b65875" + expectedHash = "692e5ef48a76de96" ) db, err := suite.pgEngine.CreateDatabase() suite.Require().NoError(err) From e5403fbd6008a37fe08bd3fe04984d012de51a7c Mon Sep 17 00:00:00 2001 From: Bryce Plunkett <130488190+bplunkett-stripe@users.noreply.github.com> Date: Thu, 10 Aug 2023 09:24:50 -0700 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: alexaub-stripe <130488060+alexaub-stripe@users.noreply.github.com> --- pkg/diff/sql_generator.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index c00b140..d2ba86a 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -1512,8 +1512,7 @@ func (f *foreignKeyConstraintSQLVertexGenerator) GetDeleteDependencies(con schem mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildTableVertexId(con.OwningTableUnescapedName), diffTypeDelete), mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildTableVertexId(con.ForeignTableUnescapedName), diffTypeDelete), } - // This is the slightly lazy way of ensuring the foreign key constraint is added after the requisite index is - // built and marked as valid. + // This is the slightly lazy way of ensuring the foreign key constraint is deleted before the index it depends on is deleted // We __could__ do this just for the index the fk depends on, but that's slightly more wiring than we need right now // because of partitioned indexes, which are only valid when all child indexes have been built for _, i := range f.indexInOldSchemaByTableName[con.ForeignTableUnescapedName] {