Re: NOT NULL NOT ENFORCED - Mailing list pgsql-hackers

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)