Re: [PATCH] REPLICA IDENTITY USING INDEX accepts column with invalid NOT NULL

On Tue, May 26, 2026 at 2:36 AM Aleksander Alekseev
<aleksander(at)tigerdata(dot)com> wrote:
>
> Hi Ante,
>
> > ALTER TABLE ... REPLICA IDENTITY USING INDEX checks key columns by
> > reading attnotnull, but since a379061a22a8 (PG 18, NOT NULL NOT VALID)
> > attnotnull is set even when the constraint is unvalidated. An index on
> > a column that actually contains NULLs is therefore accepted as replica
> > identity:
> >
> > CREATE TABLE t (id int);
> > INSERT INTO t VALUES (1), (NULL), (2);
> > ALTER TABLE t ADD CONSTRAINT id_nn NOT NULL id NOT VALID;
> > CREATE UNIQUE INDEX t_idx ON t(id);
> > ALTER TABLE t REPLICA IDENTITY USING INDEX t_idx; -- accepted
> > -- relreplident => 'i'
> >
> > This would cause data divergence on UPDATE/DELETE targeting NULL-keyed rows.
> >
> > Patch follows same pattern as d9ffc27291f (the same bugfix for identity columns);
> > after the attnotnull check, look up the constraint and reject if !convalidated.
>

Good catch!

> Thanks for the patch. This clearly seems to be a bug.
>
> The patch is well written and has regression tests. The tests pass, I
> also tested under Valgrind. All in all the patch looks good to me.

Regarding the patch,

+ /*
+ * 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("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"));
+ heap_freetuple(contup);
+ }

It would be better to factor the into a common function that checks
whether the column has a valid NOT NULL constraint, to remove the code
duplication.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com