Thread: Re: Difference in dump from original and restored database due to NOT NULL constraints on children

On 2024-Nov-27, Ashutosh Bapat wrote:

> I studied this in more details. Here's what is happening

First, thank you very much for spending time on this!

> First case: unnamed/default named constraint
> -------------------------------------------------------------
> On original database following DDLs are executed
> #CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED);
> #CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS
> (notnull_tbl4);
> #select conname, coninhcount, conislocal, contype from pg_constraint
> where conrelid = 'notnull_tbl4_cld2'::regclass;
>            conname            | coninhcount | conislocal | contype
> ------------------------------+-------------+------------+---------
>  notnull_tbl4_cld2_a_not_null |           1 | t          | n
>  notnull_tbl4_cld2_pkey       |           0 | t          | p
> (2 rows)
> 
> Though the child inherited primary key constraint it was overridden by
> local constraint that's how we see coninhcount = 0 and conislocal = t.
> But NOT NULL constraint shows both inherited and local (coninhcount =
> 1 and conislocal = t) because of the logic in
> AdjustNotNullInheritance(). When the table is dumped, it is dumped as
> 
> CREATE TABLE public.notnull_tbl4 (
>     a integer NOT NULL
> );
> CREATE TABLE public.notnull_tbl4_cld2 (
> )
> INHERITS (public.notnull_tbl4);
> ALTER TABLE ONLY public.notnull_tbl4_cld2 ALTER COLUMN a SET NOT NULL;

Hmm.  Actually, I think this would work fine if we make pg_dump emit the
constraint with its name with the child creation, _without the column_:

CREATE TABLE public.notnull_tbl4_cld2 (                                                                               
   PRIMARY KEY (a) DEFERRABLE,
   CONSTRAINT notnull_tbl4_cld2_a_not_null NOT NULL a
)
INHERITS (public.notnull_tbl4);

This works already seems a lot simpler.  The column definition is
obtained from the parent, but the constraint is defined locally in
addition to inherited.  So we just need to change pg_dump to dump local
constraints using that syntax; no backend changes needed.

I elided a lot of your email here because I didn't understand it.
I think you pasted your commit message several times.

> You suggested using ALTER TABLE ... RENAME CONSTRAINT, but renaming an
> inherited constraint is not allowed.
> #alter table notnull_tbl4_cld3 rename constraint
> notnull_tbl4_a_not_null to a_nn;
> ERROR:  cannot rename inherited constraint "notnull_tbl4_a_not_null"
> ... that goes back 13 years.
> commit 39d74e346c083aa371ba64c4edb1332c40b56530
> Author: Peter Eisentraut <peter_e@gmx.net>
> Date:   Sat Mar 10 20:19:13 2012 +0200
> 
>     Add support for renaming constraints
> 
>     reviewed by Josh Berkus and Dimitri Fontaine
> So I don't think we should change that behaviour.

I don't necessarily agree with feeling ourselves restricted by this; I
have to assume that that case was forbidden for CHECK constraints
because they must have identical names on all children, because we match
them by name when walking down hierarchies for whatever DDL operation.
But as has been established, constraint names don't have to be identical
for not-null constraints, because we don't match them by name.

However, I'm not sure we need to remove this restriction, at least not
to fix this problem.

As far as backwards compatibility arguments go, I think we need to
observe a rule like: if something worked previously, then it should
continue to work, otherwise there's a regression.  But if something
didn't previously work, then it's okay to make it work.  You're not
regressing anything.


> 2. Allow local and inherited NOT NULL constraints to co-exist
> separately (similar to primary key constraint). I am not sure why we
> don't allow this behaviour. AdjustNotNullInheritance() doesn't explain
> the reason.

IIRC we discussed this during the previous discussion of not-null
constraints (in the 16 cycle).  The main problem is that if you have two
not-null constraints for the same column, and the user runs ALTER TABLE
.. DROP NOT NULL, what do you think should happen?  We saw three
alternatives, neither of them good:

1. arbitrarily choose one to drop, leave the others alone
2. remove them all
3. error out and ask the user to use DROP CONSTRAINT instead, to choose
which one to drop.


If you look at the SQL standard 2016 edition, they have 11.16 <drop
column not null clause>, whose General Rules say this:

1) If the column descriptor of C contains an indication that C is defined as
   NOT NULL, then:
   a) Let ACN be the constraint name of the associated table constraint
      definition included in the column descriptor of C.
   b) The column descriptor of C is modified as follows:
      i) The indication that C is defined as NOT NULL is removed.
      ii) The constraint name of the associated table constraint definition is removed.

This is clearly assuming that we have a single constraint to drop.
I don't think it'd be wise to change this.  Initially we allowed
multiple not-null constraints per column, and when we decided to
disallow it, a lot of code that supported that case could be removed.
I'm not inclined to put that back.  Things start to become brittle if
you do; I had to work pretty hard to make it all robust.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I can see support will not be a problem.  10 out of 10."    (Simon Wittber)
      (http://archives.postgresql.org/pgsql-general/2004-12/msg00159.php)

On Wed, Nov 27, 2024 at 5:56 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Nov-27, Ashutosh Bapat wrote:
>
> > I studied this in more details. Here's what is happening
>
> First, thank you very much for spending time on this!
>
> > First case: unnamed/default named constraint
> > -------------------------------------------------------------
> > On original database following DDLs are executed
> > #CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED);
> > #CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS
> > (notnull_tbl4);
> > #select conname, coninhcount, conislocal, contype from pg_constraint
> > where conrelid = 'notnull_tbl4_cld2'::regclass;
> >            conname            | coninhcount | conislocal | contype
> > ------------------------------+-------------+------------+---------
> >  notnull_tbl4_cld2_a_not_null |           1 | t          | n
> >  notnull_tbl4_cld2_pkey       |           0 | t          | p
> > (2 rows)
> >
> > Though the child inherited primary key constraint it was overridden by
> > local constraint that's how we see coninhcount = 0 and conislocal = t.
> > But NOT NULL constraint shows both inherited and local (coninhcount =
> > 1 and conislocal = t) because of the logic in
> > AdjustNotNullInheritance(). When the table is dumped, it is dumped as
> >
> > CREATE TABLE public.notnull_tbl4 (
> >     a integer NOT NULL
> > );
> > CREATE TABLE public.notnull_tbl4_cld2 (
> > )
> > INHERITS (public.notnull_tbl4);
> > ALTER TABLE ONLY public.notnull_tbl4_cld2 ALTER COLUMN a SET NOT NULL;
>
> Hmm.  Actually, I think this would work fine if we make pg_dump emit the
> constraint with its name with the child creation, _without the column_:
>
> CREATE TABLE public.notnull_tbl4_cld2 (
>    PRIMARY KEY (a) DEFERRABLE,
>    CONSTRAINT notnull_tbl4_cld2_a_not_null NOT NULL a
> )
> INHERITS (public.notnull_tbl4);
>
> This works already seems a lot simpler.  The column definition is
> obtained from the parent, but the constraint is defined locally in
> addition to inherited.  So we just need to change pg_dump to dump local
> constraints using that syntax; no backend changes needed.
>

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.

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.

--
Best Wishes,
Ashutosh Bapat

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"

On Thu, Nov 28, 2024 at 4:44 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Nov 27, 2024 at 7:04 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > 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.
> >
>
> Ok. Here's the patch implementing the same. As you said, it's a much
> simpler patch. The test being developed in [1] passes with this
> change. pg_dump and pg_upgrade test suites also pass.
>
> [1]
https://www.postgresql.org/message-id/flat/CAExHW5uvx2LEyrUBdctV5gS25Zeb%2B-eXESkK93siQxWSjYFy6A%40mail.gmail.com#c8ed57b77d2f6132d5b8e1ecb2a8c47b
>
> Adding this to CF for CI run.

CF entry: https://commitfest.postgresql.org/51/5408/

--
Best Wishes,
Ashutosh Bapat