pg_dump misses comments on NOT NULL constraints

On 2025/06/18 9:13, Fujii Masao wrote:

Hi,

In v18, we can now add comments on NOT NULL constraints. However, I noticed
that pg_dump doesn't include those comments in its output. For example:

--------------------------
$ psql <<EOF
CREATE TABLE t (i int);
ALTER TABLE t ADD CONSTRAINT my_not_null NOT NULL i;
ALTER TABLE t ADD CONSTRAINT my_check CHECK (i > 0);
COMMENT ON CONSTRAINT my_not_null ON t IS 'my not null';
COMMENT ON CONSTRAINT my_check ON t IS 'my check';
EOF

$ pg_dump | grep COMMENT
-- Name: CONSTRAINT my_check ON t; Type: COMMENT; Schema: public; Owner: postgres
COMMENT ON CONSTRAINT my_check ON public.t IS 'my check';
--------------------------

As shown above, the comment on my_not_null is missing from the dump output.

Is this an oversight in commit 14e87ffa5c5? If so, I'll add it as
a v18 open item.

I'm aware of a related open item [1] affecting both v17 and v18,
but this seems like a separate issue, since it relates to a new v18 feature...
Or we should treat them the same?

I ran into another issue related to comments on NOT NULL constraints.
When using CREATE TABLE ... (LIKE ... INCLUDING ALL), the NOT NULL constraints
are copied, but their comments are not. For example:

-----------------------------------------------------
=# CREATE TABLE t (i int);
=# ALTER TABLE t ADD CONSTRAINT my_not_null_i NOT NULL i;
=# ALTER TABLE t ADD CONSTRAINT my_check_i CHECK (i > 0);
=# COMMENT ON CONSTRAINT my_not_null_i ON t IS 'my not null for i';
=# COMMENT ON CONSTRAINT my_check_i ON t IS 'my check for i';

=# CREATE TABLE t_copied (LIKE t INCLUDING ALL);

=# SELECT cls.relname, cnst.conname, obj_description(cnst.oid, 'pg_constraint')
FROM pg_constraint cnst, pg_class cls
WHERE cnst.conrelid = cls.oid AND cnst.conname like '%my_%'
ORDER BY cls.relname, cnst.conname;

relname | conname | obj_description
----------+---------------+-------------------
t | my_check_i | my check for i
t | my_not_null_i | my not null for i
t_copied | my_check_i | my check for i
t_copied | my_not_null_i | (null)
(4 rows)
-----------------------------------------------------

As shown, the comment on my_not_null_i is not copied to the new table,
even though the constraint itself is. Could this be another oversight
in commit 14e87ffa5c5?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025-Jun-18, Fujii Masao wrote:

On 2025/06/18 9:13, Fujii Masao wrote:

In v18, we can now add comments on NOT NULL constraints. However, I noticed
that pg_dump doesn't include those comments in its output. For example:

This is definitely a bug, thanks for reporting.

I ran into another issue related to comments on NOT NULL constraints.
When using CREATE TABLE ... (LIKE ... INCLUDING ALL), the NOT NULL constraints
are copied, but their comments are not.

... and I agree that it'd be good to fix this one as well.

Jian He just mentioned to me that he's looking into these problems.
Many thanks to him. I'm pretty much unable to get anything done this
week, but I'll be back online next week.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Java is clearly an example of money oriented programming" (A. Stepanov)

On Wed, Jun 18, 2025 at 8:13 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

I'm aware of a related open item [1] affecting both v17 and v18,
but this seems like a separate issue, since it relates to a new v18 feature...
Or we should treat them the same?

I think they are separate issues.

for check constraint that dump willed separately:
check constraint comments will go through dumpConstraint,
dumpTableConstraintComment
for check constraint that dump won't dump separately:
check constraint comments will go through dumpTableSchema,
dumpTableConstraintComment

Similarly we don't need to worry about not-null constraints that are
dumped separately.
dumpConstraint, dumpTableConstraintComment will do the job.

however per comment from collectComments say
"/* We needn't remember comments that don't match an */"

there is no ConstraintInfo for these inlined not-null, that means
``static CommentItem *comments = NULL;``
does not hold any comments for these inlined-null constraints.

so for
CREATE TABLE t (i int);
ALTER TABLE t ADD CONSTRAINT my_not_null NOT NULL i;
COMMENT ON CONSTRAINT my_not_null ON t IS 'my not null';

we can not locate the not-null constraint and use dumpComment to dump
the comments.
dumpComment->findComments will return nothing.
but we need to do the similar thing of dumpCommentExtended
``if (ncomments > 0){}`` code branch.

dumpTableSchema handles dumping of table column definitions, and tells us which
column print_notnull is true. Since we already know which columns have had
their not-null constraints printed, it makes sense to dump inline not-null
comments here too.

Please check the attached POC patch.

On 2025-Jun-18, jian he wrote:

Similarly we don't need to worry about not-null constraints that are
dumped separately.
dumpConstraint, dumpTableConstraintComment will do the job.

Right.

dumpTableSchema handles dumping of table column definitions, and tells us which
column print_notnull is true. Since we already know which columns have had
their not-null constraints printed, it makes sense to dump inline not-null
comments here too.

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.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)

On Wed, Jun 18, 2025 at 11:05 PM Álvaro Herrera <alvherre@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.

On Wed, Jun 18, 2025 at 10:21 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

I ran into another issue related to comments on NOT NULL constraints.
When using CREATE TABLE ... (LIKE ... INCLUDING ALL), the NOT NULL constraints
are copied, but their comments are not. For example:

-----------------------------------------------------
=# CREATE TABLE t (i int);
=# ALTER TABLE t ADD CONSTRAINT my_not_null_i NOT NULL i;
=# ALTER TABLE t ADD CONSTRAINT my_check_i CHECK (i > 0);
=# COMMENT ON CONSTRAINT my_not_null_i ON t IS 'my not null for i';
=# COMMENT ON CONSTRAINT my_check_i ON t IS 'my check for i';

=# CREATE TABLE t_copied (LIKE t INCLUDING ALL);

As shown, the comment on my_not_null_i is not copied to the new table,
even though the constraint itself is. Could this be another oversight
in commit 14e87ffa5c5?

hi.
in transformTableLikeClausem, let cxt(CreateStmtContext) to add
CommentStmt should just work.
Please check attached, tests also added.

On 2025/06/19 14:42, jian he wrote:

On Wed, Jun 18, 2025 at 10:21 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

I ran into another issue related to comments on NOT NULL constraints.
When using CREATE TABLE ... (LIKE ... INCLUDING ALL), the NOT NULL constraints
are copied, but their comments are not. For example:

-----------------------------------------------------
=# CREATE TABLE t (i int);
=# ALTER TABLE t ADD CONSTRAINT my_not_null_i NOT NULL i;
=# ALTER TABLE t ADD CONSTRAINT my_check_i CHECK (i > 0);
=# COMMENT ON CONSTRAINT my_not_null_i ON t IS 'my not null for i';
=# COMMENT ON CONSTRAINT my_check_i ON t IS 'my check for i';

=# CREATE TABLE t_copied (LIKE t INCLUDING ALL);

As shown, the comment on my_not_null_i is not copied to the new table,
even though the constraint itself is. Could this be another oversight
in commit 14e87ffa5c5?

hi.
in transformTableLikeClausem, let cxt(CreateStmtContext) to add
CommentStmt should just work.
Please check attached, tests also added.

Thanks for the patch! LGTM.

Just one minor suggestion:

+		/* Copy comments on not-null constraints */
+		if (table_like_clause->options & CREATE_TABLE_LIKE_COMMENTS)
+		{

It might be clearer to move this block after the line:

cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);

That would make the code a bit more readable.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025/06/19 11:38, jian he wrote:

On Wed, Jun 18, 2025 at 11:05 PM Álvaro Herrera <alvherre@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] = "";

Do we really need this part? With the patch, due to this part,
pg_dump might emit output like:

CREATE TABLE t (i int NOT NULL);
COMMENT ON CONSTRAINT t_i_not_null ON public.t IS '.......';

This seems to rely on the assumption that the naming convention for
NOT NULL constraints (e.g., t_i_not_null) will remain stable across versions.
If that ever changes, the dumped DDL could fail.

Wouldn't it be safer to always print the constraint name explicitly,
even if it's the default name? For example:

CREATE TABLE t (i int CONSTRAINT t_i_not_null NOT NULL);
COMMENT ON CONSTRAINT t_i_not_null ON public.t IS '.......';

This is consistent with how check constraints are handled.

Regarding the patch:

+	if (!fout->dopt->no_comments &&
+		 dopt->dumpSchema &&
+		 fout->remoteVersion >= 180000)

The dopt->dumpSchema check is redundant, since dumpTableSchema() is
only called when that flag is already true?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025/06/19 20:53, Fujii Masao wrote:

On 2025/06/19 14:42, jian he wrote:

On Wed, Jun 18, 2025 at 10:21 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

I ran into another issue related to comments on NOT NULL constraints.
When using CREATE TABLE ... (LIKE ... INCLUDING ALL), the NOT NULL constraints
are copied, but their comments are not. For example:

-----------------------------------------------------
=# CREATE TABLE t (i int);
=# ALTER TABLE t ADD CONSTRAINT my_not_null_i NOT NULL i;
=# ALTER TABLE t ADD CONSTRAINT my_check_i CHECK (i > 0);
=# COMMENT ON CONSTRAINT my_not_null_i ON t IS 'my not null for i';
=# COMMENT ON CONSTRAINT my_check_i ON t IS 'my check for i';

=# CREATE TABLE t_copied (LIKE t INCLUDING ALL);

As shown, the comment on my_not_null_i is not copied to the new table,
even though the constraint itself is. Could this be another oversight
in commit 14e87ffa5c5?

hi.
in transformTableLikeClausem, let cxt(CreateStmtContext) to add
CommentStmt should just work.
Please check attached, tests also added.

Thanks for the patch! LGTM.

Just one minor suggestion:

+        /* Copy comments on not-null constraints */
+        if (table_like_clause->options & CREATE_TABLE_LIKE_COMMENTS)
+        {

It might be clearer to move this block after the line:

    cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);

That would make the code a bit more readable.

I've applied this cosmetic change to the patch.
Barring objections, I'll commit the patch.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025-Jun-25, Fujii Masao wrote:

From 516e647e7d1fdafc64dba092389963f32cd688e5 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 25 Jun 2025 10:02:56 +0900
Subject: [PATCH v2] Make CREATE TABLE LIKE copy comments on NOT NULL
constraints when requested.

Commit 14e87ffa5c5 introduced support for adding comments to NOT NULL
constraints. However, CREATE TABLE LIKE INCLUDING COMMENTS did not copy
these comments to the new table. This was an oversight in that commit.

This commit corrects the behavior by ensuring CREATE TABLE LIKE to also copy
the comments on NOT NULL constraints when INCLUDING COMMENTS is specified.

LGTM. I'd add a line in the test showing that these comments are copied
even if INCLUDING CONSTRAINTS is not given, because not-null constraints
comments themselves are -- as attached.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there. Did
somebody make percentages logarithmic while I wasn't looking?"
http://smylers.hates-software.com/2005/09/08/1995c749.html

On 2025-Jun-25, Fujii Masao wrote:

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] = "";

Do we really need this part? With the patch, due to this part,
pg_dump might emit output like:

CREATE TABLE t (i int NOT NULL);
COMMENT ON CONSTRAINT t_i_not_null ON public.t IS '.......';

This seems to rely on the assumption that the naming convention for
NOT NULL constraints (e.g., t_i_not_null) will remain stable across versions.

Yeah, I think in this case we need to extract the constraint name so
that we have it available to print the COMMENT command, rather than
making any assumptions about it. In fact I suspect this would fail if
the table or column names are very long. For the other pg_dump uses of
this logic it doesn't matter AFAIR, but here I think we must be
stricter.

Wouldn't it be safer to always print the constraint name explicitly,
even if it's the default name?

Yes, but we wanted to avoid printing those names in "normal cases," so
we go some lengths to avoid them if not necessary. However, I suspect
it's easy to print the names only if a comment exists -- which is also
an unusual case.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"XML!" Exclaimed C++. "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/

On 2025-Jun-25, Álvaro Herrera wrote:

Yeah, I think in this case we need to extract the constraint name so
that we have it available to print the COMMENT command, rather than
making any assumptions about it. In fact I suspect this would fail if
the table or column names are very long. For the other pg_dump uses of
this logic it doesn't matter AFAIR, but here I think we must be
stricter.

As attached.

I'm bothered by this not having any tests -- I'll see about adding some
after lunch. But at least, this seems to be dumped correctly:

CREATE TABLE supercallifragilisticexpialidocious ("dociousaliexpilistic fragilcalirupus" int not null);
COMMENT ON CONSTRAINT "supercallifragilisticexpial_dociousaliexpilistic fragi_not_null" ON public.supercallifragilisticexpialidocious IS 'long names, huh?';

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

On 2025/06/25 20:46, Álvaro Herrera wrote:

On 2025-Jun-25, Fujii Masao wrote:

From 516e647e7d1fdafc64dba092389963f32cd688e5 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 25 Jun 2025 10:02:56 +0900
Subject: [PATCH v2] Make CREATE TABLE LIKE copy comments on NOT NULL
constraints when requested.

Commit 14e87ffa5c5 introduced support for adding comments to NOT NULL
constraints. However, CREATE TABLE LIKE INCLUDING COMMENTS did not copy
these comments to the new table. This was an oversight in that commit.

This commit corrects the behavior by ensuring CREATE TABLE LIKE to also copy
the comments on NOT NULL constraints when INCLUDING COMMENTS is specified.

LGTM. I'd add a line in the test showing that these comments are copied
even if INCLUDING CONSTRAINTS is not given,

+1 to adding that test.

  CREATE TABLE ctlt1_inh (LIKE ctlt1 INCLUDING CONSTRAINTS INCLUDING COMMENTS) INHERITS (ctlt1);
  \d+ ctlt1_inh
-SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass;
+SELECT conname, description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass ORDER BY conname COLLATE "C";

However, since ctlt1_inh is created with INCLUDING COMMENTS, this test
doesn't seem to demonstrate the case you mentioned — that comments on
not-null constraints are copied even without INCLUDING CONSTRAINTS.
Am I misunderstanding?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025/06/25 22:36, Álvaro Herrera wrote:

On 2025-Jun-25, Álvaro Herrera wrote:

Yeah, I think in this case we need to extract the constraint name so
that we have it available to print the COMMENT command, rather than
making any assumptions about it. In fact I suspect this would fail if
the table or column names are very long. For the other pg_dump uses of
this logic it doesn't matter AFAIR, but here I think we must be
stricter.

As attached.

Thanks for the patch! I agree with the approach, i.e., printing the not-null
constraint name only when there's a comment on it.

However, with the patch applied, I encountered a segmentation fault in pg_dump
as follows:

$ psql <<EOF
create table t (i int);
alter table t add constraint t_i_not_null not null i not valid;
comment on constraint t_i_not_null ON t IS 'iii';
EOF

$ pg_dump
Segmentation fault: 11

I'm bothered by this not having any tests -- I'll see about adding some
after lunch.

+1. Thanks!

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On Wed, Jun 25, 2025 at 11:04 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

This commit corrects the behavior by ensuring CREATE TABLE LIKE to also copy
the comments on NOT NULL constraints when INCLUDING COMMENTS is specified.

LGTM. I'd add a line in the test showing that these comments are copied
even if INCLUDING CONSTRAINTS is not given,

+1 to adding that test.

CREATE TABLE ctlt1_inh (LIKE ctlt1 INCLUDING CONSTRAINTS INCLUDING COMMENTS) INHERITS (ctlt1);
\d+ ctlt1_inh
-SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass;
+SELECT conname, description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass ORDER BY conname COLLATE "C";

However, since ctlt1_inh is created with INCLUDING COMMENTS, this test
doesn't seem to demonstrate the case you mentioned — that comments on
not-null constraints are copied even without INCLUDING CONSTRAINTS.
Am I misunderstanding?

to do that,
we can create another table in create_table_like.sql:
CREATE TABLE noinh_con_copy2 (LIKE noinh_con_copy INCLUDING COMMENTS);

or change noinh_con_copy1 definition to
CREATE TABLE noinh_con_copy1 (LIKE noinh_con_copy INCLUDING COMMENTS);

On 2025-Jun-26, Fujii Masao wrote:

CREATE TABLE ctlt1_inh (LIKE ctlt1 INCLUDING CONSTRAINTS INCLUDING COMMENTS) INHERITS (ctlt1);
\d+ ctlt1_inh
-SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass;
+SELECT conname, description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass ORDER BY conname COLLATE "C";

However, since ctlt1_inh is created with INCLUDING COMMENTS, this test
doesn't seem to demonstrate the case you mentioned — that comments on
not-null constraints are copied even without INCLUDING CONSTRAINTS.
Am I misunderstanding?

Hmm, yeah the case I wanted to modify was for ctlt12_comments which does
INCLUDING COMMENTS without constraints, apologies. In that case it's
better to modify ctlt2 instead, as shown here.

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

On 2025-Jun-26, Fujii Masao wrote:

However, with the patch applied, I encountered a segmentation fault in pg_dump
as follows:

Ah, thanks for the test case. Yeah, I removed one 'if' condition too
many from Jian's patch. We just need to test the constraint name for
nullness and then things seem to work:

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5dd1f42631d..7b5024d4f71 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -17719,7 +17719,8 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)

 		for (j = 0; j < tbinfo->numatts; j++)
 		{
-			if (tbinfo->notnull_comment[j] != NULL)
+			if (tbinfo->notnull_constrs[j] != NULL &&
+				tbinfo->notnull_comment[j] != NULL)
 			{
 				if (comment == NULL)
 				{

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On 2025-Jun-25, Álvaro Herrera wrote:

Ah, thanks for the test case. Yeah, I removed one 'if' condition too
many from Jian's patch. We just need to test the constraint name for
nullness and then things seem to work:

One more thing was missing, which I noticed as I added the tests.
Apparently the COMMENT objects can end up in any section of the dump,
but for comments on valid constraints, this is not what we want -- they
should go into the PRE_DATA section only. When running the tests with
the code still putting them in SECTION_NONE, apparently pg_dump would
randomly put them either here or there, so the tests would fail
intermittently. Or at least that's what I think happened. Once I made
them be in SECTION_PRE_DATA, that problem disappeared.

Anyway, here's what I intend to push tomorrow, unless further problems
are found.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)

On 2025/06/26 3:45, Álvaro Herrera wrote:

On 2025-Jun-25, Álvaro Herrera wrote:

Ah, thanks for the test case. Yeah, I removed one 'if' condition too
many from Jian's patch. We just need to test the constraint name for
nullness and then things seem to work:

One more thing was missing, which I noticed as I added the tests.
Apparently the COMMENT objects can end up in any section of the dump,
but for comments on valid constraints, this is not what we want -- they
should go into the PRE_DATA section only. When running the tests with
the code still putting them in SECTION_NONE, apparently pg_dump would
randomly put them either here or there, so the tests would fail
intermittently. Or at least that's what I think happened. Once I made
them be in SECTION_PRE_DATA, that problem disappeared.

Anyway, here's what I intend to push tomorrow, unless further problems
are found.

Thanks for updating the patch!

I noticed a small inconsistency in the output of pg_dump when handling
comments for COMMENT commands on not-null constraints. For example,
with the following DDL:

-------------
CREATE SCHEMA hoge;
CREATE TABLE hoge.t (i int PRIMARY KEY, j int NOT NULL CHECK (j > 0));
COMMENT ON CONSTRAINT t_pkey ON hoge.t IS 'primary key';
COMMENT ON CONSTRAINT t_j_not_null ON hoge.t IS 'not null';
COMMENT ON CONSTRAINT t_j_check ON hoge.t IS 'check';
-------------

The pg_dump output shows the following comments for COMMENT commands:

-------------
-- Name: CONSTRAINT t_j_not_null ON hoge.t; Type: COMMENT; Schema: hoge; Owner: postgres
-- Name: CONSTRAINT t_j_check ON t; Type: COMMENT; Schema: hoge; Owner: postgres
-- Name: CONSTRAINT t_pkey ON t; Type: COMMENT; Schema: hoge; Owner: postgres
-------------

You can see that only comments for the not-null constraint includes
the schema-qualified table name (hoge.t) after "ON". The others just
show "t". It's a very minor issue, but for consistency, it would be
better if all constraint comments used the same format.

+		if (comment != NULL)
+		{
+			destroyPQExpBuffer(comment);
+			destroyPQExpBuffer(tag);

The "comment != NULL" check isn't needed here, since destroyPQExpBuffer()
already handles null safely.

Since valid not-null constraints are dumped in the pre-data section,
the following change seems necessary in pg_dump.sgml.

            statistics for indexes, and constraints other than validated check
-          constraints.
+          and not-null constraints.
            Pre-data items include all other data definition items.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation