Re: NOT NULL NOT ENFORCED

On 2025-Sep-04, jian he wrote:

> @@ -3093,6 +3115,16 @@ AddRelationNotNullConstraints(Relation rel, List *constraints,
> conname = other->name;
>
> inhcount++;
> +
> + /*
> + * if a column inherit multiple not-null constraints, the
> + * enforced status should the same.
> + */
> + if (other->is_enforced != cooked->is_enforced)
> + ereport(ERROR,
> + errcode(ERRCODE_DATATYPE_MISMATCH),
> + errmsg("cannot define not-null constraint on column \"%s\"", conname),
> + errdetail("The column inherited not-null constraints have conflict ENFORCED status."));
> old_notnulls = list_delete_nth_cell(old_notnulls, restpos);
> }
> else

Hmmm, are you sure about this? I think if a table has two parents, one
with enforced and the other with not enforced constraint, then it's okay
to get them combined resulting in one enforced constraint.

> @@ -777,6 +778,18 @@ AdjustNotNullInheritance(Oid relid, AttrNumber attnum,
> errhint("You might need to validate it using %s.",
> "ALTER TABLE ... VALIDATE CONSTRAINT"));
>
> + /*
> + * If the ENFORCED status we're asked for doesn't match what the
> + * existing constraint has, throw an error.
> + */
> + if (is_enforced != conform->conenforced)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot change ENFORCED status of NOT NULL constraint \"%s\" on relation \"%s\"",
> + NameStr(conform->conname), get_rel_name(relid)),
> + errhint("You might need to drop the existing not enforced constraint using %s.",
> + "ALTER TABLE ... DROP CONSTRAINT"));

I think the hint here should suggest to make the existing constraint as
enforced, rather than drop it.

> @@ -9937,9 +9962,9 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
> * If adding a valid not-null constraint, set the pg_attribute flag
> * and tell phase 3 to verify existing rows, if needed. For an
> * invalid constraint, just set attnotnull, without queueing
> - * verification.
> + * verification. For not enforced not-null, no need set attnotnull.
> */
> - if (constr->contype == CONSTR_NOTNULL)
> + if (constr->contype == CONSTR_NOTNULL && ccon->is_enforced)
> set_attnotnull(wqueue, rel, ccon->attnum,
> !constr->skip_validation,
> !constr->skip_validation);

Didn't we decide that attnotnull meant whether a constraint exists at
all, without making a judgement on whether it's enforced or valid? The
important change should be in CheckNNConstraintFetch() which should
determine attnullability in a way that allows executor know whether the
column is nullable or not. I admit we might want to handle this
differently for unenforced constraints, but we should discuss that and
make sure that's what we want.

> + else if (notenforced)
> + {
> + /*
> + * We can't use ATExecSetNotNull here because it adds an enforced
> + * not-null constraint, but here we only want a non-enforced one.
> + */

Umm, wouldn't it make more sense to modify ATExecSetNotNull() so that it
does what we want? This seems hackish.

> @@ -1272,33 +1294,41 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
> * Reproduce not-null constraints, if any, by copying them. We do this
> * regardless of options given.
> */
> - if (tupleDesc->constr && tupleDesc->constr->has_not_null)
> - {
> - List *lst;
> + lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false,
> + true);
> + cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);
>
> - lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false,
> - true);

> + /*
> + * When creating a new relation, marking the enforced not-null constraint as
> + * not valid doesn't make sense, so we treat it as valid.
> + */
> + foreach_node(Constraint, nnconstr, lst)
> + {
> + if (nnconstr->is_enforced)
> + {
> + nnconstr->skip_validation = false;
> + nnconstr->initially_valid = true;
> + }
> + }

Hmmm, this bit here (making constraints as valid if they're not valid in
the source table) looks like a fix for the existing code. I think it
should be a separate patch, perhaps back-patchable to 18. Or maybe I'm
missing something ...?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can." (Ken Rockwell)