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

Hi Alvaro,

On Thu, Nov 28, 2024 at 4:47 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Thu, Nov 28, 2024 at 4:44 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, Nov 27, 2024 at 7:04 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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

I looked at the patch again. Here are notes from self-review
1. The code to properly format non-first attribute is related in both
if and else blocks
if (shouldPrintColumn(..))
{
...
}
else if (<not-null attribute conditions>)
{

}
However, the code needs to be executed only when we are printing
something, so it can not be removed outside the if () else ()
structure to avoid duplication. Separating this small piece of code
into a function would add more lines of code than it would save. So I
have left it as is.

2. Improved the comment to make the purpose and context of the code clear.

3. With the code changes in the patch, we either print a local NOT
NULL constraint along with the attribute definition or as a separate
constraint in CREATE TABLE command. Non-local NOT NULL constraints
will be inherited from the parent. So there's no case where a NOT NULL
constraint would be lost. So it looks safe to remove the code to add
constraints through the ALTER TABLE command.

Attached updated patch. Once we commit this patch, I will be able to
proceed with the dump/restore test at [1].

[1] https://www.postgresql.org/message-id/flat/CAExHW5uvx2LEyrUBdctV5gS25Zeb%2B-eXESkK93siQxWSjYFy6A%40mail.gmail.com#c8ed57b77d2f6132d5b8e1ecb2a8c47b

--
Best Wishes,
Ashutosh Bapat