Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix pg_dump COMMENT dependency for separate domain constraints.

  2. pg_dump: include comments on not-null constraints on domains, too

  3. Fix dumping of comments on invalid constraints on domains

  4. Catalog domain not-null constraints

  5. In pg_dump, don't dump a stats object unless dumping underlying table.

  6. Enable CHECK constraints to be declared NOT VALID

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

    jian he <jian.universality@gmail.com> — 2025-05-07T09:04:41Z

    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.

  2. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-07T09:25:53Z

    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)

  3. Re: pg_dump does not dump domain not-null constraint's comments

    jian he <jian.universality@gmail.com> — 2025-05-07T14:37:27Z

    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.

  4. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-07T14:56:01Z

    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/

  5. Re: pg_dump does not dump domain not-null constraint's comments

    jian he <jian.universality@gmail.com> — 2025-05-08T04:59:02Z

    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.

  6. Re: pg_dump does not dump domain not-null constraint's comments

    jian he <jian.universality@gmail.com> — 2025-05-22T07:08:46Z

    hi.

"Catalog domain not-null constraints" commit[1]
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

  7. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-14T18:23:58Z

    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/

  8. Re: pg_dump does not dump domain not-null constraint's comments

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-14T18:43:31Z

    =?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

  9. Re: pg_dump does not dump domain not-null constraint's comments

    jian he <jian.universality@gmail.com> — 2025-07-15T06:10:38Z

    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.

  10. Re: pg_dump does not dump domain not-null constraint's comments

    jian he <jian.universality@gmail.com> — 2025-07-15T07:44:10Z

    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.

  11. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-15T13:40:38Z

    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/

  12. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-17T17:04:36Z

    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/

  13. Re: pg_dump does not dump domain not-null constraint's comments

    jian he <jian.universality@gmail.com> — 2025-07-18T14:44:26Z

    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.

  14. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-21T09:48:52Z

    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)

  15. Re: pg_dump does not dump domain not-null constraint's comments

    Noah Misch <noah@leadboat.com> — 2025-09-13T02:02:33Z

    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.

  16. Re: pg_dump does not dump domain not-null constraint's comments

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-13T11:59:29Z

    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/