Re: not null constraints, again

On 2024-Sep-11, jian he wrote:

> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > Hello, here's a v2 of this patch. I have fixed --I think-- all the
> > issues you and Tender Wang reported (unless I declined a fix in some
> > previous email).
> >
>
> + /*
> + * The constraint must appear as inherited in children, so create a
> + * modified constraint object to use.
> + */
> + constr = copyObject(constr);
> + constr->inhcount = 1;
>
> in ATAddCheckNNConstraint, we don't need the above copyObject call.
> because at the beginning of ATAddCheckNNConstraint, we do
> newcons = AddRelationNewConstraints(rel, NIL,
> list_make1(copyObject(constr)),
> recursing || is_readd, /*
> allow_merge */
> !recursing, /* is_local */
> is_readd, /* is_internal */
> NULL); /* queryString not available
> * here */

I'm disinclined to change this. The purpose of creating a copy at this
point is to avoid modifying an object that doesn't belong to us. It
doesn't really matter much that we have an additional copy anyway; this
isn't in any way performance-critical or memory-intensive.

> create table idxpart (a int) partition by range (a);
> create table idxpart0 (like idxpart);
> alter table idxpart0 add primary key (a);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table idxpart add primary key (a);
>
> alter table idxpart0 DROP CONSTRAINT idxpart0_pkey;
> alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null;
>
> First DROP CONSTRAINT failed as the doc said,
> but the second success.
> but the second DROP CONSTRAINT should fail?
> Even if you drop success, idxpart0_a_not_null still exists.
> it also conflicts with the pg_constraint I've quoted above.

Hmm, this is because we allow a DROP CONSTRAINT to set conislocal from
true to false. So the constraint isn't *actually* dropped. If you try
the DROP CONSTRAINT a second time, you'll get an error then. Maybe I
should change the order of checks here, so that we forbid doing the
conislocal change; that would be more consistent with what we document.
I'm undecided about this TBH -- maybe I should reword the documentation
you cite in a different way.

> transformTableLikeClause, expandTableLikeClause
> can be further simplified when the relation don't have not-null as all like:
>
> /*
> * Reproduce not-null constraints by copying them. This doesn't require
> * any option to have been given.
> */
> if (tupleDesc->constr && tupleDesc->constr->has_not_null)
> {
> lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false);
> cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);
> }

True.

> Also, seems AdjustNotNullInheritance never being called/used?

Oh, right, I removed the last callsite recently. I'll remove the
function, and rename AdjustNotNullInheritance1 to
AdjustNotNullInheritance now that that name is free.

Thanks for reviewing! I'll handle your other comment tomorrow.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/