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.reh...@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).
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
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) "
+ "(SELECT acl, row_n FROM
pg_catalog.unnest(%s) "
"WITH ORDINALITY AS
initp(acl,row_n) "
"WHERE NOT EXISTS ( "
"SELECT 1 FROM "
"pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
"AS privm(orig_acl) WHERE acl
= orig_acl)) as foo) END",
+ initprivs_expr,
obj_kind,
acl_owner);
@@ -823,10 +827,11 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer
racl_subquery,
"pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
"WITH ORDINALITY AS
privp(acl,row_n) "
"WHERE NOT EXISTS ( "
- "SELECT 1 FROM
pg_catalog.unnest(pip.initprivs) "
+ "SELECT 1 FROM
pg_catalog.unnest(%s) "
"AS initp(init_acl) WHERE acl
= init_acl)) as foo) END",
obj_kind,
- acl_owner);
+ acl_owner,
+ initprivs_expr);
}
else
{
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index 6e97da7..f5465f1 100644
--- a/src/bin/pg_dump/dumputils.h
+++ b/src/bin/pg_dump/dumputils.h
@@ -54,6 +54,7 @@ extern void emitShSecLabels(PGconn *conn, PGresult *res,
extern 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);
extern bool variable_is_guc_list_quote(const char *name);
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 86de26a..4f96d9a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3570,10 +3570,12 @@ _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 variant for schema
+ * "public" that is 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));
}
@@ -3585,11 +3587,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;
when
+ * a dump emits a comment in lieu of creating it, 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->desc, "SCHEMA") == 0 &&
+ strncmp(te->defn, "--", 2) == 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 e397b76..350f7f3 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"
@@ -1611,6 +1612,13 @@ static void
selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
{
/*
+ * DUMP_COMPONENT_DEFINITION typically implies a CREATE SCHEMA statement
+ * and (for --clean) a DROP SCHEMA statement. (In the absence of
+ * DUMP_COMPONENT_DEFINITION, this value is irrelevant.)
+ */
+ nsinfo->create = true;
+
+ /*
* If specific tables are being dumped, do not dump any complete
* namespaces. If specific namespaces are being dumped, dump just those
* namespaces. Otherwise, dump all non-system namespaces.
@@ -1645,10 +1653,15 @@ 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. Hence, unless
the
+ * owner is the default, dump a "definition" bearing just a
comment.
*/
- nsinfo->dobj.dump = DUMP_COMPONENT_ACL;
+ nsinfo->create = false;
+ 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
@@ -3492,8 +3505,8 @@ getBlobs(Archive *fout)
PQExpBuffer init_racl_subquery = createPQExpBuffer();
buildACLQueries(acl_subquery, racl_subquery, init_acl_subquery,
- init_racl_subquery, "l.lomacl",
"l.lomowner", "'L'",
- dopt->binary_upgrade);
+ init_racl_subquery, "l.lomacl",
"l.lomowner",
+ "pip.initprivs", "'L'",
dopt->binary_upgrade);
appendPQExpBuffer(blobQry,
"SELECT l.oid, (%s
l.lomowner) AS rolname, "
@@ -4894,6 +4907,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;
@@ -4913,11 +4927,27 @@ getNamespaces(Archive *fout, int *numNamespaces)
PQExpBuffer init_acl_subquery = createPQExpBuffer();
PQExpBuffer init_racl_subquery = createPQExpBuffer();
+ /*
+ * Bypass pg_init_privs.initprivs for the public schema.
Dropping and
+ * recreating the schema detaches it from its pg_init_privs
row, but
+ * an empty destination database starts with this ACL
nonetheless.
+ * Also, we support dump/reload of public schema ownership
changes.
+ * ALTER SCHEMA OWNER filters nspacl through aclnewowner(), but
+ * initprivs continues to reflect the initial owner (the
bootstrap
+ * superuser). Hence, synthesize the value that nspacl will
have
+ * after the restore's ALTER SCHEMA OWNER.
+ */
buildACLQueries(acl_subquery, racl_subquery, init_acl_subquery,
- init_racl_subquery, "n.nspacl",
"n.nspowner", "'n'",
- dopt->binary_upgrade);
+ init_racl_subquery, "n.nspacl",
"n.nspowner",
+ "CASE WHEN n.nspname = 'public'
THEN array["
+ " format('%s=UC/%s', "
+ " n.nspowner::regrole,
n.nspowner::regrole),"
+ " format('=UC/%s',
n.nspowner::regrole)]::aclitem[] "
+ "ELSE pip.initprivs END",
+ "'n'", dopt->binary_upgrade);
appendPQExpBuffer(query, "SELECT n.tableoid, n.oid, n.nspname, "
+ "n.nspowner, "
"(%s nspowner) AS rolname, "
"%s as nspacl, "
"%s as rnspacl, "
@@ -4942,7 +4972,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 "
@@ -4958,6 +4988,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");
@@ -4971,6 +5002,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));
@@ -5162,8 +5194,8 @@ getTypes(Archive *fout, int *numTypes)
PQExpBuffer initracl_subquery = createPQExpBuffer();
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
- initracl_subquery, "t.typacl",
"t.typowner", "'T'",
- dopt->binary_upgrade);
+ initracl_subquery, "t.typacl",
"t.typowner",
+ "pip.initprivs", "'T'",
dopt->binary_upgrade);
appendPQExpBuffer(query, "SELECT t.tableoid, t.oid, t.typname, "
"t.typnamespace, "
@@ -5864,8 +5896,8 @@ getAggregates(Archive *fout, int *numAggs)
const char *agg_check;
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
- initracl_subquery, "p.proacl",
"p.proowner", "'f'",
- dopt->binary_upgrade);
+ initracl_subquery, "p.proacl",
"p.proowner",
+ "pip.initprivs", "'f'",
dopt->binary_upgrade);
agg_check = (fout->remoteVersion >= 110000 ? "p.prokind = 'a'"
: "p.proisagg");
@@ -6077,8 +6109,8 @@ getFuncs(Archive *fout, int *numFuncs)
const char *not_agg_check;
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
- initracl_subquery, "p.proacl",
"p.proowner", "'f'",
- dopt->binary_upgrade);
+ initracl_subquery, "p.proacl",
"p.proowner",
+ "pip.initprivs", "'f'",
dopt->binary_upgrade);
not_agg_check = (fout->remoteVersion >= 110000 ? "p.prokind <>
'a'"
: "NOT p.proisagg");
@@ -6374,13 +6406,14 @@ getTables(Archive *fout, int *numTables)
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
initracl_subquery, "c.relacl",
"c.relowner",
+ "pip.initprivs",
"CASE WHEN c.relkind = "
CppAsString2(RELKIND_SEQUENCE)
" THEN 's' ELSE 'r'
END::\"char\"",
dopt->binary_upgrade);
buildACLQueries(attacl_subquery, attracl_subquery,
attinitacl_subquery,
- attinitracl_subquery,
"at.attacl", "c.relowner", "'c'",
- dopt->binary_upgrade);
+ attinitracl_subquery,
"at.attacl", "c.relowner",
+ "pip.initprivs", "'c'",
dopt->binary_upgrade);
appendPQExpBuffer(query,
"SELECT c.tableoid, c.oid,
c.relname, "
@@ -8378,8 +8411,8 @@ getProcLangs(Archive *fout, int *numProcLangs)
PQExpBuffer initracl_subquery = createPQExpBuffer();
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
- initracl_subquery, "l.lanacl",
"l.lanowner", "'l'",
- dopt->binary_upgrade);
+ initracl_subquery, "l.lanacl",
"l.lanowner",
+ "pip.initprivs", "'l'",
dopt->binary_upgrade);
/* pg_language has a laninline column */
appendPQExpBuffer(query, "SELECT l.tableoid, l.oid, "
@@ -9569,8 +9602,8 @@ getForeignDataWrappers(Archive *fout, int
*numForeignDataWrappers)
PQExpBuffer initracl_subquery = createPQExpBuffer();
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
- initracl_subquery, "f.fdwacl",
"f.fdwowner", "'F'",
- dopt->binary_upgrade);
+ initracl_subquery, "f.fdwacl",
"f.fdwowner",
+ "pip.initprivs", "'F'",
dopt->binary_upgrade);
appendPQExpBuffer(query, "SELECT f.tableoid, f.oid, f.fdwname, "
"(%s f.fdwowner) AS rolname, "
@@ -9736,8 +9769,8 @@ getForeignServers(Archive *fout, int *numForeignServers)
PQExpBuffer initracl_subquery = createPQExpBuffer();
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
- initracl_subquery, "f.srvacl",
"f.srvowner", "'S'",
- dopt->binary_upgrade);
+ initracl_subquery, "f.srvacl",
"f.srvowner",
+ "pip.initprivs", "'S'",
dopt->binary_upgrade);
appendPQExpBuffer(query, "SELECT f.tableoid, f.oid, f.srvname, "
"(%s f.srvowner) AS rolname, "
@@ -9883,6 +9916,7 @@ getDefaultACLs(Archive *fout, int *numDefaultACLs)
buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
initracl_subquery, "defaclacl",
"defaclrole",
+ "pip.initprivs",
"CASE WHEN defaclobjtype = 'S'
THEN 's' ELSE defaclobjtype END::\"char\"",
dopt->binary_upgrade);
@@ -10500,9 +10534,19 @@ dumpNamespace(Archive *fout, const NamespaceInfo
*nspinfo)
qnspname = pg_strdup(fmtId(nspinfo->dobj.name));
- appendPQExpBuffer(delq, "DROP SCHEMA %s;\n", qnspname);
-
- appendPQExpBuffer(q, "CREATE SCHEMA %s;\n", qnspname);
+ if (nspinfo->create)
+ {
+ appendPQExpBuffer(delq, "DROP SCHEMA %s;\n", qnspname);
+ appendPQExpBuffer(q, "CREATE SCHEMA %s;\n", qnspname);
+ }
+ else
+ {
+ /* see selectDumpableNamespace() */
+ appendPQExpBufferStr(delq,
+ "-- *not* dropping
schema, since initdb creates it\n");
+ appendPQExpBufferStr(q,
+ "-- *not* creating
schema, since initdb creates it\n");
+ }
if (dopt->binary_upgrade)
binary_upgrade_extension_member(q, &nspinfo->dobj,
@@ -15673,8 +15717,8 @@ dumpTable(Archive *fout, const TableInfo *tbinfo)
PQExpBuffer initracl_subquery = createPQExpBuffer();
buildACLQueries(acl_subquery, racl_subquery,
initacl_subquery,
- initracl_subquery,
"at.attacl", "c.relowner", "'c'",
- dopt->binary_upgrade);
+ initracl_subquery,
"at.attacl", "c.relowner",
+ "pip.initprivs", "'c'",
dopt->binary_upgrade);
appendPQExpBuffer(query,
"SELECT at.attname, "
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 5340843..0868bea 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -142,6 +142,8 @@ typedef struct _dumpableObject
typedef struct _namespaceInfo
{
DumpableObject dobj;
+ bool create; /* CREATE SCHEMA, or just set
owner? */
+ 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 86113df..e8919a0 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -125,6 +125,14 @@ my %pgdump_runs = (
'regress_pg_dump_test',
],
},
+ defaults_public_owner => {
+ database => 'regress_public_owner',
+ dump_cmd => [
+ 'pg_dump', '--no-sync', '-f',
+ "$tempdir/defaults_public_owner.sql",
+ 'regress_public_owner',
+ ],
+ },
# Do not use --no-sync to give test coverage for data sync.
defaults_custom_format => {
@@ -605,6 +613,24 @@ my %tests = (
unlike => { no_owner => 1, },
},
+ 'ALTER SCHEMA public OWNER TO' => {
+ # see test "REVOKE CREATE ON SCHEMA public" for causative
create_sql
+ regexp => qr/^ALTER SCHEMA public OWNER TO .+;/m,
+ like => {
+ %full_runs, section_pre_data => 1,
+ },
+ unlike => { no_owner => 1, },
+ },
+
+ 'ALTER SCHEMA public OWNER TO (w/o ACL changes)' => {
+ database => 'regress_public_owner',
+ create_order => 100,
+ create_sql =>
+ 'ALTER SCHEMA public OWNER TO "regress_quoted \"" role";',
+ regexp => qr/^(GRANT|REVOKE)/m,
+ unlike => { defaults_public_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 +966,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
@@ -1370,6 +1403,18 @@ my %tests = (
},
},
+ 'CREATE ROLE regress_quoted...' => {
+ create_order => 1,
+ create_sql => 'CREATE ROLE "regress_quoted \"" role";',
+ regexp => qr/^\QCREATE ROLE "regress_quoted \""
role";\E/m,
+ like => {
+ pg_dumpall_dbprivs => 1,
+ pg_dumpall_exclude => 1,
+ pg_dumpall_globals => 1,
+ pg_dumpall_globals_clean => 1,
+ },
+ },
+
'CREATE ACCESS METHOD gist2' => {
create_order => 52,
create_sql =>
@@ -3273,6 +3318,23 @@ my %tests = (
unlike => { no_privs => 1, },
},
+ # With the exception of the public schema, we don't dump ownership
changes
+ # for objects originating at initdb. Hence, any GRANT or REVOKE
affecting
+ # owner privileges for those objects should reference the bootstrap
+ # superuser, not the dump-time owner.
+ 'REVOKE EXECUTE ON FUNCTION pg_stat_reset FROM regress_dump_test_role'
=>
+ {
+ create_order => 15,
+ create_sql => '
+ ALTER FUNCTION pg_stat_reset OWNER TO
regress_dump_test_role;
+ REVOKE EXECUTE ON FUNCTION pg_stat_reset
+ FROM regress_dump_test_role;',
+ regexp => qr/^[^-].*pg_stat_reset.* regress_dump_test_role/m,
+
+ # this shouldn't ever get emitted
+ like => {},
+ },
+
'REVOKE SELECT ON TABLE pg_proc FROM public' => {
create_order => 45,
create_sql => 'REVOKE SELECT ON TABLE pg_proc FROM public;',
@@ -3284,9 +3346,13 @@ my %tests = (
'REVOKE CREATE ON SCHEMA public FROM public' => {
create_order => 16,
- create_sql => 'REVOKE CREATE ON SCHEMA public FROM public;',
- regexp => qr/^
- \QREVOKE ALL ON SCHEMA public FROM PUBLIC;\E
+ create_sql => '
+ REVOKE CREATE ON SCHEMA public FROM public;
+ ALTER SCHEMA public OWNER TO "regress_quoted \"" role";
+ REVOKE ALL ON SCHEMA public FROM "regress_quoted \""
role";',
+ regexp => qr/^
+ \QREVOKE ALL ON SCHEMA public FROM "regress_quoted \""
role";\E
+ \n\QREVOKE ALL ON SCHEMA public FROM PUBLIC;\E
\n\QGRANT USAGE ON SCHEMA public TO PUBLIC;\E
/xm,
like => { %full_runs, section_pre_data => 1, },
@@ -3388,8 +3454,9 @@ if ($collation_check_stderr !~ /ERROR: /)
$collation_support = 1;
}
-# Create a second database for certain tests to work against
+# Create additional databases for mutations of schema public
$node->psql('postgres', 'create database regress_pg_dump_test;');
+$node->psql('postgres', 'create database regress_public_owner;');
# Start with number of command_fails_like()*2 tests below (each
# command_fails_like is actually 2 tests)
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl
b/src/test/modules/test_pg_dump/t/001_base.pl
index 1cc6f29..7683725 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -315,6 +315,14 @@ my %tests = (
like => { pg_dumpall_globals => 1, },
},
+ 'CREATE SCHEMA public' => {
+ regexp => qr/^CREATE SCHEMA public;/m,
+ like => {
+ extension_schema => 1,
+ without_extension_explicit_schema => 1,
+ },
+ },
+
'CREATE SEQUENCE regress_pg_dump_table_col1_seq' => {
regexp => qr/^
\QCREATE SEQUENCE public.regress_pg_dump_table_col1_seq\E
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Dump COMMENT ON SCHEMA public.
As we do for other attributes of the public schema, omit the COMMENT
command when its payload would match what initdb had installed. For
dumps that do carry this new COMMENT command, non-superusers restoring
them are likely to get an error.
Reviewed by Asif Rehman.
Discussion:
https://postgr.es/m/ab48a34c-60f6-e388-502a-3e5fe46a2...@postgresfriends.org
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 350f7f3..a8fe192 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -170,9 +170,15 @@ static NamespaceInfo *findNamespace(Oid nsoid);
static void dumpTableData(Archive *fout, const TableDataInfo *tdinfo);
static void refreshMatViewData(Archive *fout, const TableDataInfo *tdinfo);
static void guessConstraintInheritance(TableInfo *tblinfo, int numTables);
-static void dumpComment(Archive *fout, const char *type, const char *name,
- const char *namespace, const
char *owner,
- CatalogId catalogId, int subid,
DumpId dumpId);
+static void dumpCommentExtended(Archive *fout, const char *type,
+ const char
*name, const char *namespace,
+ const char
*owner, CatalogId catalogId,
+ int subid,
DumpId dumpId,
+ const char
*initdb_comment);
+static inline void dumpComment(Archive *fout, const char *type,
+ const char *name,
const char *namespace,
+ const char *owner,
CatalogId catalogId,
+ int subid, DumpId
dumpId);
static int findComments(Archive *fout, Oid classoid, Oid objoid,
CommentItem **items);
static int collectComments(Archive *fout, CommentItem **items);
@@ -1650,16 +1656,13 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive
*fout)
{
/*
* The public schema is a strange beast that sits in a sort of
- * 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 ownership changes, security labels, and ACL
- * changes, and of course we should dump contained objects.
pg_dump
- * ties ownership to DUMP_COMPONENT_DEFINITION. Hence, unless
the
- * owner is the default, dump a "definition" bearing just a
comment.
+ * no-mans-land between being a system object and a user object.
+ * CREATE SCHEMA would fail, so its DUMP_COMPONENT_DEFINITION
is just
+ * a comment and an indication of ownership. If the owner is
the
+ * default, that DUMP_COMPONENT_DEFINITION is superfluous.
*/
nsinfo->create = false;
- nsinfo->dobj.dump = DUMP_COMPONENT_ALL &
~DUMP_COMPONENT_COMMENT;
+ nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
if (nsinfo->nspowner == BOOTSTRAP_SUPERUSERID)
nsinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;
nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
@@ -10010,7 +10013,7 @@ getDefaultACLs(Archive *fout, int *numDefaultACLs)
}
/*
- * dumpComment --
+ * dumpCommentExtended --
*
* This routine is used to dump any comments associated with the
* object handed to this routine. The routine takes the object type
@@ -10033,9 +10036,11 @@ getDefaultACLs(Archive *fout, int *numDefaultACLs)
* calling ArchiveEntry() for the specified object.
*/
static void
-dumpComment(Archive *fout, const char *type, const char *name,
- const char *namespace, const char *owner,
- CatalogId catalogId, int subid, DumpId dumpId)
+dumpCommentExtended(Archive *fout, const char *type,
+ const char *name, const char *namespace,
+ const char *owner, CatalogId catalogId,
+ int subid, DumpId dumpId,
+ const char *initdb_comment)
{
DumpOptions *dopt = fout->dopt;
CommentItem *comments;
@@ -10071,6 +10076,25 @@ dumpComment(Archive *fout, const char *type, const
char *name,
ncomments--;
}
+ if (initdb_comment != NULL)
+ {
+ static CommentItem empty_comment = {.descr = ""};
+
+ /*
+ * initdb creates this object with a comment. Skip dumping the
+ * initdb-provided comment, which would complicate matters for
+ * non-superuser use of pg_dump. When the DBA has removed
initdb's
+ * comment, replicate that.
+ */
+ if (ncomments == 0)
+ {
+ comments = &empty_comment;
+ ncomments = 1;
+ }
+ else if (strcmp(comments->descr, initdb_comment) == 0)
+ ncomments = 0;
+ }
+
/* If a comment exists, build COMMENT ON statement */
if (ncomments > 0)
{
@@ -10107,6 +10131,21 @@ dumpComment(Archive *fout, const char *type, const
char *name,
}
/*
+ * dumpComment --
+ *
+ * Typical simplification of the above function.
+ */
+static inline void
+dumpComment(Archive *fout, const char *type,
+ const char *name, const char *namespace,
+ const char *owner, CatalogId catalogId,
+ int subid, DumpId dumpId)
+{
+ dumpCommentExtended(fout, type, name, namespace, owner,
+ catalogId, subid, dumpId, NULL);
+}
+
+/*
* dumpTableComment --
*
* As above, but dump comments for both the specified table (or view)
@@ -10563,9 +10602,18 @@ dumpNamespace(Archive *fout, const NamespaceInfo
*nspinfo)
/* Dump Schema Comments and Security Labels */
if (nspinfo->dobj.dump & DUMP_COMPONENT_COMMENT)
- dumpComment(fout, "SCHEMA", qnspname,
- NULL, nspinfo->rolname,
- nspinfo->dobj.catId, 0,
nspinfo->dobj.dumpId);
+ {
+ const char *initdb_comment = NULL;
+
+ if (!nspinfo->create && strcmp(qnspname, "public") == 0)
+ initdb_comment = (fout->remoteVersion >= 80300 ?
+ "standard public
schema" :
+ "Standard public
schema");
+ dumpCommentExtended(fout, "SCHEMA", qnspname,
+ NULL, nspinfo->rolname,
+ nspinfo->dobj.catId, 0,
nspinfo->dobj.dumpId,
+ initdb_comment);
+ }
if (nspinfo->dobj.dump & DUMP_COMPONENT_SECLABEL)
dumpSecLabel(fout, "SCHEMA", qnspname,
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index e8919a0..a176f08 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -968,9 +968,19 @@ my %tests = (
'COMMENT ON SCHEMA public' => {
regexp => qr/^COMMENT ON SCHEMA public IS .+;/m,
+ # regress_public_owner emits this, due to create_sql of next
test
+ like => {
+ pg_dumpall_dbprivs => 1,
+ pg_dumpall_exclude => 1,
+ },
+ },
- # this shouldn't ever get emitted
- like => {},
+ 'COMMENT ON SCHEMA public IS NULL' => {
+ database => 'regress_public_owner',
+ create_order => 100,
+ create_sql => 'COMMENT ON SCHEMA public IS NULL;',
+ regexp => qr/^COMMENT ON SCHEMA public IS '';/m,
+ like => { defaults_public_owner => 1 },
},
'COMMENT ON TABLE dump_test.test_table' => {
diff --git a/src/include/catalog/pg_namespace.dat
b/src/include/catalog/pg_namespace.dat
index 2ed136b..33992af 100644
--- a/src/include/catalog/pg_namespace.dat
+++ b/src/include/catalog/pg_namespace.dat
@@ -18,6 +18,7 @@
{ oid => '99', oid_symbol => 'PG_TOAST_NAMESPACE',
descr => 'reserved schema for TOAST tables',
nspname => 'pg_toast', nspacl => '_null_' },
+# update dumpNamespace() if changing this descr
{ oid => '2200', oid_symbol => 'PG_PUBLIC_NAMESPACE',
descr => 'standard public schema',
nspname => 'public', nspacl => '_null_' },
