Обсуждение: 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.

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

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

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

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

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