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

On Wed, Nov 27, 2024 at 5:56 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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