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 <sulamul@gmail.com>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com

Author: alvherre

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)

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,

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: */

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;

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;

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;

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.

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;

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;