From 270af6f0df764d326e1a7355f4ce10dc73b05dac Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 8 Mar 2024 16:32:29 +0100 Subject: [PATCH] Admit deferrable PKs into rd_pkindex, but flag them as such ... and in particular don't return them as replica identity. The motivation for this change is letting the primary keys be seen by code that derives NOT NULL constraints from them, when creating inheritance children; before this change, if you had a deferrable PK, pg_dump would not recreate the attnotnull marking properly, because the column would not be considered as having anything to back said marking after dropping the throwaway NOT NULL constraint. The reason we don't want these PKs as replica identities is that replication can corrupt data, if the uniqueness constraint is transiently broken. Reported-by: Amul Sul Reviewed-by: Dean Rasheed Discussion: https://postgr.es/m/CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com --- src/backend/replication/logical/relation.c | 6 +- src/backend/utils/cache/relcache.c | 27 ++++-- src/include/utils/rel.h | 3 +- src/test/regress/expected/constraints.out | 92 +++++++++++++++++++ src/test/regress/expected/publication.out | 10 ++ .../regress/expected/replica_identity.out | 5 + src/test/regress/sql/constraints.sql | 25 +++++ src/test/regress/sql/publication.sql | 9 ++ src/test/regress/sql/replica_identity.sql | 4 + 9 files changed, 170 insertions(+), 11 deletions(-) diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index 4f79f46cae2..f139e7b01e9 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -843,9 +843,9 @@ IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo, AttrMap *attrmap) } /* - * Get replica identity index or if it is not defined a primary key. - * - * If neither is defined, returns InvalidOid + * Return the OID of the replica identity index if one is defined; + * the OID of the PK if one exists and is not deferrable; + * otherwise, InvalidOid. */ Oid GetRelationIdentityOrPK(Relation rel) diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 4d339ee7950..7ad6a35a50e 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -4761,6 +4761,7 @@ RelationGetIndexList(Relation relation) char replident = relation->rd_rel->relreplident; Oid pkeyIndex = InvalidOid; Oid candidateIndex = InvalidOid; + bool pkdeferrable = false; MemoryContext oldcxt; /* Quick exit if we already computed the list. */ @@ -4802,12 +4803,12 @@ RelationGetIndexList(Relation relation) result = lappend_oid(result, index->indexrelid); /* - * Non-unique, non-immediate or predicate indexes aren't interesting - * for either oid indexes or replication identity indexes, so don't - * check them. + * Non-unique or predicate indexes aren't interesting for either oid + * indexes or replication identity indexes, so don't check them. + * Deferred ones are not useful for replication identity either; but + * we do include them if they are PKs. */ if (!index->indisunique || - !index->indimmediate || !heap_attisnull(htup, Anum_pg_index_indpred, NULL)) continue; @@ -4832,7 +4833,13 @@ RelationGetIndexList(Relation relation) if (index->indisprimary && (index->indisvalid || relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)) + { pkeyIndex = index->indexrelid; + pkdeferrable = !index->indimmediate; + } + + if (!index->indimmediate) + continue; if (!index->indisvalid) continue; @@ -4854,7 +4861,8 @@ RelationGetIndexList(Relation relation) oldlist = relation->rd_indexlist; relation->rd_indexlist = list_copy(result); relation->rd_pkindex = pkeyIndex; - if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex)) + relation->rd_ispkdeferrable = pkdeferrable; + if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex) && !pkdeferrable) relation->rd_replidindex = pkeyIndex; else if (replident == REPLICA_IDENTITY_INDEX && OidIsValid(candidateIndex)) relation->rd_replidindex = candidateIndex; @@ -4957,7 +4965,8 @@ RelationGetStatExtList(Relation relation) /* * RelationGetPrimaryKeyIndex -- get OID of the relation's primary key index * - * Returns InvalidOid if there is no such index. + * Returns InvalidOid if there is no such index, or if the primary key is + * DEFERRABLE. */ Oid RelationGetPrimaryKeyIndex(Relation relation) @@ -4972,7 +4981,7 @@ RelationGetPrimaryKeyIndex(Relation relation) Assert(relation->rd_indexvalid); } - return relation->rd_pkindex; + return relation->rd_ispkdeferrable ? InvalidOid : relation->rd_pkindex; } /* @@ -5190,6 +5199,7 @@ RelationGetIndexPredicate(Relation relation) * INDEX_ATTR_BITMAP_KEY Columns in non-partial unique indexes not * in expressions (i.e., usable for FKs) * INDEX_ATTR_BITMAP_PRIMARY_KEY Columns in the table's primary key + * (beware: even if PK is deferrable!) * INDEX_ATTR_BITMAP_IDENTITY_KEY Columns in the table's replica identity * index (empty if FULL) * INDEX_ATTR_BITMAP_HOT_BLOCKING Columns that block updates from being HOT @@ -5198,6 +5208,9 @@ RelationGetIndexPredicate(Relation relation) * Attribute numbers are offset by FirstLowInvalidHeapAttributeNumber so that * we can include system attributes (e.g., OID) in the bitmap representation. * + * Deferred indexes are considered for the primary key, but not for replica + * identity. + * * Caller had better hold at least RowExclusiveLock on the target relation * to ensure it is safe (deadlock-free) for us to take locks on the relation's * indexes. Note that since the introduction of CREATE INDEX CONCURRENTLY, diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index b3ea2b20421..87002049538 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -150,7 +150,8 @@ typedef struct RelationData /* data managed by RelationGetIndexList: */ List *rd_indexlist; /* list of OIDs of indexes on relation */ - Oid rd_pkindex; /* OID of primary key, if any */ + Oid rd_pkindex; /* OID of (deferrable?) primary key, if any */ + bool rd_ispkdeferrable; /* is rd_pkindex a deferrable PK? */ Oid rd_replidindex; /* OID of replica identity index, if any */ /* data managed by RelationGetStatExtList: */ diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index 5b068477bf8..d9d8408e869 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -1017,6 +1017,98 @@ create table cnn2_part1(a int primary key); alter table cnn2_parted attach partition cnn2_part1 for values in (1); ERROR: column "a" in child table must be marked NOT NULL drop table cnn2_parted, cnn2_part1; +-- columns in regular and LIKE inheritance should be marked not-nullable +-- for primary keys, even if those are deferred +CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED); +CREATE TABLE notnull_tbl4_lk (LIKE notnull_tbl4); +CREATE TABLE notnull_tbl4_lk2 (LIKE notnull_tbl4 INCLUDING INDEXES); +CREATE TABLE notnull_tbl4_lk3 (LIKE notnull_tbl4 INCLUDING INDEXES, CONSTRAINT a_nn NOT NULL a); +CREATE TABLE notnull_tbl4_cld () INHERITS (notnull_tbl4); +CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS (notnull_tbl4); +CREATE TABLE notnull_tbl4_cld3 (PRIMARY KEY (a) DEFERRABLE, CONSTRAINT a_nn NOT NULL a) INHERITS (notnull_tbl4); +\d+ notnull_tbl4 + Table "public.notnull_tbl4" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Indexes: + "notnull_tbl4_pkey" PRIMARY KEY, btree (a) DEFERRABLE INITIALLY DEFERRED +Child tables: notnull_tbl4_cld, + notnull_tbl4_cld2, + notnull_tbl4_cld3 + +\d+ notnull_tbl4_lk + Table "public.notnull_tbl4_lk" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Not-null constraints: + "notnull_tbl4_lk_a_not_null" NOT NULL "a" + +\d+ notnull_tbl4_lk2 + Table "public.notnull_tbl4_lk2" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Indexes: + "notnull_tbl4_lk2_pkey" PRIMARY KEY, btree (a) DEFERRABLE INITIALLY DEFERRED + +\d+ notnull_tbl4_lk3 + Table "public.notnull_tbl4_lk3" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Indexes: + "notnull_tbl4_lk3_pkey" PRIMARY KEY, btree (a) DEFERRABLE INITIALLY DEFERRED +Not-null constraints: + "a_nn" NOT NULL "a" + +\d+ notnull_tbl4_cld + Table "public.notnull_tbl4_cld" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Not-null constraints: + "notnull_tbl4_cld_a_not_null" NOT NULL "a" (inherited) +Inherits: notnull_tbl4 + +\d+ notnull_tbl4_cld2 + Table "public.notnull_tbl4_cld2" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Indexes: + "notnull_tbl4_cld2_pkey" PRIMARY KEY, btree (a) DEFERRABLE +Not-null constraints: + "notnull_tbl4_cld2_a_not_null" NOT NULL "a" (inherited) +Inherits: notnull_tbl4 + +\d+ notnull_tbl4_cld3 + Table "public.notnull_tbl4_cld3" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Indexes: + "notnull_tbl4_cld3_pkey" PRIMARY KEY, btree (a) DEFERRABLE +Not-null constraints: + "a_nn" NOT NULL "a" (local, inherited) +Inherits: notnull_tbl4 + +-- leave these tables around for pg_upgrade testing +-- also, if a NOT NULL is dropped underneath a deferrable PK, the column +-- should still be nullable afterwards. This mimics what pg_dump does. +CREATE TABLE notnull_tbl5 (a INTEGER CONSTRAINT a_nn NOT NULL); +ALTER TABLE notnull_tbl5 ADD PRIMARY KEY (a) DEFERRABLE; +ALTER TABLE notnull_tbl5 DROP CONSTRAINT a_nn; +\d+ notnull_tbl5 + Table "public.notnull_tbl5" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Indexes: + "notnull_tbl5_pkey" PRIMARY KEY, btree (a) DEFERRABLE + +DROP TABLE notnull_tbl5; -- Comments -- Setup a low-level role to enforce non-superuser checks. CREATE ROLE regress_constraint_comments; diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 16361a91f9f..0c5521d2aa9 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -732,6 +732,16 @@ ALTER PUBLICATION testpub_table_ins ADD TABLE testpub_tbl5 (a); -- ok Tables: "public.testpub_tbl5" (a) +-- error: cannot work with deferrable primary keys +CREATE TABLE testpub_tbl5d (a int PRIMARY KEY DEFERRABLE); +ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5d; +UPDATE testpub_tbl5d SET a = 1; +ERROR: cannot update table "testpub_tbl5d" because it does not have a replica identity and publishes updates +HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE. +/* but works fine with FULL replica identity */ +ALTER TABLE testpub_tbl5d REPLICA IDENTITY FULL; +UPDATE testpub_tbl5d SET a = 1; +DROP TABLE testpub_tbl5d; -- tests with REPLICA IDENTITY FULL CREATE TABLE testpub_tbl6 (a int, b text, c text); ALTER TABLE testpub_tbl6 REPLICA IDENTITY FULL; diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out index 6038bf8e9f7..cb3ef599d56 100644 --- a/src/test/regress/expected/replica_identity.out +++ b/src/test/regress/expected/replica_identity.out @@ -7,6 +7,7 @@ CREATE TABLE test_replica_identity ( CONSTRAINT test_replica_identity_unique_nondefer UNIQUE (keya, keyb) ) ; CREATE TABLE test_replica_identity_othertable (id serial primary key); +CREATE TABLE test_replica_identity_t3 (id serial constraint pk primary key deferrable); CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb); CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb); CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey); @@ -57,6 +58,9 @@ ERROR: "test_replica_identity_othertable_pkey" is not an index for table "test_ -- fail, deferrable ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_unique_defer; ERROR: cannot use non-immediate index "test_replica_identity_unique_defer" as replica identity +-- fail, deferrable +ALTER TABLE test_replica_identity_t3 REPLICA IDENTITY USING INDEX pk; +ERROR: cannot use non-immediate index "pk" as replica identity SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity'::regclass; relreplident -------------- @@ -292,3 +296,4 @@ DROP TABLE test_replica_identity3; DROP TABLE test_replica_identity4; DROP TABLE test_replica_identity5; DROP TABLE test_replica_identity_othertable; +DROP TABLE test_replica_identity_t3; diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index a7d96e98f58..87d685ae392 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -668,6 +668,31 @@ create table cnn2_part1(a int primary key); alter table cnn2_parted attach partition cnn2_part1 for values in (1); drop table cnn2_parted, cnn2_part1; +-- columns in regular and LIKE inheritance should be marked not-nullable +-- for primary keys, even if those are deferred +CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED); +CREATE TABLE notnull_tbl4_lk (LIKE notnull_tbl4); +CREATE TABLE notnull_tbl4_lk2 (LIKE notnull_tbl4 INCLUDING INDEXES); +CREATE TABLE notnull_tbl4_lk3 (LIKE notnull_tbl4 INCLUDING INDEXES, CONSTRAINT a_nn NOT NULL a); +CREATE TABLE notnull_tbl4_cld () INHERITS (notnull_tbl4); +CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS (notnull_tbl4); +CREATE TABLE notnull_tbl4_cld3 (PRIMARY KEY (a) DEFERRABLE, CONSTRAINT a_nn NOT NULL a) INHERITS (notnull_tbl4); +\d+ notnull_tbl4 +\d+ notnull_tbl4_lk +\d+ notnull_tbl4_lk2 +\d+ notnull_tbl4_lk3 +\d+ notnull_tbl4_cld +\d+ notnull_tbl4_cld2 +\d+ notnull_tbl4_cld3 +-- leave these tables around for pg_upgrade testing + +-- also, if a NOT NULL is dropped underneath a deferrable PK, the column +-- should still be nullable afterwards. This mimics what pg_dump does. +CREATE TABLE notnull_tbl5 (a INTEGER CONSTRAINT a_nn NOT NULL); +ALTER TABLE notnull_tbl5 ADD PRIMARY KEY (a) DEFERRABLE; +ALTER TABLE notnull_tbl5 DROP CONSTRAINT a_nn; +\d+ notnull_tbl5 +DROP TABLE notnull_tbl5; -- Comments -- Setup a low-level role to enforce non-superuser checks. diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index d5051a5e746..8ba8036bfbd 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -444,6 +444,15 @@ RESET client_min_messages; ALTER PUBLICATION testpub_table_ins ADD TABLE testpub_tbl5 (a); -- ok \dRp+ testpub_table_ins +-- error: cannot work with deferrable primary keys +CREATE TABLE testpub_tbl5d (a int PRIMARY KEY DEFERRABLE); +ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5d; +UPDATE testpub_tbl5d SET a = 1; +/* but works fine with FULL replica identity */ +ALTER TABLE testpub_tbl5d REPLICA IDENTITY FULL; +UPDATE testpub_tbl5d SET a = 1; +DROP TABLE testpub_tbl5d; + -- tests with REPLICA IDENTITY FULL CREATE TABLE testpub_tbl6 (a int, b text, c text); ALTER TABLE testpub_tbl6 REPLICA IDENTITY FULL; diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql index dd43650586c..30daec05b71 100644 --- a/src/test/regress/sql/replica_identity.sql +++ b/src/test/regress/sql/replica_identity.sql @@ -8,6 +8,7 @@ CREATE TABLE test_replica_identity ( ) ; CREATE TABLE test_replica_identity_othertable (id serial primary key); +CREATE TABLE test_replica_identity_t3 (id serial constraint pk primary key deferrable); CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb); CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb); @@ -40,6 +41,8 @@ ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_iden ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_othertable_pkey; -- fail, deferrable ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_unique_defer; +-- fail, deferrable +ALTER TABLE test_replica_identity_t3 REPLICA IDENTITY USING INDEX pk; SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity'::regclass; @@ -137,3 +140,4 @@ DROP TABLE test_replica_identity3; DROP TABLE test_replica_identity4; DROP TABLE test_replica_identity5; DROP TABLE test_replica_identity_othertable; +DROP TABLE test_replica_identity_t3;