Feedback-ID: ie3de48e3:Fastmail
Date: Mon, 14 Jul 2025 20:23:58 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: jian he <jian.universality@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_dump does not dump domain not-null constraint's comments
Message-ID: <202507141823.prfl24mpicy5@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="b2isa43e5xopowtz"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxEzGhfmBVeLGzOALZYwrje2aZT9RZeq0ZsmUEh3TLb3CQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202507141823.prfl24mpicy5%40alvherre.pgsql>
Precedence: bulk


--b2isa43e5xopowtz
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2025-May-22, jian he wrote:

> I actually found another bug.
> create schema test;
> CREATE DOMAIN test.d1 AS integer NOT NULL default 11;
> pg_dump  --schema=test > 1.sql
> ""
> pg_dump: warning: could not resolve dependency loop among these items:
> pg_dump: detail: TYPE d1  (ID 1415 OID 18007)
> pg_dump: detail: CONSTRAINT d1_not_null  (ID 1416 OID 18008)
> ""

Oh, interesting.  I agree with the rough fix, but I think it's better if
we keep the contype comparisons rather than removing them, relaxing to
allow for one more char.

I didn't like the idea of stashing the not-null constraint in the same
array as the check constraints; it feels a bit dirty (especially because
of the need to scan the array in order to find the not-null one).  I
opted to add a separate TypeInfo->notnull pointer instead.  This feels
more natural.  This works because we know a domain has only one not-null
constraint.  Note that this means we don't need your proposed 0002
anymore.

I wonder to what extent we promise ABI compatibility of pg_dump.h
structs in stable branches.  If that's an issue, we could easily use
your original patch for 17, and use the TypeInfo->notnull addition only
in 18, but I'm starting to lean on not bothering (i.e., use the same
patch in all branches).  Compare commit 7418767f1 which was backpatched
all the way and modified struct StatsExtInfo; I don't think we got any
complaints.


I also modified the Perl test so that the COMMENT ON CONSTRAINT
statement is checked in a separate stanza.  This works fine: the comment
is created in the create_sql of one stanza (the same where you had it),
then checked in the 'regexp' entry of another.  I opted to move the
regexp for both constraints out of the main one.


The attached applies on top of your patch.  Opinions?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

--b2isa43e5xopowtz
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment; filename="0001-fix-tweak.patch.txt"

From 5d29cbcee5ad62083f232db7fa0a3a7cf76455dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 14 Jul 2025 18:25:48 +0200
Subject: [PATCH] fix tweak

---
 src/bin/pg_dump/pg_dump.c        | 137 +++++++++++++++++++------------
 src/bin/pg_dump/pg_dump.h        |   4 +-
 src/bin/pg_dump/pg_dump_sort.c   |  10 ++-
 src/bin/pg_dump/t/002_pg_dump.pl |  26 +++++-
 4 files changed, 119 insertions(+), 58 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7849770fdee..2253f772fcc 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -47,6 +47,7 @@
 #include "catalog/pg_authid_d.h"
 #include "catalog/pg_cast_d.h"
 #include "catalog/pg_class_d.h"
+#include "catalog/pg_constraint_d.h"
 #include "catalog/pg_default_acl_d.h"
 #include "catalog/pg_largeobject_d.h"
 #include "catalog/pg_proc_d.h"
@@ -6122,6 +6123,7 @@ getTypes(Archive *fout)
 		 */
 		tyinfo[i].nDomChecks = 0;
 		tyinfo[i].domChecks = NULL;
+		tyinfo[i].notnull = NULL;
 		if ((tyinfo[i].dobj.dump & DUMP_COMPONENT_DEFINITION) &&
 			tyinfo[i].typtype == TYPTYPE_DOMAIN)
 			getDomainConstraints(fout, &(tyinfo[i]));
@@ -8247,7 +8249,6 @@ addConstrChildIdxDeps(DumpableObject *dobj, const IndxInfo *refidx)
 static void
 getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
 {
-	int			i;
 	ConstraintInfo *constrinfo;
 	PQExpBuffer query = createPQExpBuffer();
 	PGresult   *res;
@@ -8255,29 +8256,26 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
 				i_oid,
 				i_conname,
 				i_consrc,
+				i_convalidated,
 				i_contype;
 	int			ntups;
 
 	if (!fout->is_prepared[PREPQUERY_GETDOMAINCONSTRAINTS])
 	{
-		/* Set up query for constraint-specific details */
-		appendPQExpBufferStr(query,
-							 "PREPARE getDomainConstraints(pg_catalog.oid) AS\n"
-							 "SELECT tableoid, oid, conname, "
-							 "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
-							 "convalidated, "
-							 "contype "
-							 "FROM pg_catalog.pg_constraint "
-							 "WHERE contypid = $1 ");
-
 		/*
-		 * only PG17 or later versions, not-null domain constraint catalog
-		 * information is stored in the pg_constraint.
+		 * Set up query for constraint-specific details.  For servers 17 and
+		 * up, domains have constraints of type 'n' as well as 'c', otherwise
+		 * just the latter.
 		 */
-		if (fout->remoteVersion < 170000)
-			appendPQExpBufferStr(query, "AND contype = 'c' ");
-
-		appendPQExpBufferStr(query, "ORDER BY conname");
+		appendPQExpBuffer(query,
+						  "PREPARE getDomainConstraints(pg_catalog.oid) AS\n"
+						  "SELECT tableoid, oid, conname, "
+						  "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
+						  "convalidated, contype "
+						  "FROM pg_catalog.pg_constraint "
+						  "WHERE contypid = $1 AND contype IN (%s) "
+						  "ORDER BY conname",
+						  fout->remoteVersion < 170000 ? "'c'" : "'c', 'n'");
 
 		ExecuteSqlStatement(fout, query->data);
 
@@ -8296,34 +8294,49 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
 	i_oid = PQfnumber(res, "oid");
 	i_conname = PQfnumber(res, "conname");
 	i_consrc = PQfnumber(res, "consrc");
+	i_convalidated = PQfnumber(res, "convalidated");
 	i_contype = PQfnumber(res, "contype");
 
 	constrinfo = (ConstraintInfo *) pg_malloc(ntups * sizeof(ConstraintInfo));
-
-	tyinfo->nDomChecks = ntups;
 	tyinfo->domChecks = constrinfo;
 
-	for (i = 0; i < ntups; i++)
+	for (int i = 0, j = 0; i < ntups; i++)
 	{
-		bool		validated = PQgetvalue(res, i, 4)[0] == 't';
+		bool		validated = PQgetvalue(res, i, i_convalidated)[0] == 't';
+		char		contype = (PQgetvalue(res, i, i_contype))[0];
+		ConstraintInfo *constraint;
 
-		constrinfo[i].dobj.objType = DO_CONSTRAINT;
-		constrinfo[i].dobj.catId.tableoid = atooid(PQgetvalue(res, i, i_tableoid));
-		constrinfo[i].dobj.catId.oid = atooid(PQgetvalue(res, i, i_oid));
-		AssignDumpId(&constrinfo[i].dobj);
-		constrinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_conname));
-		constrinfo[i].dobj.namespace = tyinfo->dobj.namespace;
-		constrinfo[i].contable = NULL;
-		constrinfo[i].condomain = tyinfo;
-		constrinfo[i].contype = *(PQgetvalue(res, i, i_contype));
-		constrinfo[i].condef = pg_strdup(PQgetvalue(res, i, i_consrc));
-		constrinfo[i].confrelid = InvalidOid;
-		constrinfo[i].conindex = 0;
-		constrinfo[i].condeferrable = false;
-		constrinfo[i].condeferred = false;
-		constrinfo[i].conislocal = true;
+		if (contype == CONSTRAINT_CHECK)
+		{
+			constraint = &constrinfo[j++];
+			tyinfo->nDomChecks++;
+		}
+		else
+		{
+			Assert(contype == CONSTRAINT_NOTNULL);
+			Assert(tyinfo->notnull == NULL);
+			/* use last item in array for the not-null constraint */
+			tyinfo->notnull = &(constrinfo[ntups - 1]);
+			constraint = tyinfo->notnull;
+		}
 
-		constrinfo[i].separate = !validated;
+		constraint->dobj.objType = DO_CONSTRAINT;
+		constraint->dobj.catId.tableoid = atooid(PQgetvalue(res, i, i_tableoid));
+		constraint->dobj.catId.oid = atooid(PQgetvalue(res, i, i_oid));
+		AssignDumpId(&(constraint->dobj));
+		constraint->dobj.name = pg_strdup(PQgetvalue(res, i, i_conname));
+		constraint->dobj.namespace = tyinfo->dobj.namespace;
+		constraint->contable = NULL;
+		constraint->condomain = tyinfo;
+		constraint->contype = *(PQgetvalue(res, i, i_contype));
+		constraint->condef = pg_strdup(PQgetvalue(res, i, i_consrc));
+		constraint->confrelid = InvalidOid;
+		constraint->conindex = 0;
+		constraint->condeferrable = false;
+		constraint->condeferred = false;
+		constraint->conislocal = true;
+
+		constraint->separate = !validated;
 
 		/*
 		 * Make the domain depend on the constraint, ensuring it won't be
@@ -8332,8 +8345,7 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
 		 * anyway, so this doesn't matter.
 		 */
 		if (validated)
-			addObjectDependency(&tyinfo->dobj,
-								constrinfo[i].dobj.dumpId);
+			addObjectDependency(&tyinfo->dobj, constraint->dobj.dumpId);
 	}
 
 	PQclear(res);
@@ -12528,29 +12540,32 @@ dumpDomain(Archive *fout, const TypeInfo *tyinfo)
 			appendPQExpBuffer(q, " COLLATE %s", fmtQualifiedDumpable(coll));
 	}
 
+	/*
+	 * Print a not-null constraint if there's one.  In servers older than 17
+	 * these don't have names, so just print it unadorned; in newer ones they
+	 * do, but most of the time it's going to be the standard generated one,
+	 * so omit the name in that case also.
+	 */
 	if (typnotnull[0] == 't')
 	{
 		if (fout->remoteVersion < 170000)
 			appendPQExpBufferStr(q, " NOT NULL");
 		else
 		{
-			for (i = 0; i < tyinfo->nDomChecks; i++)
+			ConstraintInfo *notnull = tyinfo->notnull;
+
+			if (!notnull->separate)
 			{
-				ConstraintInfo *domcheck = &(tyinfo->domChecks[i]);
+				char	   *default_name;
 
-				if (!domcheck->separate && domcheck->contype == 'n')
-				{
-					char	   *default_name;
+				/* XXX should match ChooseConstraintName better */
+				default_name = psprintf("%s_not_null", qtypname);
 
-					/* XXX should match ChooseConstraintName better */
-					default_name = psprintf("%s_not_null", qtypname);
-
-					if (strcmp(default_name, fmtId(domcheck->dobj.name)) == 0)
-						appendPQExpBufferStr(q, " NOT NULL");
-					else
-						appendPQExpBuffer(q, " CONSTRAINT %s %s",
-										fmtId(domcheck->dobj.name), domcheck->condef);
-				}
+				if (strcmp(default_name, fmtId(notnull->dobj.name)) == 0)
+					appendPQExpBufferStr(q, " NOT NULL");
+				else
+					appendPQExpBuffer(q, " CONSTRAINT %s %s",
+									  fmtId(notnull->dobj.name), notnull->condef);
 			}
 		}
 	}
@@ -12632,6 +12647,22 @@ dumpDomain(Archive *fout, const TypeInfo *tyinfo)
 		destroyPQExpBuffer(conprefix);
 	}
 
+	/* And a comment on the not-null constraint, if there's one */
+	if (tyinfo->notnull != NULL)
+	{
+		PQExpBuffer conprefix = createPQExpBuffer();
+
+		appendPQExpBuffer(conprefix, "CONSTRAINT %s ON DOMAIN",
+						  fmtId(tyinfo->notnull->dobj.name));
+
+		if (tyinfo->notnull->dobj.dump & DUMP_COMPONENT_COMMENT)
+			dumpComment(fout, conprefix->data, qtypname,
+						tyinfo->dobj.namespace->dobj.name,
+						tyinfo->rolname,
+						tyinfo->notnull->dobj.catId, 0, tyinfo->dobj.dumpId);
+		destroyPQExpBuffer(conprefix);
+	}
+
 	destroyPQExpBuffer(q);
 	destroyPQExpBuffer(delq);
 	destroyPQExpBuffer(query);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 39eef1d6617..2370c98d192 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -222,7 +222,9 @@ typedef struct _typeInfo
 	bool		isDefined;		/* true if typisdefined */
 	/* If needed, we'll create a "shell type" entry for it; link that here: */
 	struct _shellTypeInfo *shellType;	/* shell-type entry, or NULL */
-	/* If it's a domain, we store links to its constraints here: */
+	/* If it's a domain, its not-null constraint is here: */
+	struct _constraintInfo *notnull;
+	/* If it's a domain, we store links to its CHECK constraints here: */
 	int			nDomChecks;
 	struct _constraintInfo *domChecks;
 } TypeInfo;
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index d3eec6b2598..7b42f4882cc 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -1173,10 +1173,12 @@ repairDependencyLoop(DumpableObject **loop,
 		}
 	}
 
-	/* Domain and CHECK, NOT NULL constraint */
+	/* Domain and CHECK or NOT NULL constraint */
 	if (nLoop == 2 &&
 		loop[0]->objType == DO_TYPE &&
 		loop[1]->objType == DO_CONSTRAINT &&
+		(((ConstraintInfo *) loop[1])->contype == 'c' ||
+		 ((ConstraintInfo *) loop[1])->contype == 'n') &&
 		((ConstraintInfo *) loop[1])->condomain == (TypeInfo *) loop[0])
 	{
 		repairDomainConstraintLoop(loop[0], loop[1]);
@@ -1185,13 +1187,15 @@ repairDependencyLoop(DumpableObject **loop,
 	if (nLoop == 2 &&
 		loop[1]->objType == DO_TYPE &&
 		loop[0]->objType == DO_CONSTRAINT &&
+		(((ConstraintInfo *) loop[1])->contype == 'c' ||
+		 ((ConstraintInfo *) loop[1])->contype == 'n') &&
 		((ConstraintInfo *) loop[0])->condomain == (TypeInfo *) loop[1])
 	{
 		repairDomainConstraintLoop(loop[1], loop[0]);
 		return;
 	}
 
-	/* Indirect loop involving domain and CHECK OR NOT NULL constraint */
+	/* Indirect loop involving domain and CHECK or NOT NULL constraint */
 	if (nLoop > 2)
 	{
 		for (i = 0; i < nLoop; i++)
@@ -1201,6 +1205,8 @@ repairDependencyLoop(DumpableObject **loop,
 				for (j = 0; j < nLoop; j++)
 				{
 					if (loop[j]->objType == DO_CONSTRAINT &&
+						(((ConstraintInfo *) loop[1])->contype == 'c' ||
+						 ((ConstraintInfo *) loop[1])->contype == 'n') &&
 						((ConstraintInfo *) loop[j])->condomain == (TypeInfo *) loop[i])
 					{
 						repairDomainConstraintMultiLoop(loop[i], loop[j]);
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 90d1067f3e2..771cdcecb60 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2390,8 +2390,6 @@ my %tests = (
 			\Q(((VALUE ~ '^\d{5}\E
 			\$\Q'::text) OR (VALUE ~ '^\d{5}-\d{4}\E\$
 			\Q'::text)));\E(.|\n)*
-			\QCOMMENT ON CONSTRAINT nn ON DOMAIN dump_test.us_postal_code IS 'not null';\E(.|\n)*
-			\QCOMMENT ON CONSTRAINT us_postal_code_check ON DOMAIN dump_test.us_postal_code IS 'check it';\E
 			/xm,
 		like =>
 		  { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
@@ -2401,6 +2399,30 @@ my %tests = (
 		},
 	},
 
+	'COMMENT ON CONSTRAINT ON DOMAIN (1)' => {
+		regexp => qr/^
+		\QCOMMENT ON CONSTRAINT nn ON DOMAIN dump_test.us_postal_code IS 'not null';\E
+		/xm,
+		like =>
+		  { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+		unlike => {
+			exclude_dump_test_schema => 1,
+			only_dump_measurement => 1,
+		},
+	},
+
+	'COMMENT ON CONSTRAINT ON DOMAIN (2)' => {
+		regexp => qr/^
+		\QCOMMENT ON CONSTRAINT us_postal_code_check ON DOMAIN dump_test.us_postal_code IS 'check it';\E
+		/xm,
+		like =>
+		  { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+		unlike => {
+			exclude_dump_test_schema => 1,
+			only_dump_measurement => 1,
+		},
+	},
+
 	'CREATE FUNCTION dump_test.pltestlang_call_handler' => {
 		create_order => 17,
 		create_sql => 'CREATE FUNCTION dump_test.pltestlang_call_handler()
-- 
2.39.5


--b2isa43e5xopowtz--