Thread: 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.
Attachment
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 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