Re: Dump public schema ownership & seclabels
Noah Misch writes: > On Tue, Jun 29, 2021 at 01:53:42AM -0400, Tom Lane wrote: >> Noah Misch writes: >>> Done. This upset one buildfarm member so far: >>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur=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
Re: Dump public schema ownership & seclabels
On Tue, Jun 29, 2021 at 01:53:42AM -0400, Tom Lane wrote: > Noah Misch writes: > > Done. This upset one buildfarm member so far: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur=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.
Re: Dump public schema ownership & seclabels
Noah Misch writes: > Done. This upset one buildfarm member so far: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gaur=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
Re: Dump public schema ownership & seclabels
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 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=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?
Re: Dump public schema ownership & seclabels
On Sat, May 01, 2021 at 09:43:36AM -0700, Zhihong Yu wrote:
> On Sat, May 1, 2021 at 8:16 AM Asif Rehman 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 = _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).
Author: Noah Misch
Commit: Noah Misch
Dump public schema ownership and security labels.
As a side effect, this corrects dumps of public schema ACLs in databases
where the DBA dropped and recreated that schema.
Reviewed by Asif Rehman.
Discussion: https://postgr.es/m/20201229134924.ga1431...@rfd.leadboat.com
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 60d306e..ea67e52 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -725,6 +725,7 @@ void
buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
PQExpBuffer init_acl_subquery, PQExpBuffer
init_racl_subquery,
const char *acl_column, const char *acl_owner,
+ const char *initprivs_expr,
const char *obj_kind, bool binary_upgrade)
{
/*
@@ -765,23 +766,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer
racl_subquery,
"WITH ORDINALITY AS perm(acl,row_n) "
"WHERE NOT EXISTS ( "
"SELECT 1 FROM "
-
"pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+
"pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
"AS init(init_acl) WHERE acl =
init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
+ initprivs_expr,
obj_kind,
acl_owner);
printfPQExpBuffer(racl_subquery,
"(SELECT pg_catalog.array_agg(acl
ORDER BY row_n) FROM "
"(SELECT acl, row_n FROM "
-
"pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+
"pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
"WITH ORDINALITY AS initp(acl,row_n) "
"WHERE NOT EXISTS ( "
"SELECT 1 FROM "
"pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
"AS permp(orig_acl) WHERE acl =
orig_acl)) as foo)",
+ initprivs_expr,
obj_kind,
acl_owner,
acl_column,
@@ -807,12 +810,13 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer
racl_subquery,
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e'
THEN "
"(SELECT
pg_catalog.array_agg(acl ORDER BY row_n) FROM "
- "(SELECT acl, row_n FROM
pg_catalog.unnest(pip.initprivs) "
+
Re: Dump public schema ownership & seclabels
On Sat, May 1, 2021 at 8:16 AM Asif Rehman 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 = _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
Re: Dump public schema ownership & seclabels
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
Re: Dump public schema ownership & seclabels
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.gb4039...@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.
Author: Noah Misch
Commit: Noah Misch
Dump public schema ownership and security labels.
As a side effect, this corrects dumps of public schema ACLs in databases
where the DBA dropped and recreated that schema.
Reviewed by FIXME.
Discussion: https://postgr.es/m/20201229134924.ga1431...@rfd.leadboat.com
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 60d306e..ea67e52 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -725,6 +725,7 @@ void
buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
PQExpBuffer init_acl_subquery, PQExpBuffer
init_racl_subquery,
const char *acl_column, const char *acl_owner,
+ const char *initprivs_expr,
const char *obj_kind, bool binary_upgrade)
{
/*
@@ -765,23 +766,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer
racl_subquery,
"WITH ORDINALITY AS perm(acl,row_n) "
"WHERE NOT EXISTS ( "
"SELECT 1 FROM "
-
"pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+
"pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
"AS init(init_acl) WHERE acl =
init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
+ initprivs_expr,
obj_kind,
acl_owner);
printfPQExpBuffer(racl_subquery,
"(SELECT pg_catalog.array_agg(acl
ORDER BY row_n) FROM "
"(SELECT acl, row_n FROM "
-
"pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+
"pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
"WITH ORDINALITY AS initp(acl,row_n) "
"WHERE NOT EXISTS ( "
"SELECT 1 FROM "
"pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
"AS permp(orig_acl) WHERE acl =
orig_acl)) as foo)",
+ initprivs_expr,
obj_kind,
acl_owner,
Re: Dump public schema ownership & seclabels
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.gb4039...@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
Re: Dump public schema ownership & seclabels
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.gb4039...@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?
Re: Dump public schema ownership & seclabels
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.gb4039...@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
Re: Dump public schema ownership & seclabels
On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote: > https://postgr.es/m/20201031163518.gb4039...@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.
Dump public schema ownership & seclabels
https://postgr.es/m/20201031163518.gb4039...@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
Author: Noah Misch
Commit: Noah Misch
Dump public schema ownership and security labels.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 1f82c64..a16d149 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3597,10 +3597,13 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool
isData)
* Actually print the definition.
*
* Really crude hack for suppressing AUTHORIZATION clause that old
pg_dump
-* versions put into CREATE SCHEMA. We have to do this when --no-owner
-* mode is selected. This is ugly, but I see no other good way ...
+* versions put into CREATE SCHEMA. Don't mutate the modern definition
+* for schema "public", which consists of a comment. We have to do this
+* when --no-owner mode is selected. This is ugly, but I see no other
+* good way ...
*/
- if (ropt->noOwner && strcmp(te->desc, "SCHEMA") == 0)
+ if (ropt->noOwner &&
+ strcmp(te->desc, "SCHEMA") == 0 && strncmp(te->defn, "--", 2)
!= 0)
{
ahprintf(AH, "CREATE SCHEMA %s;\n\n\n", fmtId(te->tag));
}
@@ -3612,11 +3615,16 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool
isData)
/*
* If we aren't using SET SESSION AUTH to determine ownership, we must
-* instead issue an ALTER OWNER command. We assume that anything
without
-* a DROP command is not a separately ownable object. All the
categories
-* with DROP commands must appear in one list or the other.
+* instead issue an ALTER OWNER command. Schema "public" is special; a
+* dump never creates it, so we use ALTER OWNER even when using SET
+* SESSION for all other objects. We assume that anything without a
DROP
+* command is not a separately ownable object. All the categories with
+* DROP commands must appear in one list or the other.
*/
- if (!ropt->noOwner && !ropt->use_setsessauth &&
+ if (!ropt->noOwner &&
+ (!ropt->use_setsessauth ||
+(strcmp(te->tag, "public") == 0
+ && strcmp(te->desc, "SCHEMA") == 0)) &&
te->owner && strlen(te->owner) > 0 &&
te->dropStmt && strlen(te->dropStmt) > 0)
{
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1ab98a2..c633644 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -44,6 +44,7 @@
#include "catalog/pg_aggregate_d.h"
#include "catalog/pg_am_d.h"
#include "catalog/pg_attribute_d.h"
+#include "catalog/pg_authid_d.h"
#include "catalog/pg_cast_d.h"
#include "catalog/pg_class_d.h"
#include "catalog/pg_collation_d.h"
@@ -1566,10 +1567,14 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive
*fout)
* no-mans-land between being a system object and a user
object. We
* don't want to dump creation or comment commands for it,
because
* that complicates matters for non-superuser use of pg_dump.
But we
-* should dump any ACL changes that have occurred for it, and of
-* course we should dump contained objects.
+* should dump any ownership changes, security labels, and ACL
+* changes, and of course we should dump contained objects.
pg_dump
+* ties ownership to DUMP_COMPONENT_DEFINITION, so dump a
"definition"
+* bearing just a comment.
*/
- nsinfo->dobj.dump = DUMP_COMPONENT_ACL;
+ nsinfo->dobj.dump = DUMP_COMPONENT_ALL &
~DUMP_COMPONENT_COMMENT;
+ if (nsinfo->nspowner == BOOTSTRAP_SUPERUSERID)
+ nsinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;
nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
}
else
@@ -4748,6 +4753,7 @@ getNamespaces(Archive *fout, int *numNamespaces)
int i_tableoid;
int
