Feedback-ID: ia2694551:Fastmail
Date: Wed, 11 Sep 2024 21:32:16 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: not null constraints, again
Message-ID: <202409111932.pxovsxtwxaqu@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxGgY5CK_gBjWynoV4iw1NwfN_mM7g1dhvFhQ8pWesfnKw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202409111932.pxovsxtwxaqu%40alvherre.pgsql>
Precedence: bulk

On 2024-Sep-11, jian he wrote:

> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.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/