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 <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
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 i_oid;
int i_nspname;
+ int i_nspowner;
int i_rolname;
int i_nspacl;
int i_rnspacl;
@@ -4772,6 +4778,7 @@ getNamespaces(Archive *fout, int *numNamespaces)
dopt->binary_upgrade);
appendPQExpBuffer(query, "SELECT n.tableoid, n.oid, n.nspname, "
+ "n.nspowner, "
"(%s nspowner) AS rolname, "
"%s as nspacl, "
"%s as rnspacl, "
@@ -4796,7 +4803,7 @@ getNamespaces(Archive *fout, int *numNamespaces)
destroyPQExpBuffer(init_racl_subquery);
}
else
- appendPQExpBuffer(query, "SELECT tableoid, oid, nspname, "
+ appendPQExpBuffer(query, "SELECT tableoid, oid, nspname,
nspowner, "
"(%s nspowner) AS rolname, "
"nspacl, NULL as rnspacl, "
"NULL AS initnspacl, NULL as
initrnspacl "
@@ -4812,6 +4819,7 @@ getNamespaces(Archive *fout, int *numNamespaces)
i_tableoid = PQfnumber(res, "tableoid");
i_oid = PQfnumber(res, "oid");
i_nspname = PQfnumber(res, "nspname");
+ i_nspowner = PQfnumber(res, "nspowner");
i_rolname = PQfnumber(res, "rolname");
i_nspacl = PQfnumber(res, "nspacl");
i_rnspacl = PQfnumber(res, "rnspacl");
@@ -4825,6 +4833,7 @@ getNamespaces(Archive *fout, int *numNamespaces)
nsinfo[i].dobj.catId.oid = atooid(PQgetvalue(res, i, i_oid));
AssignDumpId(&nsinfo[i].dobj);
nsinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_nspname));
+ nsinfo[i].nspowner = atooid(PQgetvalue(res, i, i_nspowner));
nsinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
nsinfo[i].nspacl = pg_strdup(PQgetvalue(res, i, i_nspacl));
nsinfo[i].rnspacl = pg_strdup(PQgetvalue(res, i, i_rnspacl));
@@ -10315,9 +10324,19 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo)
qnspname = pg_strdup(fmtId(nspinfo->dobj.name));
- appendPQExpBuffer(delq, "DROP SCHEMA %s;\n", qnspname);
-
- appendPQExpBuffer(q, "CREATE SCHEMA %s;\n", qnspname);
+ if (strcmp(nspinfo->dobj.name, "public") == 0)
+ {
+ /* see selectDumpableNamespace() */
+ appendPQExpBufferStr(delq,
+ "-- *not* dropping
schema, since initdb creates it\n");
+ appendPQExpBufferStr(q,
+ "-- *not* creating
schema, since initdb creates it\n");
+ }
+ else
+ {
+ appendPQExpBuffer(delq, "DROP SCHEMA %s;\n", qnspname);
+ appendPQExpBuffer(q, "CREATE SCHEMA %s;\n", qnspname);
+ }
if (dopt->binary_upgrade)
binary_upgrade_extension_member(q, &nspinfo->dobj,
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index d7f77f1..d14787b 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -141,6 +141,7 @@ typedef struct _dumpableObject
typedef struct _namespaceInfo
{
DumpableObject dobj;
+ Oid nspowner;
char *rolname; /* name of owner, or empty string */
char *nspacl;
char *rnspacl;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 11dc98e..5a5a766 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -605,6 +605,16 @@ my %tests = (
unlike => { no_owner => 1, },
},
+ 'ALTER SCHEMA public OWNER TO' => {
+ create_order => 2,
+ create_sql => 'ALTER SCHEMA public OWNER TO
regress_dump_test_role;',
+ regexp => qr/^ALTER SCHEMA public OWNER TO .+;/m,
+ like => {
+ %full_runs, section_pre_data => 1,
+ },
+ unlike => { no_owner => 1, },
+ },
+
'ALTER SEQUENCE test_table_col1_seq' => {
regexp => qr/^
\QALTER SEQUENCE dump_test.test_table_col1_seq OWNED BY
dump_test.test_table.col1;\E
@@ -940,6 +950,13 @@ my %tests = (
like => {},
},
+ 'COMMENT ON SCHEMA public' => {
+ regexp => qr/^COMMENT ON SCHEMA public IS .+;/m,
+
+ # this shouldn't ever get emitted
+ like => {},
+ },
+
'COMMENT ON TABLE dump_test.test_table' => {
create_order => 36,
create_sql => 'COMMENT ON TABLE dump_test.test_table
@@ -3229,6 +3246,7 @@ my %tests = (
create_sql => 'REVOKE CREATE ON SCHEMA public FROM public;',
regexp => qr/^
\QREVOKE ALL ON SCHEMA public FROM PUBLIC;\E
+ \n\QGRANT ALL ON SCHEMA public TO
regress_dump_test_role;\E
\n\QGRANT USAGE ON SCHEMA public TO PUBLIC;\E
/xm,
like => { %full_runs, section_pre_data => 1, },
