pg_dump: include comments on valid not-null constraints, too

We were missing collecting comments for not-null constraints that are
dumped inline with the table definition (i.e., valid ones), because they
aren't represented by a separately dumpable object.  Fix by creating
separate TocEntries for the comments.

Co-Authored-By: Jian He <jian.universality@gmail.com>
Co-Authored-By: Álvaro Herrera <alvherre@kurilemu.de>
Reported-By: Fujii Masao <masao.fujii@oss.nttdata.com>
Reviewed-By: Fujii Masao <masao.fujii@oss.nttdata.com>
Discussion: https://postgr.es/m/d50ff977-c728-4e9e-8488-fc2688e08754@oss.nttdata.com

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 0d9270116549a549f36ffc5e2b9149d3f0c242ef..edbb377a5eda14aec772646cc924d3a616b633e7 100644 (file)
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1281,7 +1281,7 @@ PostgreSQL documentation
           materialized views, and foriegn tables.
           Post-data items include definitions of indexes, triggers, rules,
           statistics for indexes, and constraints other than validated check
-          constraints.
+          and not-null constraints.
           Pre-data items include all other data definition items.
          </para>
        </listitem>

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index db944ec22307187bc140f76281aa89f80574ce1a..1937997ea674d23303a737605404058189280f9d 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -350,7 +350,9 @@ static void buildMatViewRefreshDependencies(Archive *fout);
 static void getTableDataFKConstraints(void);
 static void determineNotNullFlags(Archive *fout, PGresult *res, int r,
                                                                  TableInfo *tbinfo, int j,
-                                                                 int i_notnull_name, int i_notnull_invalidoid,
+                                                                 int i_notnull_name,
+                                                                 int i_notnull_comment,
+                                                                 int i_notnull_invalidoid,
                                                                  int i_notnull_noinherit,
                                                                  int i_notnull_islocal,
                                                                  PQExpBuffer *invalidnotnulloids);
@@ -9006,6 +9008,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
        int                     i_attalign;
        int                     i_attislocal;
        int                     i_notnull_name;
+       int                     i_notnull_comment;
        int                     i_notnull_noinherit;
        int                     i_notnull_islocal;
        int                     i_notnull_invalidoid;
@@ -9089,7 +9092,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 
        /*
         * Find out any NOT NULL markings for each column.  In 18 and up we read
-        * pg_constraint to obtain the constraint name.  notnull_noinherit is set
+        * pg_constraint to obtain the constraint name, and for valid constraints
+        * also pg_description to obtain its comment.  notnull_noinherit is set
         * according to the NO INHERIT property.  For versions prior to 18, we
         * store an empty string as the name when a constraint is marked as
         * attnotnull (this cues dumpTableSchema to print the NOT NULL clause
@@ -9097,7 +9101,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
         *
         * For invalid constraints, we need to store their OIDs for processing
         * elsewhere, so we bring the pg_constraint.oid value when the constraint
-        * is invalid, and NULL otherwise.
+        * is invalid, and NULL otherwise.  Their comments are handled not here
+        * but by collectComments, because they're their own dumpable object.
         *
         * We track in notnull_islocal whether the constraint was defined directly
         * in this table or via an ancestor, for binary upgrade.  flagInhAttrs
@@ -9107,6 +9112,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
        if (fout->remoteVersion >= 180000)
                appendPQExpBufferStr(q,
                                                         "co.conname AS notnull_name,\n"
+                                                        "CASE WHEN co.convalidated THEN pt.description"
+                                                        " ELSE NULL END AS notnull_comment,\n"
                                                         "CASE WHEN NOT co.convalidated THEN co.oid "
                                                         "ELSE NULL END AS notnull_invalidoid,\n"
                                                         "co.connoinherit AS notnull_noinherit,\n"
@@ -9114,6 +9121,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
        else
                appendPQExpBufferStr(q,
                                                         "CASE WHEN a.attnotnull THEN '' ELSE NULL END AS notnull_name,\n"
+                                                        "NULL AS notnull_comment,\n"
                                                         "NULL AS notnull_invalidoid,\n"
                                                         "false AS notnull_noinherit,\n"
                                                         "a.attislocal AS notnull_islocal,\n");
@@ -9157,15 +9165,16 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 
        /*
         * In versions 18 and up, we need pg_constraint for explicit NOT NULL
-        * entries.  Also, we need to know if the NOT NULL for each column is
-        * backing a primary key.
+        * entries and pg_description to get their comments.
         */
        if (fout->remoteVersion >= 180000)
                appendPQExpBufferStr(q,
                                                         " LEFT JOIN pg_catalog.pg_constraint co ON "
                                                         "(a.attrelid = co.conrelid\n"
                                                         "   AND co.contype = 'n' AND "
-                                                        "co.conkey = array[a.attnum])\n");
+                                                        "co.conkey = array[a.attnum])\n"
+                                                        " LEFT JOIN pg_catalog.pg_description pt ON "
+                                                        "(pt.classoid = co.tableoid AND pt.objoid = co.oid)\n");
 
        appendPQExpBufferStr(q,
                                                 "WHERE a.attnum > 0::pg_catalog.int2\n"
@@ -9189,6 +9198,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
        i_attalign = PQfnumber(res, "attalign");
        i_attislocal = PQfnumber(res, "attislocal");
        i_notnull_name = PQfnumber(res, "notnull_name");
+       i_notnull_comment = PQfnumber(res, "notnull_comment");
        i_notnull_invalidoid = PQfnumber(res, "notnull_invalidoid");
        i_notnull_noinherit = PQfnumber(res, "notnull_noinherit");
        i_notnull_islocal = PQfnumber(res, "notnull_islocal");
@@ -9257,6 +9267,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
                tbinfo->attfdwoptions = (char **) pg_malloc(numatts * sizeof(char *));
                tbinfo->attmissingval = (char **) pg_malloc(numatts * sizeof(char *));
                tbinfo->notnull_constrs = (char **) pg_malloc(numatts * sizeof(char *));
+               tbinfo->notnull_comment = (char **) pg_malloc(numatts * sizeof(char *));
                tbinfo->notnull_invalid = (bool *) pg_malloc(numatts * sizeof(bool));
                tbinfo->notnull_noinh = (bool *) pg_malloc(numatts * sizeof(bool));
                tbinfo->notnull_islocal = (bool *) pg_malloc(numatts * sizeof(bool));
@@ -9288,11 +9299,14 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
                        determineNotNullFlags(fout, res, r,
                                                                  tbinfo, j,
                                                                  i_notnull_name,
+                                                                 i_notnull_comment,
                                                                  i_notnull_invalidoid,
                                                                  i_notnull_noinherit,
                                                                  i_notnull_islocal,
                                                                  &invalidnotnulloids);
 
+                       tbinfo->notnull_comment[j] = PQgetisnull(res, r, i_notnull_comment) ?
+                               NULL : pg_strdup(PQgetvalue(res, r, i_notnull_comment));
                        tbinfo->attoptions[j] = pg_strdup(PQgetvalue(res, r, i_attoptions));
                        tbinfo->attcollation[j] = atooid(PQgetvalue(res, r, i_attcollation));
                        tbinfo->attcompression[j] = *(PQgetvalue(res, r, i_attcompression));
@@ -9704,8 +9718,9 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
  * 4) The column has a constraint with a known name; in that case
  *    notnull_constrs carries that name and dumpTableSchema will print
  *    "CONSTRAINT the_name NOT NULL".  However, if the name is the default
- *    (table_column_not_null), there's no need to print that name in the dump,
- *    so notnull_constrs is set to the empty string and it behaves as case 2.
+ *    (table_column_not_null) and there's no comment on the constraint,
+ *    there's no need to print that name in the dump, so notnull_constrs
+ *    is set to the empty string and it behaves as case 2.
  *
  * In a child table that inherits from a parent already containing NOT NULL
  * constraints and the columns in the child don't have their own NOT NULL
@@ -9732,6 +9747,7 @@ static void
 determineNotNullFlags(Archive *fout, PGresult *res, int r,
                                          TableInfo *tbinfo, int j,
                                          int i_notnull_name,
+                                         int i_notnull_comment,
                                          int i_notnull_invalidoid,
                                          int i_notnull_noinherit,
                                          int i_notnull_islocal,
@@ -9805,11 +9821,13 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
                {
                        /*
                         * In binary upgrade of inheritance child tables, must have a
-                        * constraint name that we can UPDATE later.
+                        * constraint name that we can UPDATE later; same if there's a
+                        * comment on the constraint.
                         */
-                       if (dopt->binary_upgrade &&
-                               !tbinfo->ispartition &&
-                               !tbinfo->notnull_islocal)
+                       if ((dopt->binary_upgrade &&
+                                !tbinfo->ispartition &&
+                                !tbinfo->notnull_islocal) ||
+                               !PQgetisnull(res, r, i_notnull_comment))
                        {
                                tbinfo->notnull_constrs[j] =
                                        pstrdup(PQgetvalue(res, r, i_notnull_name));
@@ -17686,6 +17704,56 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
        if (tbinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
                dumpTableSecLabel(fout, tbinfo, reltypename);
 
+       /*
+        * Dump comments for not-null constraints that aren't to be dumped
+        * separately (those are processed by collectComments/dumpComment).
+        */
+       if (!fout->dopt->no_comments && dopt->dumpSchema &&
+               fout->remoteVersion >= 180000)
+       {
+               PQExpBuffer comment = NULL;
+               PQExpBuffer tag = NULL;
+
+               for (j = 0; j < tbinfo->numatts; j++)
+               {
+                       if (tbinfo->notnull_constrs[j] != NULL &&
+                               tbinfo->notnull_comment[j] != NULL)
+                       {
+                               if (comment == NULL)
+                               {
+                                       comment = createPQExpBuffer();
+                                       tag = createPQExpBuffer();
+                               }
+                               else
+                               {
+                                       resetPQExpBuffer(comment);
+                                       resetPQExpBuffer(tag);
+                               }
+
+                               appendPQExpBuffer(comment, "COMMENT ON CONSTRAINT %s ON %s IS ",
+                                                                 fmtId(tbinfo->notnull_constrs[j]), qualrelname);
+                               appendStringLiteralAH(comment, tbinfo->notnull_comment[j], fout);
+                               appendPQExpBufferStr(comment, ";\n");
+
+                               appendPQExpBuffer(tag, "CONSTRAINT %s ON %s",
+                                                                 fmtId(tbinfo->notnull_constrs[j]), qrelname);
+
+                               ArchiveEntry(fout, nilCatalogId, createDumpId(),
+                                                        ARCHIVE_OPTS(.tag = tag->data,
+                                                                                 .namespace = tbinfo->dobj.namespace->dobj.name,
+                                                                                 .owner = tbinfo->rolname,
+                                                                                 .description = "COMMENT",
+                                                                                 .section = SECTION_NONE,
+                                                                                 .createStmt = comment->data,
+                                                                                 .deps = &(tbinfo->dobj.dumpId),
+                                                                                 .nDeps = 1));
+                       }
+               }
+
+               destroyPQExpBuffer(comment);
+               destroyPQExpBuffer(tag);
+       }
+
        /* Dump comments on inlined table constraints */
        for (j = 0; j < tbinfo->ncheck; j++)
        {

diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 7417eab6aefa601fd7402d7e5077893da98d04f7..39eef1d6617f4bdd0d5000a654544901e83bd8f2 100644 (file)
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -365,6 +365,7 @@ typedef struct _tableInfo
                                                                         * there isn't one on this column. If
                                                                         * empty string, unnamed constraint
                                                                         * (pre-v17) */
+       char      **notnull_comment;    /* comment thereof */
        bool       *notnull_invalid;    /* true for NOT NULL NOT VALID */
        bool       *notnull_noinh;      /* NOT NULL is NO INHERIT */
        bool       *notnull_islocal;    /* true if NOT NULL has local definition */

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 386e21e0c596a5f972836f3149f6e3adc1313dcf..e1cfa99874ec4d9c41b96ea46e9e2d412b6974bc 100644 (file)
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1191,7 +1191,9 @@ my %tests = (
                                                        ) INHERITS (dump_test.test_table_nn, dump_test.test_table_nn_2);
                        ALTER TABLE dump_test.test_table_nn ADD CONSTRAINT nn NOT NULL col1 NOT VALID;
                        ALTER TABLE dump_test.test_table_nn_chld1 VALIDATE CONSTRAINT nn;
-                       ALTER TABLE dump_test.test_table_nn_chld2 VALIDATE CONSTRAINT nn;',
+                       ALTER TABLE dump_test.test_table_nn_chld2 VALIDATE CONSTRAINT nn;
+                       COMMENT ON CONSTRAINT nn ON dump_test.test_table_nn IS \'nn comment is valid\';
+                       COMMENT ON CONSTRAINT nn ON dump_test.test_table_nn_chld2 IS \'nn_chld2 comment is valid\';',
                regexp => qr/^
                        \QALTER TABLE dump_test.test_table_nn\E \n^\s+
                        \QADD CONSTRAINT nn NOT NULL col1 NOT VALID;\E
@@ -1205,6 +1207,34 @@ my %tests = (
                },
        },
 
+       # This constraint is invalid therefore it goes in SECTION_POST_DATA
+       'COMMENT ON CONSTRAINT ON test_table_nn' => {
+               regexp => qr/^
+               \QCOMMENT ON CONSTRAINT nn ON dump_test.test_table_nn IS\E
+               /xm,
+               like => {
+                       %full_runs, %dump_test_schema_runs, section_post_data => 1,
+               },
+               unlike => {
+                       exclude_dump_test_schema => 1,
+                       only_dump_measurement => 1,
+               },
+       },
+
+       # This constraint is valid therefore it goes in SECTION_PRE_DATA
+       'COMMENT ON CONSTRAINT ON test_table_chld2' => {
+               regexp => qr/^
+               \QCOMMENT ON CONSTRAINT nn ON dump_test.test_table_nn_chld2 IS\E
+               /xm,
+               like => {
+                       %full_runs, %dump_test_schema_runs, section_pre_data => 1,
+               },
+               unlike => {
+                       exclude_dump_test_schema => 1,
+                       only_dump_measurement => 1,
+               },
+       },
+
        'CONSTRAINT NOT NULL / NOT VALID (child1)' => {
                regexp => qr/^
                \QCREATE TABLE dump_test.test_table_nn_chld1 (\E\n

diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index ad6aaab738538a1fec4c4d322a4688bdaa90f5ec..b5592617d97550d2f66161d5d783fafdb1b2cc23 100644 (file)
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -1659,6 +1659,8 @@ EXECUTE get_nnconstraint_info('{constr_parent3, constr_child3}');
  constr_parent3 | constr_parent3_a_not_null | t            | t          |           0
 (2 rows)
 
+COMMENT ON CONSTRAINT constr_parent2_a_not_null ON constr_parent2 IS 'this constraint is invalid';
+COMMENT ON CONSTRAINT constr_parent2_a_not_null ON constr_child2 IS 'this constraint is valid';
 DEALLOCATE get_nnconstraint_info;
 -- end NOT NULL NOT VALID
 -- Comments

diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 337baab7ced93723f060069a6090f885a59e820f..12668f0e0ce0f4867a2077de1d4f409c5bc350fa 100644 (file)
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -997,6 +997,9 @@ create table constr_parent3 (a int not null);
 create table constr_child3 () inherits (constr_parent2, constr_parent3);
 EXECUTE get_nnconstraint_info('{constr_parent3, constr_child3}');
 
+COMMENT ON CONSTRAINT constr_parent2_a_not_null ON constr_parent2 IS 'this constraint is invalid';
+COMMENT ON CONSTRAINT constr_parent2_a_not_null ON constr_child2 IS 'this constraint is valid';
+
 DEALLOCATE get_nnconstraint_info;
 
 -- end NOT NULL NOT VALID