On 2025-Jun-25, Álvaro Herrera wrote: > Ah, thanks for the test case. Yeah, I removed one 'if' condition too > many from Jian's patch. We just need to test the constraint name for > nullness and then things seem to work:
One more thing was missing, which I noticed as I added the tests. Apparently the COMMENT objects can end up in any section of the dump, but for comments on valid constraints, this is not what we want -- they should go into the PRE_DATA section only. When running the tests with the code still putting them in SECTION_NONE, apparently pg_dump would randomly put them either here or there, so the tests would fail intermittently. Or at least that's what I think happened. Once I made them be in SECTION_PRE_DATA, that problem disappeared. Anyway, here's what I intend to push tomorrow, unless further problems are found. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "No nos atrevemos a muchas cosas porque son difíciles, pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)
>From 7a4cc26d12da930049a6810b77b35b38deb80e37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@kurilemu.de> Date: Wed, 25 Jun 2025 20:18:31 +0200 Subject: [PATCH v4] 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, because they aren't represented by a separately dumpable object. Fix by creating separate TocEntries for them. Author: Jian He <jian.universal...@gmail.com> Reported-By: Fujii Masao <masao.fu...@oss.nttdata.com> Discussion: https://postgr.es/m/d50ff977-c728-4e9e-8488-fc2688e08...@oss.nttdata.com --- src/bin/pg_dump/pg_dump.c | 97 +++++++++++++++++++++++++++++--- src/bin/pg_dump/pg_dump.h | 1 + src/bin/pg_dump/t/002_pg_dump.pl | 32 ++++++++++- 3 files changed, 120 insertions(+), 10 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index db944ec2230..99969428f9d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -353,6 +353,7 @@ static void determineNotNullFlags(Archive *fout, PGresult *res, int r, int i_notnull_name, int i_notnull_invalidoid, int i_notnull_noinherit, int i_notnull_islocal, + int i_notnull_comment, PQExpBuffer *invalidnotnulloids); static char *format_function_arguments(const FuncInfo *finfo, const char *funcargs, bool is_agg); @@ -9008,6 +9009,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) int i_notnull_name; int i_notnull_noinherit; int i_notnull_islocal; + int i_notnull_comment; int i_notnull_invalidoid; int i_attoptions; int i_attcollation; @@ -9118,6 +9120,11 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) "false AS notnull_noinherit,\n" "a.attislocal AS notnull_islocal,\n"); + if (fout->remoteVersion >= 180000) + appendPQExpBufferStr(q, "pt.description AS notnull_comment,\n"); + else + appendPQExpBufferStr(q, "NULL AS notnull_comment,\n"); + if (fout->remoteVersion >= 140000) appendPQExpBufferStr(q, "a.attcompression AS attcompression,\n"); @@ -9158,15 +9165,19 @@ 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. + * backing a primary key. Lastly, we join to 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" "ORDER BY a.attrelid, a.attnum"); @@ -9192,6 +9203,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) i_notnull_invalidoid = PQfnumber(res, "notnull_invalidoid"); i_notnull_noinherit = PQfnumber(res, "notnull_noinherit"); i_notnull_islocal = PQfnumber(res, "notnull_islocal"); + i_notnull_comment = PQfnumber(res, "notnull_comment"); i_attoptions = PQfnumber(res, "attoptions"); i_attcollation = PQfnumber(res, "attcollation"); i_attcompression = PQfnumber(res, "attcompression"); @@ -9260,6 +9272,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) 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)); + tbinfo->notnull_comment = (char **) pg_malloc(numatts * sizeof(char *)); tbinfo->attrdefs = (AttrDefInfo **) pg_malloc(numatts * sizeof(AttrDefInfo *)); hasdefaults = false; @@ -9291,8 +9304,11 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) i_notnull_invalidoid, i_notnull_noinherit, i_notnull_islocal, + i_notnull_comment, &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 +9720,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 @@ -9735,6 +9752,7 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r, int i_notnull_invalidoid, int i_notnull_noinherit, int i_notnull_islocal, + int i_notnull_comment, PQExpBuffer *invalidnotnulloids) { DumpOptions *dopt = fout->dopt; @@ -9805,11 +9823,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 +17706,65 @@ 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 dumped by collectComments). + */ + if (!fout->dopt->no_comments && dopt->dumpSchema && + fout->remoteVersion >= 180000) + { + const char *owner = tbinfo->rolname; + 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 ", + fmtId(tbinfo->notnull_constrs[j])); + appendPQExpBuffer(comment, "ON %s IS ", qualrelname); + appendStringLiteralAH(comment, tbinfo->notnull_comment[j], fout); + appendPQExpBufferStr(comment, ";\n"); + + appendPQExpBuffer(tag, "CONSTRAINT %s ON %s", + fmtId(tbinfo->notnull_constrs[j]), qualrelname); + + /* + * These entries exist only for (valid) constraints that are + * dumped together with the table definition, so they're all + * in SECTION_PRE_DATA. + */ + ArchiveEntry(fout, nilCatalogId, createDumpId(), + ARCHIVE_OPTS(.tag = tag->data, + .namespace = tbinfo->dobj.namespace->dobj.name, + .owner = owner, + .description = "COMMENT", + .section = SECTION_PRE_DATA, + .createStmt = comment->data, + .deps = &(tbinfo->dobj.dumpId), + .nDeps = 1)); + } + } + if (comment != NULL) + { + 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 7417eab6aef..39eef1d6617 100644 --- 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 386e21e0c59..e1cfa99874e 100644 --- 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 -- 2.39.5
