Thread: Dump public schema ownership & seclabels

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

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.

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

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?

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

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.

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.

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

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).

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?

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

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.

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