Обсуждение: pg_dump misses comments on NOT NULL constraints
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? Regards, [1] https://www.postgresql.org/message-id/CACJufxF-0bqVR%3Dj4jonS6N2Ka6hHUpFyu3_3TWKNhOW_4yFSSg%40mail.gmail.com -- Fujii Masao NTT DATA Japan Corporation
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 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 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-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.supercallifragilisticexpialidociousIS '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 ANDc.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.oidAND 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: > 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/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
On 2025/06/26 0:45, Álvaro Herrera wrote: > 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.oidAND 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. Thanks for updating the tests! I've incorporated the changes into the patch and committed it. Regards, -- Fujii Masao NTT DATA Japan Corporation
On 2025-Jun-26, Fujii Masao wrote: > I noticed a small inconsistency in the output of pg_dump when handling > comments for COMMENT commands on not-null constraints. [...] > 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. Hmm, you're right. I think we should make the tags use qualified names instead, but that'd be a much larger patch. > + if (comment != NULL) > + { > + destroyPQExpBuffer(comment); > + destroyPQExpBuffer(tag); > > The "comment != NULL" check isn't needed here, since destroyPQExpBuffer() > already handles null safely. Hah, true. > 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. Ah, right, did that, thank you. Thanks for reporting this and to Jian for the quick analysis and patch! -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Oh, great altar of passive entertainment, bestow upon me thy discordant images at such speed as to render linear thought impossible" (Calvin a la TV)