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&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
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&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.
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&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
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&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?
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 = &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). 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 = &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
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, acl_co
Re: Dump public schema ownership & seclabels
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. 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, acl_column, @@ -807,12 +810,13 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery, printfPQExpBuffer(init_acl_subquery, "CASE WHEN privtype = 'e' THEN "
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.