Dump not-null constraints on inherited columns correctly

With not-null constraints defined in child tables for columns that are
coming from their parent tables, we were printing ALTER TABLE SET NOT
NULL commands that were missing the constraint name, so the original
constraint name was being lost, which is bogus.  Fix by instead adding
a table-constraint constraint declaration with the correct constraint
name in the CREATE TABLE instead.

Oversight in commit 14e87ffa5c54.

We could have fixed it by changing the ALTER TABLE SET NOT NULL to ALTER
TABLE ADD CONSTRAINT, but I'm not sure that's any better.  A potential
problem here might be that if sent to a non-Postgres server, the new
pg_dump output would fail because the "CONSTRAINT foo NOT NULL colname"
syntax isn't SQL-conforming.  However, Postgres' implementation of
inheritance is already non-SQL-conforming, so that'd likely fail anyway.

This problem was only noticed by Ashutosh's proposed test framework for
pg_dump, https://postgr.es/m/CAExHW5uF5V=Cjecx3_Z=7xfh4rg2Wf61PT+hfquzjBqouRzQJQ@mail.gmail.com

Author: Ashutosh Bapat <[email protected]>
Reported-by: Ashutosh Bapat <[email protected]>
Reviewed-by: Alvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/CAExHW5tbdgAKDfqjDJ-7Fk6PJtHg8D4zUF6FQ4H2Pq8zK38Nyw@mail.gmail.com

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 89276524ae011b63c74503af88e8a946131d0a3e..d34b8ed4bb12d12e380ec4dfc6ab16955dd674b8 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16220,6 +16220,38 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
                                              fmtQualifiedDumpable(coll));
                    }
                }
+
+               /*
+                * On the other hand, if we choose not to print a column
+                * (likely because it is created by inheritance), but the
+                * column has a locally-defined not-null constraint, we need
+                * to dump the constraint as a standalone object.
+                *
+                * This syntax isn't SQL-conforming, but if you wanted
+                * standard output you wouldn't be creating non-standard
+                * objects to begin with.
+                */
+               if (!shouldPrintColumn(dopt, tbinfo, j) &&
+                   !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 +16693,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