pg_restore --no-policies should not restore policies' comment
hi.
first looking at function dumpPolicy (pg_dump.c):
appendPQExpBuffer(polprefix, "POLICY %s ON",
fmtId(polinfo->polname));
tag = psprintf("%s %s", tbinfo->dobj.name, polinfo->dobj.name);
....
if (polinfo->dobj.dump & DUMP_COMPONENT_COMMENT)
dumpComment(fout, polprefix->data, qtabname,
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
then looking at function dumpCommentExtended:
appendPQExpBuffer(query, "COMMENT ON %s ", type);
if (namespace && *namespace)
appendPQExpBuffer(query, "%s.", fmtId(namespace));
appendPQExpBuffer(query, "%s IS ", name);
appendStringLiteralAH(query, comments->descr, fout);
appendPQExpBufferStr(query, ";\n");
appendPQExpBuffer(tag, "%s %s", type, name);
/*
* We mark comments as SECTION_NONE because they really belong in the
* same section as their parent, whether that is pre-data or
* post-data.
*/
ArchiveEntry(fout, nilCatalogId, createDumpId(),
ARCHIVE_OPTS(.tag = tag->data,
.namespace = namespace,
.owner = owner,
.description = "COMMENT",
.section = SECTION_NONE,
.createStmt = query->data,
.deps = &dumpId,
.nDeps = 1));
also looking at function ArchiveEntry in pg_backup_archiver.c
newToc->tag = pg_strdup(opts->tag);
if pg_restore --no-policies is specified then we generally don't want
to restore policies' comments too.
To do that, we need
1. we checked that COMMENTS on policies, the TocEntry->tag begins with
"POLICY". which is true, see above code walk through.
2. We also need to make sure that no other dumpComment call results in a
COMMENT command whose TocEntry->tag also starts with "POLICY".
which is also true, per https://www.postgresql.org/docs/current/sql-comment.html
after "COMMENT ON", the next word is fixed, and "POLICY" only occurs once.
If this is what we want, we can do the same for
"--no-publications", "--no-subscriptions" too.
Attachments:
v1-0001-fix-pg_restore-not-restore-comments.patchapplication/x-patch; name=v1-0001-fix-pg_restore-not-restore-comments.patchDownload
From 235f05dc6a1fcb99e4cfb6f9d6a3ca8bc5c392db Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Fri, 27 Jun 2025 11:59:31 +0800
Subject: [PATCH v1 1/1] fix pg_restore not restore comments
in pg_restore, if --no-policies is specified, then there's no need to restore
comments on policies either.
---
src/bin/pg_dump/pg_backup_archiver.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 197c1295d93..d81e392a832 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3020,6 +3020,14 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
strcmp(te->desc, "ROW SECURITY") == 0))
return 0;
+ /*
+ * If --no-policies is specified, there's no need to restore comments on
+ * policies either.
+ */
+ if (ropt->no_policies && (strcmp(te->desc, "COMMENT") == 0) &&
+ (strncmp(te->tag, "POLICY", strlen("POLICY")) == 0))
+ return 0;
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
--
2.34.1
On 2025/06/27 13:08, jian he wrote:
hi.
first looking at function dumpPolicy (pg_dump.c):
appendPQExpBuffer(polprefix, "POLICY %s ON",
fmtId(polinfo->polname));
tag = psprintf("%s %s", tbinfo->dobj.name, polinfo->dobj.name);
....
if (polinfo->dobj.dump & DUMP_COMPONENT_COMMENT)
dumpComment(fout, polprefix->data, qtabname,
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,then looking at function dumpCommentExtended:
appendPQExpBuffer(query, "COMMENT ON %s ", type);
if (namespace && *namespace)
appendPQExpBuffer(query, "%s.", fmtId(namespace));
appendPQExpBuffer(query, "%s IS ", name);
appendStringLiteralAH(query, comments->descr, fout);
appendPQExpBufferStr(query, ";\n");appendPQExpBuffer(tag, "%s %s", type, name);
/*
* We mark comments as SECTION_NONE because they really belong in the
* same section as their parent, whether that is pre-data or
* post-data.
*/
ArchiveEntry(fout, nilCatalogId, createDumpId(),
ARCHIVE_OPTS(.tag = tag->data,
.namespace = namespace,
.owner = owner,
.description = "COMMENT",
.section = SECTION_NONE,
.createStmt = query->data,
.deps = &dumpId,
.nDeps = 1));also looking at function ArchiveEntry in pg_backup_archiver.c
newToc->tag = pg_strdup(opts->tag);
if pg_restore --no-policies is specified then we generally don't want
to restore policies' comments too.
Agreed. Otherwise, pg_restore --no-policies might skip creating
the policy object but still try to run a COMMENT command on it,
which would fail since the policy object doesn't exist.
To do that, we need
1. we checked that COMMENTS on policies, the TocEntry->tag begins with
"POLICY". which is true, see above code walk through.
2. We also need to make sure that no other dumpComment call results in a
COMMENT command whose TocEntry->tag also starts with "POLICY".
which is also true, per https://www.postgresql.org/docs/current/sql-comment.html
after "COMMENT ON", the next word is fixed, and "POLICY" only occurs once.If this is what we want, we can do the same for
"--no-publications", "--no-subscriptions" too.
Agreed.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
On Fri, Jun 27, 2025 at 1:34 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
To do that, we need
1. we checked that COMMENTS on policies, the TocEntry->tag begins with
"POLICY". which is true, see above code walk through.
2. We also need to make sure that no other dumpComment call results in a
COMMENT command whose TocEntry->tag also starts with "POLICY".
which is also true, per https://www.postgresql.org/docs/current/sql-comment.html
after "COMMENT ON", the next word is fixed, and "POLICY" only occurs once.If this is what we want, we can do the same for
"--no-publications", "--no-subscriptions" too.Agreed.
hi.
I’ve tested the pg_restore options --no-policies, --no-publications, and
--no-subscriptions locally.
However, I haven’t tested --no-security-labels option, so no changes were
made for it. Testing --no-security-labels appears to need more setup, which
didn’t seem trivial.
writing Perl tests is not easier for me, I didn’t add those either.
(seems in master, we didn't have --no-publications, --no-subscriptions
tests too)
Attachments:
v2-0001-pg_restore-not-restore-comments-when-it-s-object-not-rest.patchtext/x-patch; charset=US-ASCII; name=v2-0001-pg_restore-not-restore-comments-when-it-s-object-not-rest.patchDownload
From 024b158f20d2c8ea10989e7e5139a771e00338ce Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Wed, 2 Jul 2025 10:14:58 +0800
Subject: [PATCH v2 1/1] pg_restore not restore comments when it's object not
restored
If certain objects are excluded from restoration in pg_restore, then comments
associated with those objects should also not be restored.
So this patch applies to the following pg_restore option:
--no-policies, --no-publications, --no-subscriptions
i manually tested these three options.
I didn't test --no-security-labels option, so no changes for it.
discussion: https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org
---
src/bin/pg_dump/pg_backup_archiver.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 197c1295d93..44bcce71712 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3020,6 +3020,23 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
strcmp(te->desc, "ROW SECURITY") == 0))
return 0;
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ /*
+ * If --no-policies, --no-publications, or --no-subscriptions is
+ * specified, comments related to those objects should also be skipped
+ * during restore.
+ */
+ if (ropt->no_policies && strncmp(te->tag, "POLICY", strlen("POLICY")) == 0)
+ return 0;
+
+ if (ropt->no_publications && strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions && strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
--
2.34.1
On 2025/07/02 11:17, jian he wrote:
On Fri, Jun 27, 2025 at 1:34 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
To do that, we need
1. we checked that COMMENTS on policies, the TocEntry->tag begins with
"POLICY". which is true, see above code walk through.
2. We also need to make sure that no other dumpComment call results in a
COMMENT command whose TocEntry->tag also starts with "POLICY".
which is also true, per https://www.postgresql.org/docs/current/sql-comment.html
after "COMMENT ON", the next word is fixed, and "POLICY" only occurs once.If this is what we want, we can do the same for
"--no-publications", "--no-subscriptions" too.Agreed.
hi.
I’ve tested the pg_restore options --no-policies, --no-publications, and
--no-subscriptions locally.
Thanks for updating the patch! Could you add it to the next CommitFest
so we don't forget about it?
However, I haven’t tested --no-security-labels option, so no changes were
made for it. Testing --no-security-labels appears to need more setup, which
didn’t seem trivial.
You're checking whether pg_restore --no-publications --no-subscriptions correctly
skips security labels for publications and subscriptions, and if not,
you'll prepare a patch. Right? I'm not sure how common it is to define security
labels on publications or subscriptions, but if the behavior is unexpected (i.e.,
the security labels are not skipped in that case), it's worth fixing.
It would probably be better to handle that in a separate patch from the one
for comments.
To set up a security label for testing, you can use the
src/test/modules/dummy_seclabel module.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
On Wed, Jul 2, 2025 at 5:18 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
hi.
I’ve tested the pg_restore options --no-policies, --no-publications, and
--no-subscriptions locally.Thanks for updating the patch! Could you add it to the next CommitFest
so we don't forget about it?
sure.
However, I haven’t tested --no-security-labels option, so no changes were
made for it. Testing --no-security-labels appears to need more setup, which
didn’t seem trivial.You're checking whether pg_restore --no-publications --no-subscriptions correctly
skips security labels for publications and subscriptions, and if not,
you'll prepare a patch. Right? I'm not sure how common it is to define security
labels on publications or subscriptions, but if the behavior is unexpected (i.e.,
the security labels are not skipped in that case), it's worth fixing.
It would probably be better to handle that in a separate patch from the one
for comments.To set up a security label for testing, you can use the
src/test/modules/dummy_seclabel module.
Thanks!
Using dummy_seclabel helped me test the pg_restore --no-security-labels
behavior. All the attached patches have now been tested locally. I’ll need help
writing the Perl tests....
I found out another problem while playing with pg_restore --no-security-labels:
pg_dump does not dump security labels on global objects like
subscriptions or roles.
summary of attached patch:
01: make pg_restore not restore comments if comments associated
objects are excluded.
02: make pg_dump dump security label for shared database objects, like
subscription, roles.
03: make pg_restore not restore security labels if the associated
object is excluded.
Attachments:
v2-0002-make-pg_dump-dump-security-label-for-shared-database-obje.patchtext/x-patch; charset=UTF-8; name=v2-0002-make-pg_dump-dump-security-label-for-shared-database-obje.patchDownload
From 62055ddac9cf73def22227e9d3af275f58180707 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Thu, 3 Jul 2025 11:18:06 +0800
Subject: [PATCH v2 2/3] make pg_dump dump security label for shared database
objects
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
As noted in [1], SUBSCRIPTION and similar objects are treated as global objects.
Therefore, we should use pg_catalog.pg_seclabels in collectSecLabels instead of
pg_catalog.pg_seclabel.
The database’s own security label is handled via dumpDatabase, so this change
should be appropriate.
[1] https://www.postgresql.org/docs/devel/catalog-pg-shseclabel.html
discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
commitfest entry: https://commitfest.postgresql.org/
---
src/bin/pg_dump/pg_dump.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1937997ea67..17cca2dd667 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16476,7 +16476,7 @@ collectSecLabels(Archive *fout)
appendPQExpBufferStr(query,
"SELECT label, provider, classoid, objoid, objsubid "
- "FROM pg_catalog.pg_seclabel "
+ "FROM pg_catalog.pg_seclabels "
"ORDER BY classoid, objoid, objsubid");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
--
2.34.1
v2-0003-not-restore-security-labels-if-the-associated-object-is-e.patchtext/x-patch; charset=US-ASCII; name=v2-0003-not-restore-security-labels-if-the-associated-object-is-e.patchDownload
From 74a92cfbf38872945ed9362bbbd4e90fb3639787 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Thu, 3 Jul 2025 22:13:43 +0800
Subject: [PATCH v2 3/3] not restore security labels if the associated object
is excluded
When --no-publications or --no-subscriptions is specified in pg_restore,
security labels associated with those objects should be excluded.
pg_dump.c, dumpSecLabel function and the Security Label synopsis section [1] helps
verify that the TocEntry->tag string for a security label begins with the object
type it is associated with. So this patch change should be fine.
[1] https://www.postgresql.org/docs/current/sql-security-label.html
discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
commitfest entry: https://commitfest.postgresql.org/
---
src/bin/pg_dump/pg_backup_archiver.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 44bcce71712..f407f9fc280 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3051,6 +3051,21 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
if (ropt->no_security_labels && strcmp(te->desc, "SECURITY LABEL") == 0)
return 0;
+
+ if (strcmp(te->desc, "SECURITY LABEL") == 0)
+ {
+ /*
+ * If --no-publications, or --no-subscriptions is specified, security
+ * label related to those objects should also be skipped during restore.
+ * see dumpSecLabel also.
+ */
+ if (ropt->no_publications && strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions && strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/* If it's a subscription, maybe ignore it */
if (ropt->no_subscriptions && strcmp(te->desc, "SUBSCRIPTION") == 0)
return 0;
--
2.34.1
v2-0001-not-restore-comments-if-the-associated-object-is-excluded.patchtext/x-patch; charset=US-ASCII; name=v2-0001-not-restore-comments-if-the-associated-object-is-excluded.patchDownload
From eeaff76aea6dc92a122b8187248773c72ffba691 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Thu, 3 Jul 2025 22:16:05 +0800
Subject: [PATCH v2 1/3] not restore comments if the associated object is
excluded
If certain objects are excluded from restoration in pg_restore, then comments
associated with those objects should also not be restored.
So this patch applies to the following pg_restore option:
--no-policies, --no-publications, --no-subscriptions
TODO: need perl tests
discussion: https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org
---
src/bin/pg_dump/pg_backup_archiver.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 197c1295d93..44bcce71712 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3020,6 +3020,23 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
strcmp(te->desc, "ROW SECURITY") == 0))
return 0;
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ /*
+ * If --no-policies, --no-publications, or --no-subscriptions is
+ * specified, comments related to those objects should also be skipped
+ * during restore.
+ */
+ if (ropt->no_policies && strncmp(te->tag, "POLICY", strlen("POLICY")) == 0)
+ return 0;
+
+ if (ropt->no_publications && strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions && strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
--
2.34.1
On Thu, Jul 3, 2025 at 11:32 PM jian he <jian.universality@gmail.com> wrote:
On Wed, Jul 2, 2025 at 5:18 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
hi.
I’ve tested the pg_restore options --no-policies, --no-publications, and
--no-subscriptions locally.Thanks for updating the patch! Could you add it to the next CommitFest
so we don't forget about it?sure.
However, I haven’t tested --no-security-labels option, so no changes were
made for it. Testing --no-security-labels appears to need more setup, which
didn’t seem trivial.You're checking whether pg_restore --no-publications --no-subscriptions correctly
skips security labels for publications and subscriptions, and if not,
you'll prepare a patch. Right? I'm not sure how common it is to define security
labels on publications or subscriptions, but if the behavior is unexpected (i.e.,
the security labels are not skipped in that case), it's worth fixing.
It would probably be better to handle that in a separate patch from the one
for comments.To set up a security label for testing, you can use the
src/test/modules/dummy_seclabel module.Thanks!
Using dummy_seclabel helped me test the pg_restore --no-security-labels
behavior. All the attached patches have now been tested locally. I’ll need help
writing the Perl tests....I found out another problem while playing with pg_restore --no-security-labels:
pg_dump does not dump security labels on global objects like
subscriptions or roles.summary of attached patch:
Thanks for the patches!
01: make pg_restore not restore comments if comments associated
objects are excluded.
TODO: need perl tests
How about adding tests for pg_restore --no-policies in 002_pg_dump.pl,
as in the attached patch?
Since --no-publications and --no-subscriptions have been around for a long time,
while --no-policies was added in v18, I wonder if it makes sense to first fix
the publications and subscriptions cases (and add tests for them) and back-patch
to all supported versions. Then we can handle the policies case and
back-patch it
only to v18. Does that sound reasonable?
02: make pg_dump dump security label for shared database objects, like
subscription, roles.
03: make pg_restore not restore security labels if the associated
object is excluded.
Will review them later.
Regards,
--
Fujii Masao
Attachments:
v3-0001-pg_restore-Fix-handling-of-comments-on-policies-p.patchapplication/octet-stream; name=v3-0001-pg_restore-Fix-handling-of-comments-on-policies-p.patchDownload
From c632ba872f3e22bd0e662d3f1908723611370da9 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Tue, 26 Aug 2025 20:11:51 +0900
Subject: [PATCH v3] pg_restore: Fix handling of comments on
policies/publications/subscriptions.
Previously, when --no-policies, --no-publications, or --no-subscriptions
were specified, pg_restore skipped restoring the corresponding objects
but did not skip their comments incorrectly. This could cause failures
by trying to restore comments on objects that were not created.
This commit fixes the issue by making pg_restore also skip comments on
these objects when the corresponding options are used.
Discussion: https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org
---
src/bin/pg_dump/pg_backup_archiver.c | 19 ++++++++++++
src/bin/pg_dump/t/002_pg_dump.pl | 46 ++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 3c3acbaccdb..c03479484c5 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3048,6 +3048,25 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
strcmp(te->desc, "ROW SECURITY") == 0))
return 0;
+ /*
+ * If it's a comment on a policy, a publication, or a subscription, maybe
+ * ignore it.
+ */
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ if (ropt->no_policies &&
+ strncmp(te->tag, "POLICY", strlen("POLICY")) == 0)
+ return 0;
+
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index e7a2d64f741..8c4e8a8bc73 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -632,6 +632,23 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_policies_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_policies_restore.dump",
+ '--statistics',
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_policies_restore.sql",
+ '--no-policies',
+ '--statistics',
+ "$tempdir/no_policies_restore.dump",
+ ],
+ },
no_privs => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -871,6 +888,7 @@ my %full_runs = (
no_large_objects => 1,
no_owner => 1,
no_policies => 1,
+ no_policies_restore => 1,
no_privs => 1,
no_statistics => 1,
no_table_access_method => 1,
@@ -1512,6 +1530,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -1830,6 +1849,27 @@ my %tests = (
},
},
+ 'COMMENT ON POLICY p1' => {
+ create_order => 55,
+ create_sql => 'COMMENT ON POLICY p1 ON dump_test.test_table
+ IS \'comment on policy\';',
+ regexp =>
+ qr/^COMMENT ON POLICY p1 ON dump_test.test_table IS 'comment on policy';/m,
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ only_dump_test_table => 1,
+ section_post_data => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ exclude_test_table => 1,
+ no_policies => 1,
+ no_policies_restore => 1,
+ only_dump_measurement => 1,
+ },
+ },
+
'COMMENT ON PUBLICATION pub1' => {
create_order => 55,
create_sql => 'COMMENT ON PUBLICATION pub1
@@ -3192,6 +3232,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3214,6 +3255,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3236,6 +3278,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3258,6 +3301,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3280,6 +3324,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3302,6 +3347,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
--
2.50.1
On Tue, Aug 26, 2025 at 7:43 PM Fujii Masao <masao.fujii@gmail.com> wrote:
summary of attached patch:
Thanks for the patches!
01: make pg_restore not restore comments if comments associated
objects are excluded.TODO: need perl tests
How about adding tests for pg_restore --no-policies in 002_pg_dump.pl,
as in the attached patch?
it works fine on my local linux machine.
Since --no-publications and --no-subscriptions have been around for a long time,
while --no-policies was added in v18, I wonder if it makes sense to first fix
the publications and subscriptions cases (and add tests for them) and back-patch
to all supported versions. Then we can handle the policies case and
back-patch it
only to v18. Does that sound reasonable?
works for me.
On Wed, Aug 27, 2025 at 3:18 PM jian he <jian.universality@gmail.com> wrote:
Since --no-publications and --no-subscriptions have been around for a long time,
while --no-policies was added in v18, I wonder if it makes sense to first fix
the publications and subscriptions cases (and add tests for them) and back-patch
to all supported versions. Then we can handle the policies case and
back-patch it
only to v18. Does that sound reasonable?works for me.
So I've split your v2-0001 patch into two patches:
* v4-0001 handles comments on publications and subscriptions when
--no-publications / --no-subscriptions are specified. This will be
backpatched to all supported versions.
* v4-0002 handles comments on policies when --no-policies is specified.
This will be backpatched to v18, where --no-policies was added.
Both v4-0001 and v4-0002 are based on your patch, but I added
regression tests for them.
02: make pg_dump dump security label for shared database objects, like
subscription, roles.
As I understand it, shared objects like roles are handled by pg_dumpall,
which already dumps their security labels via pg_shseclabel.
Subscriptions are an exception: pg_dump dumps them (and should dump
their security labels), but those labels are stored in pg_shseclabel,
which pg_dump doesn't query.
To fix this, making pg_dump query also pg_shseclabel when dumping
subscriptions would work. But your approach, having pg_dump query
pg_seclabels (covering both pg_seclabel and pg_shseclabel),
is simpler and sufficient. So I like your approach for now.
I also noticed pg_dump didn't dump security labels on event triggers,
so I extended your patch as v4-0003 to handle those as well.
03: make pg_restore not restore security labels if the associated
object is excluded.
This patch looks good. I only applied minor cosmetic changes and
attached it as v4-0004.
Regards,
--
Fujii Masao
Attachments:
v4-0001-PG17-pg_restore-Fix-comment-handling-with-no-publicati.txttext/plain; charset=US-ASCII; name=v4-0001-PG17-pg_restore-Fix-comment-handling-with-no-publicati.txtDownload
From 6d85d536c37cbce55fa07fe9300c15372ea48151 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 10:18:19 +0900
Subject: [PATCH v17 1/3] pg_restore: Fix comment handling with
--no-publications / --no-subscriptions.
Previously, pg_restore did not skip comments on publications or subscriptions
even when --no-publications or --no-subscriptions was specified. As a result,
it could issue COMMENT commands for objects that were never created,
causing those commands to fail.
This commit fixes the issue by ensuring that comments on publications and
subscriptions are also skipped when the corresponding options are used.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 14 ++++++++++
src/bin/pg_dump/t/002_pg_dump.pl | 41 ++++++++++++++++++++++++++++
2 files changed, 55 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index f830704cf37..b07ee7bb87d 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2960,6 +2960,20 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
if (ropt->no_comments && strcmp(te->desc, "COMMENT") == 0)
return 0;
+ /*
+ * If it's a comment on a publication or a subscription, maybe ignore it.
+ */
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 8d5b2e1dc45..cd69ae7479e 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -537,6 +537,29 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_subscriptions => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--file' => "$tempdir/no_subscriptions.sql",
+ '--no-subscriptions',
+ 'postgres',
+ ],
+ },
+ no_subscriptions_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.dump",
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.sql",
+ '--no-subscriptions',
+ "$tempdir/no_subscriptions_restore.dump",
+ ],
+ },
no_table_access_method => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -711,6 +734,8 @@ my %full_runs = (
no_large_objects => 1,
no_owner => 1,
no_privs => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
@@ -1532,6 +1557,10 @@ my %tests = (
regexp =>
qr/^COMMENT ON SUBSCRIPTION sub1 IS 'comment on subscription';/m,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1' => {
@@ -3016,6 +3045,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub1 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub1');\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'CREATE SUBSCRIPTION sub2' => {
@@ -3027,6 +3060,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub2 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub2', origin = none);\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'CREATE SUBSCRIPTION sub3' => {
@@ -3038,6 +3075,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub3 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub3');\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'ALTER PUBLICATION pub1 ADD TABLE test_table' => {
--
2.50.1
v4-0001-PG18-master-pg_restore-Fix-comment-handling-with-no-publicati.patchapplication/octet-stream; name=v4-0001-PG18-master-pg_restore-Fix-comment-handling-with-no-publicati.patchDownload
From 4066f2c3d17ba5ce09e158374650effde883d5a9 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 10:18:19 +0900
Subject: [PATCH v4 1/4] pg_restore: Fix comment handling with
--no-publications / --no-subscriptions.
Previously, pg_restore did not skip comments on publications or subscriptions
even when --no-publications or --no-subscriptions was specified. As a result,
it could issue COMMENT commands for objects that were never created,
causing those commands to fail.
This commit fixes the issue by ensuring that comments on publications and
subscriptions are also skipped when the corresponding options are used.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 14 +++++++++
src/bin/pg_dump/t/002_pg_dump.pl | 44 ++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 058b5d659ba..d64ce19b673 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3048,6 +3048,20 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
strcmp(te->desc, "ROW SECURITY") == 0))
return 0;
+ /*
+ * If it's a comment on a publication or a subscription, maybe ignore it.
+ */
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index e7a2d64f741..1e8c6bbf22b 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -650,6 +650,32 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_subscriptions => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--file' => "$tempdir/no_subscriptions.sql",
+ '--no-subscriptions',
+ '--statistics',
+ 'postgres',
+ ],
+ },
+ no_subscriptions_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.dump",
+ '--statistics',
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.sql",
+ '--no-subscriptions',
+ '--statistics',
+ "$tempdir/no_subscriptions_restore.dump",
+ ],
+ },
no_table_access_method => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -873,6 +899,8 @@ my %full_runs = (
no_policies => 1,
no_privs => 1,
no_statistics => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
@@ -1846,6 +1874,10 @@ my %tests = (
regexp =>
qr/^COMMENT ON SUBSCRIPTION sub1 IS 'comment on subscription';/m,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1' => {
@@ -3363,6 +3395,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub1 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub1', streaming = parallel);\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'CREATE SUBSCRIPTION sub2' => {
@@ -3374,6 +3410,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub2 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub2', streaming = off, origin = none);\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'CREATE SUBSCRIPTION sub3' => {
@@ -3385,6 +3425,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub3 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub3', streaming = on);\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
--
2.50.1
v4-0001-PG16-pg_restore-Fix-comment-handling-with-no-publicati.txttext/plain; charset=US-ASCII; name=v4-0001-PG16-pg_restore-Fix-comment-handling-with-no-publicati.txtDownload
From 4162391fc112756c91cde59fce728bed576ce05e Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 10:18:19 +0900
Subject: [PATCH v16 1/3] pg_restore: Fix comment handling with
--no-publications / --no-subscriptions.
Previously, pg_restore did not skip comments on publications or subscriptions
even when --no-publications or --no-subscriptions was specified. As a result,
it could issue COMMENT commands for objects that were never created,
causing those commands to fail.
This commit fixes the issue by ensuring that comments on publications and
subscriptions are also skipped when the corresponding options are used.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 14 +++++++++
src/bin/pg_dump/t/002_pg_dump.pl | 45 ++++++++++++++++++++++++++++
2 files changed, 59 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index b0ef1980360..93026a8ddc5 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2855,6 +2855,20 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
if (ropt->no_comments && strcmp(te->desc, "COMMENT") == 0)
return 0;
+ /*
+ * If it's a comment on a publication or a subscription, maybe ignore it.
+ */
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index b491e461158..5a69e58f217 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -548,6 +548,29 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_subscriptions => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--file' => "$tempdir/no_subscriptions.sql",
+ '--no-subscriptions',
+ 'postgres',
+ ],
+ },
+ no_subscriptions_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.dump",
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.sql",
+ '--no-subscriptions',
+ "$tempdir/no_subscriptions_restore.dump",
+ ],
+ },
no_table_access_method => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -722,6 +745,8 @@ my %full_runs = (
no_large_objects => 1,
no_owner => 1,
no_privs => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
@@ -1546,6 +1571,10 @@ my %tests = (
regexp =>
qr/^COMMENT ON SUBSCRIPTION sub1 IS 'comment on subscription';/m,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1' => {
@@ -2988,6 +3017,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub1 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub1');\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'CREATE SUBSCRIPTION sub2' => {
@@ -2999,6 +3032,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub2 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub2', origin = none);\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'CREATE SUBSCRIPTION sub3' => {
@@ -3010,6 +3047,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub3 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub3');\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'ALTER PUBLICATION pub1 ADD TABLE test_table' => {
@@ -3862,6 +3903,8 @@ my %tests = (
no_large_objects => 1,
no_privs => 1,
no_owner => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
only_dump_test_schema => 1,
pg_dumpall_dbprivs => 1,
@@ -3954,6 +3997,8 @@ my %tests = (
no_large_objects => 1,
no_privs => 1,
no_owner => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
--
2.50.1
v4-0001-PG15-pg_restore-Fix-comment-handling-with-no-publicati.txttext/plain; charset=US-ASCII; name=v4-0001-PG15-pg_restore-Fix-comment-handling-with-no-publicati.txtDownload
From 68e0ef94f5d1d4303296a0dcb647f082ef7c3461 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 10:18:19 +0900
Subject: [PATCH v15 1/3] pg_restore: Fix comment handling with
--no-publications / --no-subscriptions.
Previously, pg_restore did not skip comments on publications or subscriptions
even when --no-publications or --no-subscriptions was specified. As a result,
it could issue COMMENT commands for objects that were never created,
causing those commands to fail.
This commit fixes the issue by ensuring that comments on publications and
subscriptions are also skipped when the corresponding options are used.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 14 +++++++++++
src/bin/pg_dump/t/002_pg_dump.pl | 37 ++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index b81788f72cb..61beeea77cb 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2875,6 +2875,20 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
if (ropt->no_comments && strcmp(te->desc, "COMMENT") == 0)
return 0;
+ /*
+ * If it's a comment on a publication or a subscription, maybe ignore it.
+ */
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 70d2922db3d..bef145c3382 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -336,6 +336,29 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_subscriptions => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--file' => "$tempdir/no_subscriptions.sql",
+ '--no-subscriptions',
+ 'postgres',
+ ],
+ },
+ no_subscriptions_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.dump",
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.sql",
+ '--no-subscriptions',
+ "$tempdir/no_subscriptions_restore.dump",
+ ],
+ },
no_table_access_method => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -496,6 +519,8 @@ my %full_runs = (
no_blobs => 1,
no_owner => 1,
no_privs => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
@@ -1245,6 +1270,10 @@ my %tests = (
regexp =>
qr/^COMMENT ON SUBSCRIPTION sub1 IS 'comment on subscription';/m,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1' => {
@@ -2557,6 +2586,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub1 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub1');\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'ALTER PUBLICATION pub1 ADD TABLE test_table' => {
@@ -3251,6 +3284,8 @@ my %tests = (
no_blobs => 1,
no_privs => 1,
no_owner => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
only_dump_test_schema => 1,
pg_dumpall_dbprivs => 1,
@@ -3324,6 +3359,8 @@ my %tests = (
no_blobs => 1,
no_privs => 1,
no_owner => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
no_table_access_method => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
--
2.50.1
v4-0001-PG13-PG14-pg_restore-Fix-comment-handling-with-no-publicati.txttext/plain; charset=US-ASCII; name=v4-0001-PG13-PG14-pg_restore-Fix-comment-handling-with-no-publicati.txtDownload
From fc6a1f024b534331808214e3e73eb960b2eb8d8f Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 10:18:19 +0900
Subject: [PATCH v14 1/3] pg_restore: Fix comment handling with
--no-publications / --no-subscriptions.
Previously, pg_restore did not skip comments on publications or subscriptions
even when --no-publications or --no-subscriptions was specified. As a result,
it could issue COMMENT commands for objects that were never created,
causing those commands to fail.
This commit fixes the issue by ensuring that comments on publications and
subscriptions are also skipped when the corresponding options are used.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 14 +++++++++++
src/bin/pg_dump/t/002_pg_dump.pl | 37 ++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 6f2cd8c21ad..99c1e2d6f9a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2858,6 +2858,20 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
if (ropt->no_comments && strcmp(te->desc, "COMMENT") == 0)
return 0;
+ /*
+ * If it's a comment on a publication or a subscription, maybe ignore it.
+ */
+ if (strcmp(te->desc, "COMMENT") == 0)
+ {
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/*
* If it's a publication or a table part of a publication, maybe ignore
* it.
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index c38448153d2..09f6c230c47 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -268,6 +268,29 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_subscriptions => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--file' => "$tempdir/no_subscriptions.sql",
+ '--no-subscriptions',
+ 'postgres',
+ ],
+ },
+ no_subscriptions_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.dump",
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_subscriptions_restore.sql",
+ '--no-subscriptions',
+ "$tempdir/no_subscriptions_restore.dump",
+ ],
+ },
only_dump_test_schema => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -420,6 +443,8 @@ my %full_runs = (
no_blobs => 1,
no_owner => 1,
no_privs => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
schema_only => 1,);
@@ -1131,6 +1156,10 @@ my %tests = (
regexp =>
qr/^COMMENT ON SUBSCRIPTION sub1 IS 'comment on subscription';/m,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1' => {
@@ -2401,6 +2430,10 @@ my %tests = (
\QCREATE SUBSCRIPTION sub1 CONNECTION 'dbname=doesnotexist' PUBLICATION pub1 WITH (connect = false, slot_name = 'sub1');\E
/xm,
like => { %full_runs, section_post_data => 1, },
+ unlike => {
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
+ },
},
'ALTER PUBLICATION pub1 ADD TABLE test_table' => {
@@ -2969,6 +3002,8 @@ my %tests = (
no_blobs => 1,
no_privs => 1,
no_owner => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
only_dump_test_schema => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
@@ -3040,6 +3075,8 @@ my %tests = (
no_blobs => 1,
no_privs => 1,
no_owner => 1,
+ no_subscriptions => 1,
+ no_subscriptions_restore => 1,
pg_dumpall_dbprivs => 1,
pg_dumpall_exclude => 1,
role => 1,
--
2.50.1
v4-0004-pg_restore-Fix-security-label-handling-with-no-pu.patchapplication/octet-stream; name=v4-0004-pg_restore-Fix-security-label-handling-with-no-pu.patchDownload
From 78bc55f9120bc19753edebe855a48c2541d5420d Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 19:33:12 +0900
Subject: [PATCH v4 4/4] pg_restore: Fix security label handling with
--no-publications/subscriptions.
Previously, pg_restore did not skip security labels on publications or
subscriptions even when --no-publications or --no-subscriptions was specified.
As a result, it could issue SECURITY LABEL commands for objects that were
never created, causing those commands to fail.
This commit fixes the issue by ensuring that security labels on publications
and subscriptions are also skipped when the corresponding options are used.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 93814152a5f..59eaecb4ed7 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3081,6 +3081,21 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
if (ropt->no_security_labels && strcmp(te->desc, "SECURITY LABEL") == 0)
return 0;
+ /*
+ * If it's a security label on a publication or a subscription, maybe
+ * ignore it.
+ */
+ if (strcmp(te->desc, "SECURITY LABEL") == 0)
+ {
+ if (ropt->no_publications &&
+ strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
+ return 0;
+
+ if (ropt->no_subscriptions &&
+ strncmp(te->tag, "SUBSCRIPTION", strlen("SUBSCRIPTION")) == 0)
+ return 0;
+ }
+
/* If it's a subscription, maybe ignore it */
if (ropt->no_subscriptions && strcmp(te->desc, "SUBSCRIPTION") == 0)
return 0;
--
2.50.1
v4-0003-pg_dump-Fix-dumping-of-security-labels-on-subscri.patchapplication/octet-stream; name=v4-0003-pg_dump-Fix-dumping-of-security-labels-on-subscri.patchDownload
From 811728fe3a49c6354843718c5e34b55ae01da5bb Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 18:46:38 +0900
Subject: [PATCH v4 3/4] pg_dump: Fix dumping of security labels on
subscriptions and event triggers.
Previously, pg_dump could not dump security labels on subscriptions because
it queried pg_seclabel, while subscription labels are stored in pg_shseclabel.
This commit fixes the issue by making pg_dump query pg_seclabels,
which includes entries from both pg_seclabel and pg_shseclabel.
Querying pg_shseclabel directly would also work, but pg_seclabels
is simpler and sufficient.
In addition, pg_dump is updated to dump security labels on event triggers.
Backpatch to all supported versions.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 13
---
src/bin/pg_dump/pg_backup_archiver.c | 12 +++++++-----
src/bin/pg_dump/pg_dump.c | 7 ++++++-
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 2f92fce44f6..93814152a5f 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3325,12 +3325,14 @@ _tocEntryRestorePass(TocEntry *te)
return RESTORE_PASS_POST_ACL;
/*
- * Comments need to be emitted in the same pass as their parent objects.
- * ACLs haven't got comments, and neither do matview data objects, but
- * event triggers do. (Fortunately, event triggers haven't got ACLs, or
- * we'd need yet another weird special case.)
+ * Comments and security labels need to be emitted in the same pass as
+ * their parent objects. ACLs haven't got comments and security labels,
+ * and neither do matview data objects, but event triggers do.
+ * (Fortunately, event triggers haven't got ACLs, or we'd need yet another
+ * weird special case.)
*/
- if (strcmp(te->desc, "COMMENT") == 0 &&
+ if ((strcmp(te->desc, "COMMENT") == 0 ||
+ strcmp(te->desc, "SECURITY LABEL") == 0) &&
strncmp(te->tag, "EVENT TRIGGER ", 14) == 0)
return RESTORE_PASS_POST_ACL;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bea793456f9..20d2ee4767d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16709,7 +16709,7 @@ collectSecLabels(Archive *fout)
appendPQExpBufferStr(query,
"SELECT label, provider, classoid, objoid, objsubid "
- "FROM pg_catalog.pg_seclabel "
+ "FROM pg_catalog.pg_seclabels "
"ORDER BY classoid, objoid, objsubid");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -19419,6 +19419,11 @@ dumpEventTrigger(Archive *fout, const EventTriggerInfo *evtinfo)
NULL, evtinfo->evtowner,
evtinfo->dobj.catId, 0, evtinfo->dobj.dumpId);
+ if (evtinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
+ dumpSecLabel(fout, "EVENT TRIGGER", qevtname,
+ NULL, evtinfo->evtowner,
+ evtinfo->dobj.catId, 0, evtinfo->dobj.dumpId);
+
destroyPQExpBuffer(query);
destroyPQExpBuffer(delqry);
free(qevtname);
--
2.50.1
v4-0002-pg_restore-Fix-comment-handling-with-no-policies.patchapplication/octet-stream; name=v4-0002-pg_restore-Fix-comment-handling-with-no-policies.patchDownload
From 267d7275b6528d54076d43a2d7d8f9abe8c77e9e Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Sep 2025 16:29:53 +0900
Subject: [PATCH v4 2/4] pg_restore: Fix comment handling with --no-policies.
Previously, pg_restore did not skip comments on policies even when
--no-policies was specified. As a result, it could issue COMMENT commands
for policies that were never created, causing those commands to fail.
This commit fixes the issue by ensuring that comments on policies
are also skipped when --no-policies is used.
Backpatch to v18, where --no-policies was added in pg_restore.
Author: Jian He <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxHCt00pR9h51AVu6+yPD5J7JQn=7dQXxqacj0XyDhc-fA@mail.gmail.com
Backpatch-through: 18
---
src/bin/pg_dump/pg_backup_archiver.c | 7 ++++-
src/bin/pg_dump/t/002_pg_dump.pl | 46 ++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index d64ce19b673..2f92fce44f6 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3049,10 +3049,15 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
return 0;
/*
- * If it's a comment on a publication or a subscription, maybe ignore it.
+ * If it's a comment on a policy, a publication, or a subscription, maybe
+ * ignore it.
*/
if (strcmp(te->desc, "COMMENT") == 0)
{
+ if (ropt->no_policies &&
+ strncmp(te->tag, "POLICY", strlen("POLICY")) == 0)
+ return 0;
+
if (ropt->no_publications &&
strncmp(te->tag, "PUBLICATION", strlen("PUBLICATION")) == 0)
return 0;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 1e8c6bbf22b..fc5b9b52f80 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -632,6 +632,23 @@ my %pgdump_runs = (
'postgres',
],
},
+ no_policies_restore => {
+ dump_cmd => [
+ 'pg_dump', '--no-sync',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_policies_restore.dump",
+ '--statistics',
+ 'postgres',
+ ],
+ restore_cmd => [
+ 'pg_restore',
+ '--format' => 'custom',
+ '--file' => "$tempdir/no_policies_restore.sql",
+ '--no-policies',
+ '--statistics',
+ "$tempdir/no_policies_restore.dump",
+ ],
+ },
no_privs => {
dump_cmd => [
'pg_dump', '--no-sync',
@@ -897,6 +914,7 @@ my %full_runs = (
no_large_objects => 1,
no_owner => 1,
no_policies => 1,
+ no_policies_restore => 1,
no_privs => 1,
no_statistics => 1,
no_subscriptions => 1,
@@ -1540,6 +1558,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -1858,6 +1877,27 @@ my %tests = (
},
},
+ 'COMMENT ON POLICY p1' => {
+ create_order => 55,
+ create_sql => 'COMMENT ON POLICY p1 ON dump_test.test_table
+ IS \'comment on policy\';',
+ regexp =>
+ qr/^COMMENT ON POLICY p1 ON dump_test.test_table IS 'comment on policy';/m,
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ only_dump_test_table => 1,
+ section_post_data => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ exclude_test_table => 1,
+ no_policies => 1,
+ no_policies_restore => 1,
+ only_dump_measurement => 1,
+ },
+ },
+
'COMMENT ON PUBLICATION pub1' => {
create_order => 55,
create_sql => 'COMMENT ON PUBLICATION pub1
@@ -3224,6 +3264,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3246,6 +3287,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3268,6 +3310,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3290,6 +3333,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3312,6 +3356,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
@@ -3334,6 +3379,7 @@ my %tests = (
exclude_dump_test_schema => 1,
exclude_test_table => 1,
no_policies => 1,
+ no_policies_restore => 1,
only_dump_measurement => 1,
},
},
--
2.50.1
On Wed, Sep 3, 2025 at 7:50 PM Fujii Masao <masao.fujii@gmail.com> wrote:
02: make pg_dump dump security label for shared database objects, like
subscription, roles.As I understand it, shared objects like roles are handled by pg_dumpall,
which already dumps their security labels via pg_shseclabel.
Subscriptions are an exception: pg_dump dumps them (and should dump
their security labels), but those labels are stored in pg_shseclabel,
which pg_dump doesn't query.To fix this, making pg_dump query also pg_shseclabel when dumping
subscriptions would work. But your approach, having pg_dump query
pg_seclabels (covering both pg_seclabel and pg_shseclabel),
is simpler and sufficient. So I like your approach for now.I also noticed pg_dump didn't dump security labels on event triggers,
so I extended your patch as v4-0003 to handle those as well.
in _tocEntryRestorePass
if we do
if ((strcmp(te->desc, "COMMENT") == 0 ||
strcmp(te->desc, "SECURITY LABEL") == 0) &&
strncmp(te->tag, "EVENT TRIGGER ", 14) == 0)
return RESTORE_PASS_POST_ACL;
then RestorePass related comments also need to be adjusted for security label?
typedef enum
{
RESTORE_PASS_MAIN = 0, /* Main pass (most TOC item types) */
RESTORE_PASS_ACL, /* ACL item types */
RESTORE_PASS_POST_ACL, /* Event trigger and matview refresh items */
#define RESTORE_PASS_LAST RESTORE_PASS_POST_ACL
} RestorePass;
we do not support security label on extension, see SecLabelSupportsObjectType.
below the dumpExtension function code should be removed?
/* Dump Extension Comments and Security Labels */
if (extinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
dumpSecLabel(fout, "EXTENSION", qextname,
NULL, "",
extinfo->dobj.catId, 0, extinfo->dobj.dumpId);
On Thu, Sep 4, 2025 at 6:00 PM jian he <jian.universality@gmail.com> wrote:
in _tocEntryRestorePass
if we doif ((strcmp(te->desc, "COMMENT") == 0 ||
strcmp(te->desc, "SECURITY LABEL") == 0) &&
strncmp(te->tag, "EVENT TRIGGER ", 14) == 0)
return RESTORE_PASS_POST_ACL;then RestorePass related comments also need to be adjusted for security label?
Could you clarify which comments should be updated for SECURITY LABEL?
I don't see any references to COMMENT in RestorePass, so it doesn't look
like any updates are needed there for SECURITY LABEL either. But maybe
I'm missing something...
we do not support security label on extension, see SecLabelSupportsObjectType.
below the dumpExtension function code should be removed?/* Dump Extension Comments and Security Labels */
if (extinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
dumpSecLabel(fout, "EXTENSION", qextname,
NULL, "",
extinfo->dobj.catId, 0, extinfo->dobj.dumpId);
Good catch! This code was introduced in commit d9572c4e3b4,
which added core support for extensions. Since security labels on
extensions are not supported, I agree that the code should be
removed in master.
Regards,
--
Fujii Masao
On Tue, Sep 9, 2025 at 12:00 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Sep 4, 2025 at 6:00 PM jian he <jian.universality@gmail.com> wrote:
in _tocEntryRestorePass
if we doif ((strcmp(te->desc, "COMMENT") == 0 ||
strcmp(te->desc, "SECURITY LABEL") == 0) &&
strncmp(te->tag, "EVENT TRIGGER ", 14) == 0)
return RESTORE_PASS_POST_ACL;then RestorePass related comments also need to be adjusted for security label?
Could you clarify which comments should be updated for SECURITY LABEL?
I don't see any references to COMMENT in RestorePass, so it doesn't look
like any updates are needed there for SECURITY LABEL either. But maybe
I'm missing something...
typedef enum
{
RESTORE_PASS_MAIN = 0, /* Main pass (most TOC item types) */
RESTORE_PASS_ACL, /* ACL item types */
RESTORE_PASS_POST_ACL, /* Event trigger and matview refresh items */
#define RESTORE_PASS_LAST RESTORE_PASS_POST_ACL
} RestorePass;
I thought we needed to change the comments:
" /* Event trigger and matview refresh items */".
looking at the code again, we don't need to do that.
Overall, all the patches look good to me.
(I didn't test back branch related tests though)
On Tue, Sep 9, 2025 at 2:50 PM jian he <jian.universality@gmail.com> wrote:
On Tue, Sep 9, 2025 at 12:00 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Sep 4, 2025 at 6:00 PM jian he <jian.universality@gmail.com> wrote:
in _tocEntryRestorePass
if we doif ((strcmp(te->desc, "COMMENT") == 0 ||
strcmp(te->desc, "SECURITY LABEL") == 0) &&
strncmp(te->tag, "EVENT TRIGGER ", 14) == 0)
return RESTORE_PASS_POST_ACL;then RestorePass related comments also need to be adjusted for security label?
Could you clarify which comments should be updated for SECURITY LABEL?
I don't see any references to COMMENT in RestorePass, so it doesn't look
like any updates are needed there for SECURITY LABEL either. But maybe
I'm missing something...typedef enum
{
RESTORE_PASS_MAIN = 0, /* Main pass (most TOC item types) */
RESTORE_PASS_ACL, /* ACL item types */
RESTORE_PASS_POST_ACL, /* Event trigger and matview refresh items */#define RESTORE_PASS_LAST RESTORE_PASS_POST_ACL
} RestorePass;I thought we needed to change the comments:
" /* Event trigger and matview refresh items */".looking at the code again, we don't need to do that.
Overall, all the patches look good to me.
(I didn't test back branch related tests though)
Thanks for the review! Unless there are any objections,
I’ll run the tests again and then push the patches.
Regards,
--
Fujii Masao
On Tue, Sep 9, 2025 at 4:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
Thanks for the review! Unless there are any objections,
I’ll run the tests again and then push the patches.
I've pushed patches 0001, 0002, and 0003. I'll push 0004 later.
Buildfarm member petalura reported a failure at [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2025-09-16%2003%3A29%3A05 after 0001 was backpatched
to v13. However, the server logs show a "double free or corruption" error
occurring as the backend was exiting. Since that code was not touched by 0001,
for now I don't think this failure is related to the patch...
But am I missing something?
----------------------------------------
2025-09-16 05:47:21.231 CEST [226419][client backend][10/2179:0] LOG:
statement: RESET client_min_messages;
2025-09-16 05:47:21.231 CEST [226419][client backend][10/2180:0] LOG:
statement: RESET search_path;
2025-09-16 05:47:21.244 CEST [226419][client backend][:0] LOG:
disconnection: session time: 0:00:35.551 user=bf database=regression
host=[local]
double free or corruption (!prev)
<snip>
2025-09-16 05:47:21.550 CEST [10340][postmaster][:0] LOG: server
process (PID 226419) was terminated by signal 6: Aborted
2025-09-16 05:47:21.550 CEST [10340][postmaster][:0] LOG: terminating
any other active server processes
----------------------------------------
Regards,
[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2025-09-16%2003%3A29%3A05
--
Fujii Masao
On Tue, Sep 16, 2025 at 5:04 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Sep 9, 2025 at 4:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
Thanks for the review! Unless there are any objections,
I’ll run the tests again and then push the patches.I've pushed patches 0001, 0002, and 0003. I'll push 0004 later.
I've pushed 0004, thanks!
Buildfarm member petalura reported a failure at [1] after 0001 was backpatched
to v13. However, the server logs show a "double free or corruption" error
occurring as the backend was exiting. Since that code was not touched by 0001,
for now I don't think this failure is related to the patch...
But am I missing something?
After pushing 0004, petalura reported another failure with a "double
free or corruption" error:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2025-09-18%2002%3A14%3A32
Buildfarm member phycodurus also reported similar failures:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-09-16%2011%3A09%3A07
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-09-16%2016%3A44%3A37
I'll dig deeper into this issue.
As for the unnecessary code for security labels on extensions
you mentioned earlier, I've created a patch to remove it. Patch attached.
Regards,
--
Fujii Masao
Attachments:
v1-0001-pg_dump-Remove-unnecessary-code-for-security-labe.patchapplication/octet-stream; name=v1-0001-pg_dump-Remove-unnecessary-code-for-security-labe.patchDownload
From 32d94104ca12c57912afe0706990c4fc3f45c3b7 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Thu, 18 Sep 2025 11:27:45 +0900
Subject: [PATCH v1] pg_dump: Remove unnecessary code for security labels on
extensions.
Commit d9572c4e3b4 added extension support and made pg_dump attempt to
dump security labels on extensions. However, security labels on extensions
are not actually supported, so this code was unnecessary.
This commit removes it.
Suggested-by: Jian He <jian.universality@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACJufxF8=z0v=888NKKEoTHQ+Jc4EXutFi91BF0fFjgFsZT6JQ@mail.gmail.com
---
src/bin/pg_dump/pg_dump.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 9fc3671cb35..211e29a159a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -12008,17 +12008,12 @@ dumpExtension(Archive *fout, const ExtensionInfo *extinfo)
.createStmt = q->data,
.dropStmt = delq->data));
- /* Dump Extension Comments and Security Labels */
+ /* Dump Extension Comments */
if (extinfo->dobj.dump & DUMP_COMPONENT_COMMENT)
dumpComment(fout, "EXTENSION", qextname,
NULL, "",
extinfo->dobj.catId, 0, extinfo->dobj.dumpId);
- if (extinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
- dumpSecLabel(fout, "EXTENSION", qextname,
- NULL, "",
- extinfo->dobj.catId, 0, extinfo->dobj.dumpId);
-
free(qextname);
destroyPQExpBuffer(q);
--
2.50.1
On Thu, Sep 18, 2025 at 11:03 AM Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Sep 16, 2025 at 5:04 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Sep 9, 2025 at 4:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
Thanks for the review! Unless there are any objections,
I’ll run the tests again and then push the patches.I've pushed patches 0001, 0002, and 0003. I'll push 0004 later.
I've pushed 0004, thanks!
Buildfarm member petalura reported a failure at [1] after 0001 was backpatched
to v13. However, the server logs show a "double free or corruption" error
occurring as the backend was exiting. Since that code was not touched by 0001,
for now I don't think this failure is related to the patch...
But am I missing something?After pushing 0004, petalura reported another failure with a "double
free or corruption" error:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2025-09-18%2002%3A14%3A32Buildfarm member phycodurus also reported similar failures:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-09-16%2011%3A09%3A07
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-09-16%2016%3A44%3A37I'll dig deeper into this issue.
It appears that this issue no longer exists, per https://bfbot.cputube.org
As for the unnecessary code for security labels on extensions
you mentioned earlier, I've created a patch to remove it. Patch attached.
looks good to me.
Hello Jian,
16.10.2025 17:22, jian he пишет:
Buildfarm member petalura reported a failure at [1] after 0001 was backpatched
to v13. However, the server logs show a "double free or corruption" error
occurring as the backend was exiting. Since that code was not touched by 0001,
for now I don't think this failure is related to the patch...
But am I missing something?After pushing 0004, petalura reported another failure with a "double
free or corruption" error:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2025-09-18%2002%3A14%3A32Buildfarm member phycodurus also reported similar failures:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-09-16%2011%3A09%3A07
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-09-16%2016%3A44%3A37I'll dig deeper into this issue.
It appears that this issue no longer exists, perhttps://bfbot.cputube.org
Unfortunately, buildfarm animals (especially phycodurus) are still
failing due to "double free or corruption (!prev)":
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-10-16%2019%3A07%3A52
I can try to reproduce this locally if it can be helpful.
Best regards,
Alexander
On Thu, Oct 16, 2025 at 11:23 PM jian he <jian.universality@gmail.com> wrote:
As for the unnecessary code for security labels on extensions
you mentioned earlier, I've created a patch to remove it. Patch attached.looks good to me.
Thanks for the review! Unless there are any objections, I'll commit the patch.
By the way, while reading the documentation about security labels and
extensions, I noticed this section:
https://www.postgresql.org/docs/devel/extend-extensions.html
PostgreSQL does not currently support extension scripts issuing CREATE POLICY
or SECURITY LABEL statements. These are expected to be set after
the extension has been created. All RLS policies and security labels on
extension objects will be included in dumps created by pg_dump.
I'm not sure the last sentence is accurate - in my quick test, a security label
on the pgstattuple function wasn't included in the dump. If that's correct,
should we update this part of the documentation, as a separate patch?
------------------------
$ psql
=# CREATE EXTENSION dummy_seclabel ;
=# CREATE EXTENSION pgstattuple ;
=# SECURITY LABEL ON FUNCTION pgstattuple(regclass) IS 'classified';
=# \q
$ pg_dump | grep -i "security label"
COMMENT ON EXTENSION dummy_seclabel IS 'Test code for SECURITY LABEL feature';
------------------------
In the above example, SECURITY LABEL command for pgstattuple function
was not included in the dump.
Regards,
--
Fujii Masao
On Fri, Oct 17, 2025 at 2:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
Unfortunately, buildfarm animals (especially phycodurus) are still
failing due to "double free or corruption (!prev)":
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus&dt=2025-10-16%2019%3A07%3A52
Yeah... A new thread [1]/messages/by-id/aPAl27_urHSODwRN@paquier.xyz has been started about this issue. I'll post
any findings there.
[1]: /messages/by-id/aPAl27_urHSODwRN@paquier.xyz
/messages/by-id/aPAl27_urHSODwRN@paquier.xyz
I can try to reproduce this locally if it can be helpful.
Yes, that's really helpful.
Regards,
--
Fujii Masao
On Fri, Oct 17, 2025 at 7:23 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Oct 16, 2025 at 11:23 PM jian he <jian.universality@gmail.com> wrote:
As for the unnecessary code for security labels on extensions
you mentioned earlier, I've created a patch to remove it. Patch attached.looks good to me.
Thanks for the review! Unless there are any objections, I'll commit the patch.
I've pushed the patch. Thanks!
Regards,
--
Fujii Masao