Feedback-ID: ia2694551:Fastmail
Date: Wed, 27 Nov 2024 14:33:58 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Difference in dump from original and restored database due to
 NOT NULL constraints on children
Message-ID: <202411271333.iwaete4g4ltv@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAExHW5u+TotftfLH7v_7vT36WvQfYs5Uha7h+6s2htmfrs2_Qg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202411271333.iwaete4g4ltv%40alvherre.pgsql>
Precedence: bulk

On 2024-Nov-27, Ashutosh Bapat wrote:

> I noticed that. But two reasons why I chose the backend changes
> 1. The comment where we add explicit ADD CONSTRAINT is
> /*
> * Dump additional per-column properties that we can't handle in the
> * main CREATE TABLE command.
> */
> ... snip
> 
> /*
> * If we didn't dump the column definition explicitly above, and
> * it is not-null and did not inherit that property from a parent,
> * we have to mark it separately.
> */
> if (!shouldPrintColumn(dopt, tbinfo, j) &&
> tbinfo->notnull_constrs[j] != NULL &&
> (tbinfo->notnull_islocal[j] && !tbinfo->ispartition && !dopt->binary_upgrade))
> ... snip
> 
> The comment seems to say that we can not handle the NOT NULL
> constraint property in the CREATE TABLE command. Don't know why. We
> add CHECK constraints separately in CREATE TABLE even if we didn't add
> corresponding columns in CREATE TABLE. So there must be a reason not
> to dump NOT NULL constraints that way and hence we required separate
> code like above. I am afraid going that direction will show us some
> other problems.

I don't think this is an important restriction.  We can change that, as
long as all cases work correctly.  We previously didn't try to use
"CONSTRAINT foobar NOT NULL a" because 1) we didn't support the
table-constraint syntax for not-null constraint and 2) not-null
constraint didn't support names anyway.  We now support that syntax, so
we can use it.

> 2. Name of the constraint provided by ALTER TABLE ... ADD CONSTRAINT
> ... being silently ignored didn't seem right to me. Especially when we
> were adjusting the constraint to be local.

We could make this throw an error when the names don't match; I don't
think that'd affect anything important.  I'm not in love with the idea
of ADD CONSTRAINT changing the constraint name.  Or, to be more precise,
I think having ALTER TABLE ADD CONSTRAINT change the name of an existing
constraint is a terrible idea.  I will forever be shamed publicly if I
let that happen, and frankly I don't need any more reasons to be shamed
publicly, as I have plenty already.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"