Re: pg17 issues with not-null contraints

On 2024-Apr-15, Justin Pryzby wrote:

> 9b581c5341 can break dump/restore from old versions, including
> pgupgrade.
>
> postgres=# CREATE TABLE iparent(id serial PRIMARY KEY); CREATE TABLE child (id int) INHERITS (iparent); ALTER TABLE child ALTER id DROP NOT NULL; ALTER TABLE child ADD CONSTRAINT p PRIMARY KEY (id);
>
> $ pg_dump -h /tmp -p 5678 postgres -Fc |pg_restore -1 -h /tmp -p 5679 -d postgres
> ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint "pgdump_throwaway_notnull_0" on relation "child"
> STATEMENT: ALTER TABLE ONLY public.iparent
> ADD CONSTRAINT iparent_pkey PRIMARY KEY (id);
>
> Strangely, if I name the table "parent", it seems to work, which might
> indicate an ordering/dependency issue.

Hmm, apparently if the table is "iparent", the primary key is created in
the child first; if the table is "parent", then the PK is created first
there. I think the problem is that the ADD CONSTRAINT for the PK should
not be recursing at all in this case ... seeing in particular that the
command specifies ONLY. Should be a simple fix, looking now.

> I think there are other issues related to b0e96f3119 (Catalog not-null
> constraints) - if I dump a v16 server using v17 tools, the backup can't
> be restored into the v16 server. I'm okay ignoring a line or two like
> 'unrecognized configuration parameter "transaction_timeout", but not
> 'syntax error at or near "NO"'.

This doesn't look something that we can handle at all. The assumption
is that pg_dump's output is going to be fed to a server that's at least
the same version. Running on older versions is just not supported.

> The other version checks in pg_dump.c are used to construct sql for
> querying the source db, but this is used to create the sql to restore
> the target, using syntax that didn't exist until v17.
>
> if (print_notnull)
> {
> if (tbinfo->notnull_constrs[j][0] == '\0')
> appendPQExpBufferStr(q, " NOT NULL");
> else
> appendPQExpBuffer(q, " CONSTRAINT %s NOT NULL",
> fmtId(tbinfo->notnull_constrs[j]));
>
> if (tbinfo->notnull_noinh[j])
> appendPQExpBufferStr(q, " NO INHERIT");
> }

If you have ideas on what to do about this, I'm all ears, but keep in
mind that pg_dump doesn't necessarily know what the target version is.

>
> This other thread is 6 years old and forgotten again, but still seems
> relevant.
> https://www.postgresql.org/message-id/flat/b8794d6a-38f0-9d7c-ad4b-e85adf860fc9%40enterprisedb.com

I only skimmed very briefly, but it looks related to commit c3709100be73
that I pushed earlier today. Or if you have some specific case that
fails to be handled please let me know. (Maybe we should have the
regress tests leave some tables behind to ensure the pg_upgrade behavior
is what we want, if we continue to break it.)

> BTW, these comments are out of date:
>
> + * In versions 16 and up, we need pg_constraint for explicit NOT NULL
> + if (fout->remoteVersion >= 170000)
>
> + * that we needn't specify that again for the child. (Versions >= 16 no
> + if (fout->remoteVersion < 170000)

Thanks, will fix. But I'm probably touching this code in the fix for
Andrew Bille's problem, so I might not do so immediately.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Estoy de acuerdo contigo en que la verdad absoluta no existe...
El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)