pg_dump does not dump domain not-null constraint's comments

On 2025-May-07, jian he wrote:

in PG17 and master, pg_dump (--schema=test --no-owner)
[...]
didn't produce
COMMENT ON CONSTRAINT nn ON DOMAIN test.d1 IS 'not null constraint on
domain d1';
we should make pg_dump to produce it too?

Yes, this is clearly a pg17 bug whose fix should be backpatched.

The attached patch tries to make it produce comments on not-null
constraints on domains.

Thanks, I'll have a look.

I aslo renamed struct TypeInfo fields, nDomChecks will be renamed to
nDomConstrs; domChecks will be renamed to domConstrs.
TypeInfo->domConstrs will also include not-null constraint
information, changing from domChecks to domConstrs makes sense, IMHO.

Hmm, for a backpatch I would leave the field names alone since they are
publicly visible; we can rename separately in pg19 once it opens. Can
you resubmit splitting the renaming out to a 0002 patch?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle." (Larry Wall, Apocalypse 6)

On Wed, May 7, 2025 at 5:25 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-May-07, jian he wrote:

in PG17 and master, pg_dump (--schema=test --no-owner)
[...]
didn't produce
COMMENT ON CONSTRAINT nn ON DOMAIN test.d1 IS 'not null constraint on
domain d1';
we should make pg_dump to produce it too?

Yes, this is clearly a pg17 bug whose fix should be backpatched.

The attached patch tries to make it produce comments on not-null
constraints on domains.

Thanks, I'll have a look.

I aslo renamed struct TypeInfo fields, nDomChecks will be renamed to
nDomConstrs; domChecks will be renamed to domConstrs.
TypeInfo->domConstrs will also include not-null constraint
information, changing from domChecks to domConstrs makes sense, IMHO.

Hmm, for a backpatch I would leave the field names alone since they are
publicly visible; we can rename separately in pg19 once it opens. Can
you resubmit splitting the renaming out to a 0002 patch?

--

hi.

i didn't fully understand pg_dump perl dump test, I have added some changes on
src/bin/pg_dump/t/002_pg_dump.pl, but it fails the tests....

v2-0001 ensures pg_dump dump domain not-null constraint's comments
v2-0002 change some variable/argument/struct element name because of v2-0001.

Ooh, you know what? I ran your the CREATE DOMAIN statement in your new
test and pg_dump'ed that, and as far as I can tell the _name_ of the
not-null constraint is not dumped either. So it's not just the comment
that we're losing. That's a separate bug, and probably needs to be
fixed first, although I'm not sure if that one is going to be
back-patchable. Which means, I withdraw my earlier assessment that the
comment fix is back-patchable as a whole.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On Wed, May 7, 2025 at 10:56 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Ooh, you know what? I ran your the CREATE DOMAIN statement in your new
test and pg_dump'ed that, and as far as I can tell the _name_ of the
not-null constraint is not dumped either. So it's not just the comment
that we're losing. That's a separate bug, and probably needs to be
fixed first, although I'm not sure if that one is going to be
back-patchable. Which means, I withdraw my earlier assessment that the
comment fix is back-patchable as a whole.

for PG17. we change the dump of
create domain test.d4 as int constraint nn not null;
from
CREATE DOMAIN test.d4 AS integer NOT NULL;
to
CREATE DOMAIN test.d4 AS integer CONSTRAINT nn NOT NULL;

in PG17 we didn't preserve the constraint name, now if we did, then
it's a bug fix,
so it can be back-patchable?

Anyway, the attachment is for PG18 only now, it will fix the domain constraint
name loss and domain not-null comments loss issue together.

hi.

"Catalog domain not-null constraints" commit[1]https://git.postgresql.org/cgit/postgresql.git/commit/?id=e5da0fe3c22b34c4433f1729e88495554b5331ed
doesn't have a pg_dump test.

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)
""

Catalog domain not-null constraints is committed in 17, so no need to
worry about before 17 branches.

The attached patch will work for PG17 and PG18.

You can use the following SQL examples
to compare with master and see the intended effect of my attached patch.

CREATE DOMAIN test.d1 AS integer NOT NULL default 11;
CREATE DOMAIN test.d3 AS integer constraint nn NOT NULL default 11;

[1]: https://git.postgresql.org/cgit/postgresql.git/commit/?id=e5da0fe3c22b34c4433f1729e88495554b5331ed

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/

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:

I wonder to what extent we promise ABI compatibility of pg_dump.h
structs in stable branches.

Those should be private to pg_dump, so I see no objection to changing
them.

regards, tom lane

On Tue, Jul 15, 2025 at 2:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

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.

TypeInfo->notnull is much better than
TypeInfo->domChecks handle both check and not-null constraints.

The attached applies on top of your patch. Opinions?

+ constraint->contype = *(PQgetvalue(res, i, i_contype));
can change to
constraint->contype = contype;

in getDomainConstraints, we use pg_malloc
constrinfo = (ConstraintInfo *) pg_malloc(ntups * sizeof(ConstraintInfo));
after
constraint->condeferred = false;
would better also add
constraint->conperiod = false;

accidently found another existing bug.

create schema test;
CREATE DOMAIN test.d1 AS integer NOT NULL default 11;
alter domain test.d1 add constraint a2 check(value > 1) not valid;
comment on CONSTRAINT a2 ON DOMAIN test.d1 is 'test';
dump output is:

CREATE SCHEMA test;
CREATE DOMAIN test.d1 AS integer NOT NULL DEFAULT 11;
COMMENT ON CONSTRAINT a2 ON DOMAIN test.d1 IS 'test';
ALTER DOMAIN test.d1
ADD CONSTRAINT a2 CHECK ((VALUE > 1)) NOT VALID;

Obviously the COMMENT command will error out.
currently working on a fix, just sharing the bug details in advance.

On Tue, Jul 15, 2025 at 2:10 PM jian he <jian.universality@gmail.com> wrote:

accidently found another existing bug.

create schema test;
CREATE DOMAIN test.d1 AS integer NOT NULL default 11;
alter domain test.d1 add constraint a2 check(value > 1) not valid;
comment on CONSTRAINT a2 ON DOMAIN test.d1 is 'test';
dump output is:

CREATE SCHEMA test;
CREATE DOMAIN test.d1 AS integer NOT NULL DEFAULT 11;
COMMENT ON CONSTRAINT a2 ON DOMAIN test.d1 IS 'test';
ALTER DOMAIN test.d1
ADD CONSTRAINT a2 CHECK ((VALUE > 1)) NOT VALID;

Obviously the COMMENT command will error out.
currently working on a fix, just sharing the bug details in advance.

we should let:
dumpConstraint handles dumping separate "NOT VALID" domain constraints along
with their comments.
dumpDomain: handles dumping "inlined" valid (not separate) domain constraints
together with their comments.

tested locally, i didn't write the test on src/bin/pg_dump/t/002_pg_dump.pl....

v5-0001-fix-pg_dump-domain-not-valid-constraint-comments.no-cfbot
was based on
v4-0001-Improve-pg_dump-handling-of-domain-not-null-constraints.patch
and 0001-fix-tweak.patch.txt.

On 2025-Jul-15, jian he wrote:

On Tue, Jul 15, 2025 at 2:10 PM jian he <jian.universality@gmail.com> wrote:

accidently found another existing bug.

CREATE SCHEMA test;
CREATE DOMAIN test.d1 AS integer NOT NULL DEFAULT 11;
COMMENT ON CONSTRAINT a2 ON DOMAIN test.d1 IS 'test';
ALTER DOMAIN test.d1
ADD CONSTRAINT a2 CHECK ((VALUE > 1)) NOT VALID;

Obviously the COMMENT command will error out.
currently working on a fix, just sharing the bug details in advance.

Ouch, ugh. I think this is as old as NOT VALID constraints on domains,
maybe from commit 897795240cfa (Jun 2011), or ... no, actually
7eca575d1c28 (Dec 2014) which enabled constraints on domains to have
comments. In either case it's my fault, and the fix needs to be
backpatched all the way back, so I'm going to apply this bugfix one
ahead of the others, which are for newer bugs.

we should let:
dumpConstraint handles dumping separate "NOT VALID" domain constraints along
with their comments.
dumpDomain: handles dumping "inlined" valid (not separate) domain constraints
together with their comments.

Yeah, this makes sense.

tested locally, i didn't write the test on src/bin/pg_dump/t/002_pg_dump.pl....

I'll add something for coverage, but not yet sure if it's going to be
something in 002_pg_dump.pl, or objects to be left around for the
pg_upgrade test to pick up.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On 2025-Jul-15, jian he wrote:

we should let:
dumpConstraint handles dumping separate "NOT VALID" domain constraints along
with their comments.
dumpDomain: handles dumping "inlined" valid (not separate) domain constraints
together with their comments.

Hmm, okay, but not-null constraints on domain cannot be NOT VALID at
present. I think it would be a useful feature (so that users can make
domains not-null that are used in tables already populated with data),
but first you'd need to implement things so that each table has a
not-null constraint row for the column whose type is the domain that's
being marked not-null.

Anyway, here's a patch.

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

On Fri, Jul 18, 2025 at 5:11 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Anyway, here's a patch.

one minor issue in getDomainConstraints:

for (int i = 0, j = 0; i < ntups; i++)
{
char contype = (PQgetvalue(res, i, i_contype))[0];
....
constraint->contype = *(PQgetvalue(res, i, i_contype));
}

for the same code simplicity,
``constraint->contype = *(PQgetvalue(res, i, i_contype));``
can change to
`` constraint->contype = contype; ``
?
other than that, looks good to me.

On 2025-Jul-18, jian he wrote:

one minor issue in getDomainConstraints:

for (int i = 0, j = 0; i < ntups; i++)
{
char contype = (PQgetvalue(res, i, i_contype))[0];
....
constraint->contype = *(PQgetvalue(res, i, i_contype));
}

for the same code simplicity,
``constraint->contype = *(PQgetvalue(res, i, i_contype));``
can change to
`` constraint->contype = contype; ``
?
other than that, looks good to me.

Thanks, changed it that way.

I noticed some other issues however. First, you had removed the contype
comparisons in repairDependencyLoop; I put them back several versions of
the patch ago, but I had mistakenly made them reference the wrong array
item -- in all three places. Doh.

Second, the name comparisons to determine whether to list the constraint
name in the "CREATE DOMAIN .. NOT NULL" syntax was wrong: it was using
fmtId() around the constraint name, but of course that would have never
worked, because we're comparing to the original string, not a quoted
name. So if you had a domain called, say
CREATE DOMAIN "my domain" AS int NOT NULL;

then pg_dump would have said
CREATE DOMAIN "my domain" AS int CONSTRAINT "my domain_not_null" NOT NULL;

even though listing the name is unnecessary. This wouldn't have made
the dump fail, so it's borderline; but it would be /slightly/ ugly.

Lastly, I made dumpDomain print unadorned "NOT NULL" if a pg_constraint
row for the constraint is not found and yet we observe typnotnull set.
This can be considered catalog corruption (the row should be there), but
I think this is better than having pg_dump just crash if that row is not
there.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)

On Tue, Jul 15, 2025 at 03:40:38PM +0200, �lvaro Herrera wrote:

On 2025-Jul-15, jian he wrote:

we should let:
dumpConstraint handles dumping separate "NOT VALID" domain constraints along
with their comments.
dumpDomain: handles dumping "inlined" valid (not separate) domain constraints
together with their comments.

This became commit 0858f0f.

@@ -18487,8 +18493,25 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
.description = "CHECK CONSTRAINT",
.section = SECTION_POST_DATA,
.createStmt = q->data,
.dropStmt = delq->data));
+
+			if (coninfo->dobj.dump & DUMP_COMPONENT_COMMENT)
+			{
+				char	   *qtypname;
+
+				PQExpBuffer conprefix = createPQExpBuffer();
+				qtypname = pg_strdup(fmtId(tyinfo->dobj.name));
+
+				appendPQExpBuffer(conprefix, "CONSTRAINT %s ON DOMAIN",
+								  fmtId(coninfo->dobj.name));
+
+				dumpComment(fout, conprefix->data, qtypname,
+							tyinfo->dobj.namespace->dobj.name,
+							tyinfo->rolname,
+							coninfo->dobj.catId, 0, tyinfo->dobj.dumpId);

The last argument gives the dump object on which the comment has a dependency.
Since this is the case of a separately-dumped constraint, the comment needs to
depend on that constraint (coninfo), not on the domain (tyinfo):

-							coninfo->dobj.catId, 0, tyinfo->dobj.dumpId);
+							coninfo->dobj.catId, 0, coninfo->dobj.dumpId);

I didn't encounter a failure from this, but sufficient restore parallelism
might reach a failure. A failure would look like a "does not exist" error in
the COMMENT command, due to the constraint not yet existing.
dumpTableConstraintComment() is an older case that optimally handles the last
dumpComment() arg.

In the absence of objections, I'll make it so.

On 2025-Sep-12, Noah Misch wrote:

The last argument gives the dump object on which the comment has a dependency.
Since this is the case of a separately-dumped constraint, the comment needs to
depend on that constraint (coninfo), not on the domain (tyinfo):

-							coninfo->dobj.catId, 0, tyinfo->dobj.dumpId);
+							coninfo->dobj.catId, 0, coninfo->dobj.dumpId);

I didn't encounter a failure from this, but sufficient restore parallelism
might reach a failure. A failure would look like a "does not exist" error in
the COMMENT command, due to the constraint not yet existing.
dumpTableConstraintComment() is an older case that optimally handles the last
dumpComment() arg.

Sounds sane.

In the absence of objections, I'll make it so.

Please do, thanks.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/