On Wed, Nov 27, 2024 at 7:04 PM Alvaro Herrera <alvhe...@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. -- Best Wishes, Ashutosh Bapat
From 92be7b6f6306045f0cf84dad2625e5fd9cb16101 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <ashutosh.bapat....@gmail.com> Date: Thu, 28 Nov 2024 16:21:42 +0530 Subject: [PATCH] Dumping local and inherited NOT NULL constraints on non-local columns A child table is dumped as CREATE TABLE ... INHERITS. If there is any local NOT NULL constraint on a column not included in CREATE TABLE command, it is printed separately as an ALTER TABLE ... command. When restored this constraint gets the name of the corresponding parent constraint since CREATE TABLE is executed first and constraint name, if mentioned, in ALTER TABLE command is ignored. We end up losing the name of child constraint in the restored database. Instead dump them as part of the CREATE TABLE command itself so that their given or default name is preserved. Author: Ashutosh Bapat Reviewed-by: Alvaro Herrera Discussion: https://www.postgresql.org/message-id/flat/CAExHW5tbdgAKDfqjDJ-7Fk6PJtHg8D4zUF6FQ4H2Pq8zK38Nyw%40mail.gmail.com --- src/bin/pg_dump/pg_dump.c | 45 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index add7f16c902..2a4ff592fab 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -16221,6 +16221,28 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) fmtQualifiedDumpable(coll)); } } + /* Add local NOT NULL constraint on columns not printed above. */ + else if (!tbinfo->attisdropped[j] && + tbinfo->notnull_constrs[j] != NULL && + tbinfo->notnull_islocal[j]) + { + /* Format properly if not first attr */ + if (actual_atts == 0) + appendPQExpBufferStr(q, " ("); + else + appendPQExpBufferChar(q, ','); + appendPQExpBufferStr(q, "\n "); + actual_atts++; + + if (tbinfo->notnull_constrs[j][0] == '\0') + appendPQExpBuffer(q, "NOT NULL %s", + fmtId(tbinfo->attnames[j])); + else + appendPQExpBuffer(q, "CONSTRAINT %s NOT NULL %s", + tbinfo->notnull_constrs[j], + fmtId(tbinfo->attnames[j])); + + } } /* @@ -16662,29 +16684,6 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) if (tbinfo->attisdropped[j]) continue; - /* - * 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)) - { - /* No constraint name desired? */ - if (tbinfo->notnull_constrs[j][0] == '\0') - appendPQExpBuffer(q, - "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET NOT NULL;\n", - foreign, qualrelname, - fmtId(tbinfo->attnames[j])); - else - appendPQExpBuffer(q, - "ALTER %sTABLE ONLY %s ADD CONSTRAINT %s NOT NULL %s;\n", - foreign, qualrelname, - tbinfo->notnull_constrs[j], - fmtId(tbinfo->attnames[j])); - } - /* * Dump per-column statistics information. We only issue an ALTER * TABLE statement if the attstattarget entry for this column is -- 2.34.1
