From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Tender Wang <tndrwang@gmail.com>
Subject: Re: not null constraints, again
Date: Fri, 8 Nov 2024 10:25:57 +0100
Message-ID: <202411080925.z3appqymaoaw@alvherre.pgsql> (raw)
In-Reply-To: <CACJufxHpi2vL_S+S0Pw8v0W=J7R9Wvz5v7OW7YdJnd9ZHJgr+w@mail.gmail.com>
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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, jian.universality@gmail.com, pgsql-hackers@lists.postgresql.org, tndrwang@gmail.com
Subject: Re: not null constraints, again
In-Reply-To: <202411080925.z3appqymaoaw@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox