Feedback-ID: ia2694551:Fastmail
Date: Fri, 8 Nov 2024 10:25:57 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>,
	Tender Wang <tndrwang@gmail.com>
Subject: Re: not null constraints, again
Message-ID: <202411080925.z3appqymaoaw@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxHpi2vL_S+S0Pw8v0W=J7R9Wvz5v7OW7YdJnd9ZHJgr+w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202411080925.z3appqymaoaw%40alvherre.pgsql>
Precedence: bulk

On 2024-Nov-08, jian he wrote:

> > Here's v11, which I intended to commit today, but didn't get around to.
> > CI is happy with it, so I'll probably do it tomorrow first thing.
> >
> 
> CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b
> INTEGER CONSTRAINT blah NOT NULL);
> 
> RelationGetNotNullConstraints, StoreRelNotNull
> will first create the constraint "blah", then iterate through the
> second "blah" error out,
> which is not great for error out cleaning, i believe.

I applaud your enthusiasm, but I don't like this change.  We have plenty
of cases where we abort a command partway through after having created a
bunch of catalog rows (we even have comments about such behavior being
acceptable); if we wanted to get rid of them all, the code would become
far too complicated because it'd have to save state until the last
minute, just in case something else threw errors.  Your proposed coding
seems complicated enough, in fact, given how fringe an error condition
it's protecting against.  It's not like the user will try to run the
command thousands of times "to see if it works next time".  One dead
catalog row every now and then won't hurt anything.

> while debugging, in RelationGetNotNullConstraints
>         if (cooked)
>         {
>             CookedConstraint *cooked;
>             cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
>             cooked->contype = CONSTR_NOTNULL;
>             cooked->name = pstrdup(NameStr(conForm->conname));
>             cooked->attnum = colnum;
>           .....
>         }
> We missed the assignment of cooked->conoid?

Eh, I can't see the OID would ever be useful for anything, but let's put
it there just in case some future caller wants it for some reason.


> MergeConstraintsIntoExisting
>             /*
>              * If the CHECK child constraint is "no inherit" then cannot
>              * merge.
>              */
>             if (child_con->connoinherit)
>                 ereport(ERROR,
>                         (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>                          errmsg("constraint \"%s\" conflicts with
> non-inherited constraint on child table \"%s\"",
>                                 NameStr(child_con->conname),
> RelationGetRelationName(child_rel))));
> 
> the above comment can also be hit by not-null constraint, so the
> comment is wrong?

Strange ... my copy is fixed already, and in fact I don't see the patch
touching this function at all. [ pokes around ]  Ah, I changed it two
weeks ago:

https://github.com/alvherre/postgres/commit/efeed9416b8c7397d61446958d6835e23ec3f0b6

Thanks for looking once more!

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La grandeza es una experiencia transitoria.  Nunca es consistente.
Depende en gran parte de la imaginación humana creadora de mitos"
(Irulan)