Обсуждение: pg_dump does not dump domain not-null constraint's comments

hi.

----------------------------------------------------
begin;
create schema test;
set search_path to test;
create domain d1 as int;
alter domain d1 add constraint nn not null;
alter domain d1 add constraint cc check (value is not null);
comment on constraint nn on domain d1 is 'not null constraint on domain d1';
comment on constraint cc on domain d1 is 'check constraint on domain d1';
commit;

------the above is the test script------------------
in PG17 and master, pg_dump (--schema=test --no-owner) will only produce
COMMENT ON CONSTRAINT cc ON DOMAIN test.d1 IS 'check constraint on domain d1';
but 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?

The attached patch tries to make it produce comments on not-null
constraints on domains.
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.

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)

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 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:

> 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/