Hi all, I've rebased and updated my patch based on Álvaro Herrera's wording suggestion.
I have left the function refactoring for future work as suggested. Thanks, On Mon, Jun 15, 2026 at 8:44 PM Álvaro Herrera <[email protected]> wrote: > On 2026-May-14, Ante Krešić wrote: > > > + if (!conForm->convalidated) > > + ereport(ERROR, > > + > errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > + errmsg("index \"%s\" > cannot be used as replica identity because column \"%s\" has an invalid > not-null constraint", > > + > RelationGetRelationName(indexRel), > > + > NameStr(attr->attname)), > > + /*- translator: second %s is a constraint > characteristic such as NOT VALID */ > > + errdetail("The constraint > \"%s\" is marked %s.", > > + > NameStr(conForm->conname), "NOT VALID"), > > + errhint("You might need to > validate it using %s.", > > + "ALTER > TABLE ... VALIDATE CONSTRAINT")); > > I'd rather make this error be > > + ereport(ERROR, > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("cannot use index \"%s\" as replica identity", > + RelationGetRelationName(indexRel), > + NameStr(attr->attname)), > + /*- translator: third %s is a constraint > characteristic such as NOT VALID */ > + errdetail("The constraint \"%s\" on column \"%s\" is > marked %s.", > + NameStr(conForm->conname), > NameStr(attr->attname), "NOT VALID"), > + errhint("You might need to validate it using %s.", > + "ALTER TABLE ... VALIDATE CONSTRAINT")); > > The other ones which you're mimicking are IMO in poor style. > > -- > Álvaro Herrera Breisgau, Deutschland — > https://www.EnterpriseDB.com/ > Officer Krupke, what are we to do? > Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke") >
From 76929803690f62d1a3fc627a52fdc75f5368b0e4 Mon Sep 17 00:00:00 2001 From: Ante Kresic <[email protected]> Date: Wed, 13 May 2026 10:47:02 +0200 Subject: [PATCH v2] Reject REPLICA IDENTITY USING INDEX on column with invalid NOT NULL ALTER TABLE ... REPLICA IDENTITY USING INDEX verified key columns by reading pg_attribute.attnotnull, but commit a379061a22a8 made attnotnull true also for unvalidated (NOT VALID) not-null constraints, which do not prove the column null-free. An index over such a column could thus be marked as replica identity even though the column might contain NULLs, causing apply-side divergence for UPDATE/DELETE on the nullable rows. Fix by additionally requiring convalidated for the underlying constraint, mirroring the fix d9ffc27291f applied to ATExecAddIdentity for the analogous identity-column case. --- src/backend/commands/tablecmds.c | 28 +++++++++++++++++++ .../regress/expected/replica_identity.out | 23 +++++++++++++++ src/test/regress/sql/replica_identity.sql | 16 +++++++++++ 3 files changed, 67 insertions(+) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 38f9ffcd04f..442c0839dcb 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -18837,6 +18837,34 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode errmsg("index \"%s\" cannot be used as replica identity because column \"%s\" is nullable", RelationGetRelationName(indexRel), NameStr(attr->attname)))); + + /* + * attnotnull is set even for invalid (NOT VALID) not-null + * constraints, which do not prove the column is null-free, so verify + * that the underlying constraint is validated. + */ + { + HeapTuple contup; + Form_pg_constraint conForm; + + contup = findNotNullConstraintAttnum(RelationGetRelid(rel), attno); + if (!HeapTupleIsValid(contup)) + elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation \"%s\"", + NameStr(attr->attname), RelationGetRelationName(rel)); + + conForm = (Form_pg_constraint) GETSTRUCT(contup); + if (!conForm->convalidated) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot use index \"%s\" as replica identity", + RelationGetRelationName(indexRel)), + /*- translator: third %s is a constraint characteristic such as NOT VALID */ + errdetail("The constraint \"%s\" on column \"%s\" is marked %s.", + NameStr(conForm->conname), NameStr(attr->attname), "NOT VALID"), + errhint("You might need to validate it using %s.", + "ALTER TABLE ... VALIDATE CONSTRAINT")); + heap_freetuple(contup); + } } /* This index is suitable for use as a replica identity. Mark it. */ diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out index 336b04fa278..87feaadbb28 100644 --- a/src/test/regress/expected/replica_identity.out +++ b/src/test/regress/expected/replica_identity.out @@ -292,10 +292,33 @@ ALTER TABLE test_replica_identity5 DROP CONSTRAINT test_replica_identity5_pkey; ERROR: constraint "test_replica_identity5_pkey" of relation "test_replica_identity5" does not exist ALTER TABLE test_replica_identity5 ALTER b DROP NOT NULL; ERROR: column "b" is in index used as replica identity +-- An invalid (NOT VALID) not-null constraint sets attnotnull but does not +-- prove the column null-free, so the index must not be accepted as replica +-- identity until the constraint is validated. +CREATE TABLE test_replica_identity6 (id int); +INSERT INTO test_replica_identity6 VALUES (1), (NULL); +ALTER TABLE test_replica_identity6 ADD CONSTRAINT id_nn NOT NULL id NOT VALID; +CREATE UNIQUE INDEX test_replica_identity6_idx ON test_replica_identity6 (id); +-- should fail +ALTER TABLE test_replica_identity6 REPLICA IDENTITY USING INDEX test_replica_identity6_idx; +ERROR: cannot use index "test_replica_identity6_idx" as replica identity +DETAIL: The constraint "id_nn" on column "id" is marked NOT VALID. +HINT: You might need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. +-- after removing offending row and validating, it should succeed +DELETE FROM test_replica_identity6 WHERE id IS NULL; +ALTER TABLE test_replica_identity6 VALIDATE CONSTRAINT id_nn; +ALTER TABLE test_replica_identity6 REPLICA IDENTITY USING INDEX test_replica_identity6_idx; +SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity6'::regclass; + relreplident +-------------- + i +(1 row) + DROP TABLE test_replica_identity; DROP TABLE test_replica_identity2; DROP TABLE test_replica_identity3; DROP TABLE test_replica_identity4; DROP TABLE test_replica_identity5; +DROP TABLE test_replica_identity6; DROP TABLE test_replica_identity_othertable; DROP TABLE test_replica_identity_t3; diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql index 30daec05b71..b202b30ae2b 100644 --- a/src/test/regress/sql/replica_identity.sql +++ b/src/test/regress/sql/replica_identity.sql @@ -134,10 +134,26 @@ ALTER TABLE test_replica_identity5 ALTER b SET NOT NULL; ALTER TABLE test_replica_identity5 DROP CONSTRAINT test_replica_identity5_pkey; ALTER TABLE test_replica_identity5 ALTER b DROP NOT NULL; +-- An invalid (NOT VALID) not-null constraint sets attnotnull but does not +-- prove the column null-free, so the index must not be accepted as replica +-- identity until the constraint is validated. +CREATE TABLE test_replica_identity6 (id int); +INSERT INTO test_replica_identity6 VALUES (1), (NULL); +ALTER TABLE test_replica_identity6 ADD CONSTRAINT id_nn NOT NULL id NOT VALID; +CREATE UNIQUE INDEX test_replica_identity6_idx ON test_replica_identity6 (id); +-- should fail +ALTER TABLE test_replica_identity6 REPLICA IDENTITY USING INDEX test_replica_identity6_idx; +-- after removing offending row and validating, it should succeed +DELETE FROM test_replica_identity6 WHERE id IS NULL; +ALTER TABLE test_replica_identity6 VALIDATE CONSTRAINT id_nn; +ALTER TABLE test_replica_identity6 REPLICA IDENTITY USING INDEX test_replica_identity6_idx; +SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity6'::regclass; + DROP TABLE test_replica_identity; DROP TABLE test_replica_identity2; DROP TABLE test_replica_identity3; DROP TABLE test_replica_identity4; DROP TABLE test_replica_identity5; +DROP TABLE test_replica_identity6; DROP TABLE test_replica_identity_othertable; DROP TABLE test_replica_identity_t3; -- 2.53.0
