Horton

Thread

Commits

  1. Remove literal backslash from Perl \Q ... \E.

  2. Dump COMMENT ON SCHEMA public.

  3. Dump public schema ownership and security labels.

  1. Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2020-12-29T13:49:24Z

    https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
one of the constituent projects for changing the public schema default ACL.
This ended up being simple.  Attached.  I chose to omit the "ALTER SCHEMA
public OWNER TO" when the owner is the bootstrap superuser, like how we skip
acl GRANT/REVOKE when the ACL matches the one recorded in pg_init_privs.  I
waffled on that; would it be better to make the OWNER TO unconditional?

Like ownership, we've not been dumping security labels on the public schema.
The way I fixed ownership fixed security labels implicitly.  If anyone thinks
I should unbundle these two, let me know.

All this is arguably a fix for an ancient bug.  Some sites may need to
compensate for the behavior change, so I plan not to back-patch.

Thanks,
nm

  2. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2020-12-30T11:59:02Z

    On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
> https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
> one of the constituent projects for changing the public schema default ACL.
> This ended up being simple.  Attached.

This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
pg_write_server_files;".  I will try again later.

  3. Re: Dump public schema ownership & seclabels

    Vik Fearing <vik@postgresfriends.org> — 2021-01-16T13:05:43Z

    On 12/30/20 12:59 PM, Noah Misch wrote:
> On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
>> https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
>> one of the constituent projects for changing the public schema default ACL.
>> This ended up being simple.  Attached.
> 
> This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
> OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
> pg_write_server_files;".  I will try again later.

Could I ask you to also include COMMENTs when you try again, please?
-- 
Vik Fearing

  4. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2021-01-17T09:41:02Z

    On Sat, Jan 16, 2021 at 02:05:43PM +0100, Vik Fearing wrote:
> On 12/30/20 12:59 PM, Noah Misch wrote:
> > On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
> >> https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
> >> one of the constituent projects for changing the public schema default ACL.
> >> This ended up being simple.  Attached.
> > 
> > This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
> > OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
> > pg_write_server_files;".  I will try again later.
> 
> Could I ask you to also include COMMENTs when you try again, please?

That may work.  I had not expected to hear of a person changing the comment on
schema public.  To what do you change it?

  5. Re: Dump public schema ownership & seclabels

    Vik Fearing <vik@postgresfriends.org> — 2021-01-17T11:00:06Z

    On 1/17/21 10:41 AM, Noah Misch wrote:
> On Sat, Jan 16, 2021 at 02:05:43PM +0100, Vik Fearing wrote:
>> On 12/30/20 12:59 PM, Noah Misch wrote:
>>> On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
>>>> https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
>>>> one of the constituent projects for changing the public schema default ACL.
>>>> This ended up being simple.  Attached.
>>>
>>> This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
>>> OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
>>> pg_write_server_files;".  I will try again later.
>>
>> Could I ask you to also include COMMENTs when you try again, please?
> 
> That may work.  I had not expected to hear of a person changing the comment on
> schema public.  To what do you change it?

It was a while ago and I don't remember because it didn't appear in the
dump so I stopped doing it. :(

Mine was an actual comment, but there are some tools out there that
(ab)use COMMENTs as crude metadata for what they do.  For example:
https://postgresql-anonymizer.readthedocs.io/en/stable/declare_masking_rules/#declaring-rules-with-comments
-- 
Vik Fearing

  6. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2021-02-11T12:08:34Z

    On Sun, Jan 17, 2021 at 12:00:06PM +0100, Vik Fearing wrote:
> On 1/17/21 10:41 AM, Noah Misch wrote:
> > On Sat, Jan 16, 2021 at 02:05:43PM +0100, Vik Fearing wrote:
> >> On 12/30/20 12:59 PM, Noah Misch wrote:
> >>> On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
> >>>> https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
> >>>> one of the constituent projects for changing the public schema default ACL.
> >>>> This ended up being simple.  Attached.
> >>>
> >>> This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
> >>> OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
> >>> pg_write_server_files;".  I will try again later.

Fixed.  The comment added to getNamespaces() explains what went wrong.

Incidentally, --no-acl is fragile without --no-owner, because any REVOKE
statements assume a particular owner.  Since one can elect --no-owner at
restore time, we can't simply adjust the REVOKE statements constructed at dump
time.  That's not new with this patch or specific to initdb-created objects.

> >> Could I ask you to also include COMMENTs when you try again, please?
> > 
> > That may work.  I had not expected to hear of a person changing the comment on
> > schema public.  To what do you change it?
> 
> It was a while ago and I don't remember because it didn't appear in the
> dump so I stopped doing it. :(
> 
> Mine was an actual comment, but there are some tools out there that
> (ab)use COMMENTs as crude metadata for what they do.  For example:
> https://postgresql-anonymizer.readthedocs.io/en/stable/declare_masking_rules/#declaring-rules-with-comments

I've attached a separate patch for this, which applies atop the ownership
patch.  This makes more restores fail for non-superusers, which is okay.

  7. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2021-02-12T14:25:30Z

    On Thu, Feb 11, 2021 at 04:08:34AM -0800, Noah Misch wrote:
> On Sun, Jan 17, 2021 at 12:00:06PM +0100, Vik Fearing wrote:
> > On 1/17/21 10:41 AM, Noah Misch wrote:
> > > On Sat, Jan 16, 2021 at 02:05:43PM +0100, Vik Fearing wrote:
> > >> On 12/30/20 12:59 PM, Noah Misch wrote:
> > >>> On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
> > >>>> https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com gave $SUBJECT as
> > >>>> one of the constituent projects for changing the public schema default ACL.
> > >>>> This ended up being simple.  Attached.
> > >>>
> > >>> This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
> > >>> OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
> > >>> pg_write_server_files;".  I will try again later.
> 
> Fixed.  The comment added to getNamespaces() explains what went wrong.
> 
> Incidentally, --no-acl is fragile without --no-owner, because any REVOKE
> statements assume a particular owner.  Since one can elect --no-owner at
> restore time, we can't simply adjust the REVOKE statements constructed at dump
> time.  That's not new with this patch or specific to initdb-created objects.
> 
> > >> Could I ask you to also include COMMENTs when you try again, please?
> > > 
> > > That may work.  I had not expected to hear of a person changing the comment on
> > > schema public.  To what do you change it?
> > 
> > It was a while ago and I don't remember because it didn't appear in the
> > dump so I stopped doing it. :(
> > 
> > Mine was an actual comment, but there are some tools out there that
> > (ab)use COMMENTs as crude metadata for what they do.  For example:
> > https://postgresql-anonymizer.readthedocs.io/en/stable/declare_masking_rules/#declaring-rules-with-comments
> 
> I've attached a separate patch for this, which applies atop the ownership
> patch.  This makes more restores fail for non-superusers, which is okay.

Oops, I botched a refactoring late in the development of that patch.  Here's a
fixed pair of patches.

  8. Re: Dump public schema ownership & seclabels

    Asif Rehman <asifr.rehman@gmail.com> — 2021-05-01T15:15:58Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Hi,

I have tested this patch. This patch still applies and it works well.

Regards,
Asif

The new status of this patch is: Ready for Committer

  9. Re: Dump public schema ownership & seclabels

    Zhihong Yu <zyu@yugabyte.com> — 2021-05-01T16:43:36Z

    On Sat, May 1, 2021 at 8:16 AM Asif Rehman <asifr.rehman@gmail.com> wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            not tested
>
> Hi,
>
> I have tested this patch. This patch still applies and it works well.
>
> Regards,
> Asif
>
> The new status of this patch is: Ready for Committer
>

For public-schema-comment-dump-v2.patch :

+       if (ncomments == 0)
+       {
+           comments = &empty_comment;
+           ncomments = 1;
+       }
+       else if (strcmp(comments->descr, (fout->remoteVersion >= 80300 ?
+                                         "standard public schema" :
+                                         "Standard public schema")) == 0)
+       {
+           ncomments = 0;

Is it possible that, in the case ncomments > 0, there are more than one
comment ?
If not, an assertion can be added in the second if block above.

Cheers

  10. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2021-05-03T05:57:47Z

    On Sat, May 01, 2021 at 09:43:36AM -0700, Zhihong Yu wrote:
> On Sat, May 1, 2021 at 8:16 AM Asif Rehman <asifr.rehman@gmail.com> wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, passed
> > Implements feature:       tested, passed
> > Spec compliant:           tested, passed
> > Documentation:            not tested
> >
> > Hi,
> >
> > I have tested this patch. This patch still applies and it works well.
> >
> > Regards,
> > Asif
> >
> > The new status of this patch is: Ready for Committer

Thanks.  Later, I saw that "pg_dump --schema=public" traditionally has yielded
"CREATE SCHEMA public" and "COMMENT ON SCHEMA public".  I've updated the
patches to preserve that behavior.

I'll push this when v15 branches.  I do think it's a bug fix and could argue
for including it in v14.  On the other hand, I mailed three total patch
versions now known to be wrong, so it would be imprudent to count on no
surprises remaining.

> For public-schema-comment-dump-v2.patch :
> 
> +       if (ncomments == 0)
> +       {
> +           comments = &empty_comment;
> +           ncomments = 1;
> +       }
> +       else if (strcmp(comments->descr, (fout->remoteVersion >= 80300 ?
> +                                         "standard public schema" :
> +                                         "Standard public schema")) == 0)
> +       {
> +           ncomments = 0;
> 
> Is it possible that, in the case ncomments > 0, there are more than one
> comment ?

Yes, I think that's normal when the search terms include an objsubid (subid !=
InvalidOid).

  11. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2021-06-29T05:36:27Z

    On Sun, May 02, 2021 at 10:57:47PM -0700, Noah Misch wrote:
> On Sat, May 01, 2021 at 09:43:36AM -0700, Zhihong Yu wrote:
> > On Sat, May 1, 2021 at 8:16 AM Asif Rehman <asifr.rehman@gmail.com> wrote:
> > > The new status of this patch is: Ready for Committer

> I'll push this when v15 branches.

Done.  This upset one buildfarm member so far:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur&dt=2021-06-29%2001%3A43%3A14

I don't know what happened there.  Tom, could you post a tar of the
src/bin/pg_dump/tmp_check/tmp_test_* directory after a failed "make -C
src/bin/pg_dump check" on that machine?

  12. Re: Dump public schema ownership & seclabels

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-29T05:53:42Z

    Noah Misch <noah@leadboat.com> writes:
> Done.  This upset one buildfarm member so far:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur&dt=2021-06-29%2001%3A43%3A14

> I don't know what happened there.  Tom, could you post a tar of the
> src/bin/pg_dump/tmp_check/tmp_test_* directory after a failed "make -C
> src/bin/pg_dump check" on that machine?

I'm too tired to look at it right now, but remembering that that's
running an old Perl version, I wonder if there's some Perl
incompatibility here.

			regards, tom lane

  13. Re: Dump public schema ownership & seclabels

    Noah Misch <noah@leadboat.com> — 2021-06-29T07:49:06Z

    On Tue, Jun 29, 2021 at 01:53:42AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > Done.  This upset one buildfarm member so far:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur&dt=2021-06-29%2001%3A43%3A14
> 
> > I don't know what happened there.  Tom, could you post a tar of the
> > src/bin/pg_dump/tmp_check/tmp_test_* directory after a failed "make -C
> > src/bin/pg_dump check" on that machine?
> 
> I'm too tired to look at it right now, but remembering that that's
> running an old Perl version, I wonder if there's some Perl
> incompatibility here.

That's at least part of the problem, based on experiments on a machine with
Perl 5.8.4.  That machine can't actually build PostgreSQL.  I've pushed a
necessary fix, though I'm only about 80% confident about it being sufficient.

  14. Re: Dump public schema ownership & seclabels

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-29T16:32:08Z

    Noah Misch <noah@leadboat.com> writes:
> On Tue, Jun 29, 2021 at 01:53:42AM -0400, Tom Lane wrote:
>> Noah Misch <noah@leadboat.com> writes:
>>> Done.  This upset one buildfarm member so far:
>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur&dt=2021-06-29%2001%3A43%3A14

>> I'm too tired to look at it right now, but remembering that that's
>> running an old Perl version, I wonder if there's some Perl
>> incompatibility here.

> That's at least part of the problem, based on experiments on a machine with
> Perl 5.8.4.  That machine can't actually build PostgreSQL.  I've pushed a
> necessary fix, though I'm only about 80% confident about it being sufficient.

gaur is still plugging away on a new run, but it got past the
pg_dump-check step, so I think you're good.

prairiedog has a similar-vintage Perl, so likely it would have shown the
problem too; but it's slow enough that it never saw the intermediate state
between these commits.

			regards, tom lane