On Wed, Jun 18, 2025 at 11:05 PM Álvaro Herrera <alvhe...@kurilemu.de> wrote:
>
> I agree that this is roughly the right approach, but I think you're
> doing it harder than it needs to be -- it might be easier to add a JOIN
> to pg_description to the big query in getTableAttrs(), and add a pointer
> to the returned string in tbinfo->notnull_comments[j] (for versions
> earlier than 18, don't add the join and have it return constant NULL).
> Then in dumpTableSchema, in the place where you added the new query,
> just scan that array and print COMMENT ON commands for each valid
> constraint where that's not a null pointer.
>
Previously I was worried about print_notnull, shouldPrintColumn.
if there is a not-null constraint that is not dumped separately, it has comments
then we should dump these comments, then no need to worry about print_notnull.
using notnull_comments saves us one more query.
however, in determineNotNullFlags we have:
char *default_name;
/* XXX should match ChooseConstraintName better */
default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
tbinfo->attnames[j]);
if (strcmp(default_name,
PQgetvalue(res, r, i_notnull_name)) == 0)
tbinfo->notnull_constrs[j] = "";
then we can not blindly use tbinfo->notnull_constrs as the not-null
constraint name.
if tbinfo->notnull_constrs is an empty string, we need to use the above
"%s_%s_not_null" trick to get the default no-null constraint name.
From 4cce00779490001f4e40fb3055c7bddd539e1ad2 Mon Sep 17 00:00:00 2001
From: jian he <jian.universal...@gmail.com>
Date: Thu, 19 Jun 2025 10:26:44 +0800
Subject: [PATCH v2 1/1] fix pg_dump not dump comments on not-null constraints
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
dumpComment doesn't handle these inlined NOT NULL constraint comments, because
collectComments doesn't collect them—they aren't associated with a separate
dumpable object. Rather than modifying collectComments, we manually dump these
inlined not-null constraint comments within dumpTableSchema.
reported by: Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
discussion: https://postgr.es/m/d50ff977-c728-4e9e-8488-fc2688e08...@oss.nttdata.com
---
src/bin/pg_dump/pg_dump.c | 95 ++++++++++++++++++++++++++++++++++++++-
src/bin/pg_dump/pg_dump.h | 1 +
2 files changed, 95 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7bc0724cd30..dbd557b7484 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -9006,6 +9006,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
int i_notnull_name;
int i_notnull_noinherit;
int i_notnull_islocal;
+ int i_notnull_comments;
int i_notnull_invalidoid;
int i_attoptions;
int i_attcollation;
@@ -9116,6 +9117,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_comments,\n");
+ else
+ appendPQExpBufferStr(q, "NULL AS notnull_comments,\n");
+
if (fout->remoteVersion >= 140000)
appendPQExpBufferStr(q,
"a.attcompression AS attcompression,\n");
@@ -9159,12 +9165,16 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* backing a primary key.
*/
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");
-
+ appendPQExpBufferStr(q,
+ " 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");
@@ -9190,6 +9200,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_comments = PQfnumber(res, "notnull_comments");
i_attoptions = PQfnumber(res, "attoptions");
i_attcollation = PQfnumber(res, "attcollation");
i_attcompression = PQfnumber(res, "attcompression");
@@ -9258,6 +9269,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_comments = (char **) pg_malloc(numatts * sizeof(char *));
tbinfo->attrdefs = (AttrDefInfo **) pg_malloc(numatts * sizeof(AttrDefInfo *));
hasdefaults = false;
@@ -9291,6 +9303,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
i_notnull_islocal,
&invalidnotnulloids);
+ if (PQgetisnull(res, r, i_notnull_comments))
+ tbinfo->notnull_comments[j] = NULL;
+ else
+ tbinfo->notnull_comments[j] = pg_strdup(PQgetvalue(res, r, i_notnull_comments));
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));
@@ -17684,6 +17700,83 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
if (tbinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
dumpTableSecLabel(fout, tbinfo, reltypename);
+ /*
+ * Dump Table not-null constraint comments.
+ * Invalid not-null constraints are dumped separately. So we only need to
+ * handle comments for these "inlined" not-null constraints.
+ * collectComments function does not collect comments for inlined not-null
+ * constraints, as they don't have separate dumpable object associated with
+ * it. Therefore, we need to explicitly dump comments for these "inlined"
+ * not-null constraints now, since they won't be dumped separately.
+ */
+ if (!fout->dopt->no_comments &&
+ dopt->dumpSchema &&
+ fout->remoteVersion >= 180000)
+ {
+ char *qtabname;
+ const char *namespace = tbinfo->dobj.namespace->dobj.name;
+ const char *owner = tbinfo->rolname;
+ PQExpBuffer comments = createPQExpBuffer();
+ PQExpBuffer conprefix = createPQExpBuffer();
+ PQExpBuffer tag = createPQExpBuffer();
+ DumpId dumpId = tbinfo->dobj.dumpId;
+ qtabname = pg_strdup(fmtId(tbinfo->dobj.name));
+
+ for (j = 0; j < tbinfo->numatts; j++)
+ {
+ if (tbinfo->notnull_constrs[j] != NULL &&
+ tbinfo->notnull_comments[j] != NULL)
+ {
+ /*
+ * For some not-null constraints, the notnull_constrs is an
+ * empty string, because in certain cases we don't need to emit
+ * the constraint name. See determineNotNullFlags too.
+ */
+ if (tbinfo->notnull_constrs[j][0] == '\0')
+ {
+ char *default_name;
+ default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
+ tbinfo->attnames[j]);
+
+ appendPQExpBuffer(conprefix, "CONSTRAINT %s ON",
+ fmtId(default_name));
+ }
+ else
+ appendPQExpBuffer(conprefix, "CONSTRAINT %s ON",
+ fmtId(tbinfo->notnull_constrs[j]));
+
+ appendPQExpBuffer(comments, "COMMENT ON %s ", conprefix->data);
+ if (namespace && *namespace)
+ appendPQExpBuffer(comments, "%s.", fmtId(namespace));
+
+ appendPQExpBuffer(comments, "%s IS ", qtabname);
+ appendStringLiteralAH(comments, tbinfo->notnull_comments[j], fout);
+ appendPQExpBufferStr(comments, ";\n");
+
+ appendPQExpBuffer(tag, "%s %s", conprefix->data, qtabname);
+
+ /* see dumpCommentExtended also */
+ ArchiveEntry(fout, nilCatalogId, createDumpId(),
+ ARCHIVE_OPTS(.tag = tag->data,
+ .namespace = namespace,
+ .owner = owner,
+ .description = "COMMENT",
+ .section = SECTION_NONE,
+ .createStmt = comments->data,
+ .deps = &dumpId,
+ .nDeps = 1));
+
+ resetPQExpBuffer(comments);
+ resetPQExpBuffer(tag);
+ resetPQExpBuffer(conprefix);
+ }
+ }
+ destroyPQExpBuffer(conprefix);
+ destroyPQExpBuffer(comments);
+ destroyPQExpBuffer(tag);
+ free(qtabname);
+ }
+
/* 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..8092d5fddc2 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -368,6 +368,7 @@ typedef struct _tableInfo
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 */
+ char **notnull_comments; /* per-attribute not-null constraint's comments */
struct _attrDefInfo **attrdefs; /* DEFAULT expressions */
struct _constraintInfo *checkexprs; /* CHECK constraints */
struct _relStatsInfo *stats; /* only set for matviews */
--
2.34.1
