Hi Alvaro,
On Thu, Nov 28, 2024 at 4:47 PM Ashutosh Bapat
<ashutosh.bapat....@gmail.com> wrote:
>
> On Thu, Nov 28, 2024 at 4:44 PM Ashutosh Bapat
> <ashutosh.bapat....@gmail.com> wrote:
> >
> > 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.
>
> 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
From 796c726ec44f75fb02637aa2488c078f6df5149e 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 NOT NULL constraints on non-local columns
A child table is dumped as CREATE TABLE ... INHERITS command. A local NOT NULL
constraint on a non-local column is printed separately as a subsequent ALTER
TABLE ... command. When restored, this constraint inherits the name of the
corresponding parent constraint since the constraint name, if mentioned, in the
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 | 50 +++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 89276524ae0..a422421b6e7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16220,6 +16220,33 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
fmtQualifiedDumpable(coll));
}
}
+
+ /*
+ * Add local NOT NULL constraint on a non-local column (in a
+ * non-binary dump) so that the constraint's local name, if
+ * any, can be preserved along with conislocal.
+ */
+ 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]));
+
+ }
}
/*
@@ -16661,29 +16688,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
