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)