Improve pg_dump's handling of "special" built-in objects.

We had some pretty ad-hoc handling of the public schema and the plpgsql
extension, which are both presumed to exist in template0 but might be
modified or deleted in the database being dumped.

Up to now, by default pg_dump would emit a CREATE EXTENSION IF NOT EXISTS
command as well as a COMMENT command for plpgsql.  The usefulness of the
former is questionable, and the latter caused annoying errors in
non-superuser dump/restore scenarios.  Let's instead install a rule that
built-in extensions (identified by having low-numbered OIDs) are not to be
dumped.  We were doing it that way already in binary-upgrade mode, so this
just makes regular mode behave the same.  It remains true that if someone
has installed a non-default ACL on the plpgsql language, that will get
dumped thanks to the pg_init_privs mechanism.  This is more consistent with
the handling of built-in objects of other kinds.

Also, change the very ad-hoc mechanism that was used to avoid dumping
creation and comment commands for the public schema.  Instead of hardwiring
a test in _printTocEntry(), make use of the DUMP_COMPONENT_ infrastructure
to mark that schema up-front about what we want to do with it.  This has
the visible effect that the public schema won't be mentioned in the output
at all, except for updating its ACL if it has a non-default ACL.
Previously, while it was normally not mentioned, --clean mode would drop
and recreate it, again causing headaches for non-superuser usage.  This
change likewise makes the public schema less special and more like other
built-in objects.

If plpgsql, or the public schema, has been removed entirely in the source
DB, that situation won't be reproduced in the destination ... but that
was true before.

Discussion: https://postgr.es/m/29048.1516812451@sss.pgh.pa.us

src/bin/pg_dump/pg_backup_archiver.c

@@ -3453,29 +3453,6 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData)
 {
 	RestoreOptions *ropt = AH->public.ropt;
 
-	/*
-	 * Avoid dumping the public schema, as it will already be created ...
-	 * unless we are using --clean mode (and *not* --create mode), in which
-	 * case we've previously issued a DROP for it so we'd better recreate it.
-	 *
-	 * Likewise for its comment, if any.  (We could try issuing the COMMENT
-	 * command anyway; but it'd fail if the restore is done as non-super-user,
-	 * so let's not.)
-	 *
-	 * XXX it looks pretty ugly to hard-wire the public schema like this, but
-	 * it sits in a sort of no-mans-land between being a system object and a
-	 * user object, so it really is special in a way.
-	 */
-	if (!(ropt->dropSchema && !ropt->createDB))
-	{
-		if (strcmp(te->desc, "SCHEMA") == 0 &&
-			strcmp(te->tag, "public") == 0)
-			return;
-		if (strcmp(te->desc, "COMMENT") == 0 &&
-			strcmp(te->tag, "SCHEMA public") == 0)
-			return;
-	}
-
 	/* Select owner, schema, and tablespace as necessary */
 	_becomeOwner(AH, te);
 	_selectOutputSchema(AH, te->namespace);

src/bin/pg_dump/pg_dump.c

@@ -1407,6 +1407,19 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, Archive *fout)
 		/* Other system schemas don't get dumped */
 		nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_NONE;
 	}
+	else if (strcmp(nsinfo->dobj.name, "public") == 0)
+	{
+		/*
+		 * 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 ACL changes that have occurred for it, and of
+		 * course we should dump contained objects.
+		 */
+		nsinfo->dobj.dump = DUMP_COMPONENT_ACL;
+		nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
+	}
 	else
 		nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
 
@@ -1617,21 +1630,21 @@ selectDumpableAccessMethod(AccessMethodInfo *method, Archive *fout)
  * selectDumpableExtension: policy-setting subroutine
  *		Mark an extension as to be dumped or not
  *
- * Normally, we dump all extensions, or none of them if include_everything
- * is false (i.e., a --schema or --table switch was given).  However, in
- * binary-upgrade mode it's necessary to skip built-in extensions, since we
+ * Built-in extensions should be skipped except for checking ACLs, since we
  * assume those will already be installed in the target database.  We identify
  * such extensions by their having OIDs in the range reserved for initdb.
+ * We dump all user-added extensions by default, or none of them if
+ * include_everything is false (i.e., a --schema or --table switch was given).
  */
 static void
 selectDumpableExtension(ExtensionInfo *extinfo, DumpOptions *dopt)
 {
 	/*
-	 * Use DUMP_COMPONENT_ACL for from-initdb extensions, to allow users to
-	 * change permissions on those objects, if they wish to, and have those
-	 * changes preserved.
+	 * Use DUMP_COMPONENT_ACL for built-in extensions, to allow users to
+	 * change permissions on their member objects, if they wish to, and have
+	 * those changes preserved.
 	 */
-	if (dopt->binary_upgrade && extinfo->dobj.catId.oid <= (Oid) g_last_builtin_oid)
+	if (extinfo->dobj.catId.oid <= (Oid) g_last_builtin_oid)
 		extinfo->dobj.dump = extinfo->dobj.dump_contains = DUMP_COMPONENT_ACL;
 	else
 		extinfo->dobj.dump = extinfo->dobj.dump_contains =
@@ -4435,29 +4448,6 @@ getNamespaces(Archive *fout, int *numNamespaces)
 						  init_acl_subquery->data,
 						  init_racl_subquery->data);
 
-		/*
-		 * When we are doing a 'clean' run, we will be dropping and recreating
-		 * the 'public' schema (the only object which has that kind of
-		 * treatment in the backend and which has an entry in pg_init_privs)
-		 * and therefore we should not consider any initial privileges in
-		 * pg_init_privs in that case.
-		 *
-		 * See pg_backup_archiver.c:_printTocEntry() for the details on why
-		 * the public schema is special in this regard.
-		 *
-		 * Note that if the public schema is dropped and re-created, this is
-		 * essentially a no-op because the new public schema won't have an
-		 * entry in pg_init_privs anyway, as the entry will be removed when
-		 * the public schema is dropped.
-		 *
-		 * Further, we have to handle the case where the public schema does
-		 * not exist at all.
-		 */
-		if (dopt->outputClean)
-			appendPQExpBuffer(query, " AND pip.objoid <> "
-							  "coalesce((select oid from pg_namespace "
-							  "where nspname = 'public'),0)");
-
 		appendPQExpBuffer(query, ") ");
 
 		destroyPQExpBuffer(acl_subquery);
@@ -9945,20 +9935,28 @@ dumpExtension(Archive *fout, ExtensionInfo *extinfo)
 	if (!dopt->binary_upgrade)
 	{
 		/*
-		 * In a regular dump, we use IF NOT EXISTS so that there isn't a
-		 * problem if the extension already exists in the target database;
-		 * this is essential for installed-by-default extensions such as
-		 * plpgsql.
+		 * In a regular dump, we simply create the extension, intentionally
+		 * not specifying a version, so that the destination installation's
+		 * default version is used.
 		 *
-		 * In binary-upgrade mode, that doesn't work well, so instead we skip
-		 * built-in extensions based on their OIDs; see
-		 * selectDumpableExtension.
+		 * Use of IF NOT EXISTS here is unlike our behavior for other object
+		 * types; but there are various scenarios in which it's convenient to
+		 * manually create the desired extension before restoring, so we
+		 * prefer to allow it to exist already.
 		 */
 		appendPQExpBuffer(q, "CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s;\n",
 						  qextname, fmtId(extinfo->namespace));
 	}
 	else
 	{
+		/*
+		 * In binary-upgrade mode, it's critical to reproduce the state of the
+		 * database exactly, so our procedure is to create an empty extension,
+		 * restore all the contained objects normally, and add them to the
+		 * extension one by one.  This function performs just the first of
+		 * those steps.  binary_upgrade_extension_member() takes care of
+		 * adding member objects as they're created.
+		 */
 		int			i;
 		int			n;
 
@@ -9968,8 +9966,6 @@ dumpExtension(Archive *fout, ExtensionInfo *extinfo)
 		 * We unconditionally create the extension, so we must drop it if it
 		 * exists.  This could happen if the user deleted 'plpgsql' and then
 		 * readded it, causing its oid to be greater than g_last_builtin_oid.
-		 * The g_last_builtin_oid test was kept to avoid repeatedly dropping
-		 * and recreating extensions like 'plpgsql'.
 		 */
 		appendPQExpBuffer(q, "DROP EXTENSION IF EXISTS %s;\n", qextname);
 

src/bin/pg_dump/t/002_pg_dump.pl

@@ -1548,30 +1548,31 @@ qr/^ALTER (?!EVENT TRIGGER|LARGE OBJECT|PUBLICATION|SUBSCRIPTION)(.*) OWNER TO .
 		all_runs  => 1,
 		catch_all => 'COMMENT commands',
 		regexp    => qr/^COMMENT ON EXTENSION plpgsql IS .*;/m,
-		like      => {
+		# this shouldn't ever get emitted anymore
+		like      => {},
+		unlike => {
+			binary_upgrade           => 1,
 			clean                    => 1,
 			clean_if_exists          => 1,
+			column_inserts           => 1,
 			createdb                 => 1,
+			data_only                => 1,
 			defaults                 => 1,
 			exclude_dump_test_schema => 1,
 			exclude_test_table       => 1,
 			exclude_test_table_data  => 1,
 			no_blobs                 => 1,
-			no_privs                 => 1,
 			no_owner                 => 1,
+			no_privs                 => 1,
+			only_dump_test_schema    => 1,
+			only_dump_test_table     => 1,
 			pg_dumpall_dbprivs       => 1,
+			role                     => 1,
 			schema_only              => 1,
+			section_post_data        => 1,
 			section_pre_data         => 1,
-			with_oids                => 1, },
-		unlike => {
-			binary_upgrade         => 1,
-			column_inserts         => 1,
-			data_only              => 1,
-			only_dump_test_schema  => 1,
-			only_dump_test_table   => 1,
-			role                   => 1,
-			section_post_data      => 1,
-			test_schema_plus_blobs => 1, }, },
+			test_schema_plus_blobs   => 1,
+			with_oids                => 1, }, },
 
 	'COMMENT ON TABLE dump_test.test_table' => {
 		all_runs     => 1,
@@ -2751,33 +2752,34 @@ qr/CREATE CAST \(timestamp with time zone AS interval\) WITH FUNCTION pg_catalog
 		regexp   => qr/^
 			\QCREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;\E
 			/xm,
-		like => {
+		# this shouldn't ever get emitted anymore
+		like => {},
+		unlike => {
+			binary_upgrade           => 1,
 			clean                    => 1,
 			clean_if_exists          => 1,
+			column_inserts           => 1,
 			createdb                 => 1,
+			data_only                => 1,
 			defaults                 => 1,
 			exclude_dump_test_schema => 1,
 			exclude_test_table       => 1,
 			exclude_test_table_data  => 1,
 			no_blobs                 => 1,
-			no_privs                 => 1,
 			no_owner                 => 1,
-			pg_dumpall_dbprivs       => 1,
-			schema_only              => 1,
-			section_pre_data         => 1,
-			with_oids                => 1, },
-		unlike => {
-			binary_upgrade           => 1,
-			column_inserts           => 1,
-			data_only                => 1,
+			no_privs                 => 1,
 			only_dump_test_schema    => 1,
 			only_dump_test_table     => 1,
+			pg_dumpall_dbprivs       => 1,
 			pg_dumpall_globals       => 1,
 			pg_dumpall_globals_clean => 1,
 			role                     => 1,
+			schema_only              => 1,
 			section_data             => 1,
 			section_post_data        => 1,
-			test_schema_plus_blobs   => 1, }, },
+			section_pre_data         => 1,
+			test_schema_plus_blobs   => 1,
+			with_oids                => 1, }, },
 
 	'CREATE AGGREGATE dump_test.newavg' => {
 		all_runs     => 1,
@@ -4565,11 +4567,12 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		all_runs  => 1,
 		catch_all => 'CREATE ... commands',
 		regexp    => qr/^CREATE SCHEMA public;/m,
-		like      => {
-			clean           => 1,
-			clean_if_exists => 1, },
+		# this shouldn't ever get emitted anymore
+		like      => {},
 		unlike => {
 			binary_upgrade           => 1,
+			clean                    => 1,
+			clean_if_exists          => 1,
 			createdb                 => 1,
 			defaults                 => 1,
 			exclude_test_table       => 1,
@@ -5395,8 +5398,10 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		all_runs  => 1,
 		catch_all => 'DROP ... commands',
 		regexp    => qr/^DROP SCHEMA public;/m,
-		like      => { clean => 1 },
+		# this shouldn't ever get emitted anymore
+		like      => {},
 		unlike    => {
+			clean                    => 1,
 			clean_if_exists          => 1,
 			pg_dumpall_globals_clean => 1, }, },
 
@@ -5404,17 +5409,21 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		all_runs  => 1,
 		catch_all => 'DROP ... commands',
 		regexp    => qr/^DROP SCHEMA IF EXISTS public;/m,
-		like      => { clean_if_exists => 1 },
+		# this shouldn't ever get emitted anymore
+		like      => {},
 		unlike    => {
 			clean                    => 1,
+			clean_if_exists          => 1,
 			pg_dumpall_globals_clean => 1, }, },
 
 	'DROP EXTENSION plpgsql' => {
 		all_runs  => 1,
 		catch_all => 'DROP ... commands',
 		regexp    => qr/^DROP EXTENSION plpgsql;/m,
-		like      => { clean => 1, },
+		# this shouldn't ever get emitted anymore
+		like      => {},
 		unlike    => {
+			clean => 1,
 			clean_if_exists          => 1,
 			pg_dumpall_globals_clean => 1, }, },
 
@@ -5494,9 +5503,11 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		all_runs  => 1,
 		catch_all => 'DROP ... commands',
 		regexp    => qr/^DROP EXTENSION IF EXISTS plpgsql;/m,
-		like      => { clean_if_exists => 1, },
+		# this shouldn't ever get emitted anymore
+		like      => {},
 		unlike    => {
 			clean                    => 1,
+			clean_if_exists          => 1,
 			pg_dumpall_globals_clean => 1, }, },
 
 	'DROP FUNCTION IF EXISTS dump_test.pltestlang_call_handler()' => {
@@ -6264,11 +6275,12 @@ qr/^GRANT SELECT ON TABLE measurement_y2006m2 TO regress_dump_test_role;/m,
 			\Q--\E\n\n
 			\QGRANT USAGE ON SCHEMA public TO PUBLIC;\E
 			/xm,
-		like => {
-			clean           => 1,
-			clean_if_exists => 1, },
+		# this shouldn't ever get emitted anymore
+		like => {},
 		unlike => {
 			binary_upgrade           => 1,
+			clean                    => 1,
+			clean_if_exists          => 1,
 			createdb                 => 1,
 			defaults                 => 1,
 			exclude_dump_test_schema => 1,
@@ -6537,6 +6549,8 @@ qr/^GRANT SELECT ON TABLE measurement_y2006m2 TO regress_dump_test_role;/m,
 			/xm,
 		like => {
 			binary_upgrade           => 1,
+			clean                    => 1,
+			clean_if_exists          => 1,
 			createdb                 => 1,
 			defaults                 => 1,
 			exclude_dump_test_schema => 1,
@@ -6549,8 +6563,6 @@ qr/^GRANT SELECT ON TABLE measurement_y2006m2 TO regress_dump_test_role;/m,
 			section_pre_data         => 1,
 			with_oids                => 1, },
 		unlike => {
-			clean                    => 1,
-			clean_if_exists          => 1,
 			only_dump_test_schema    => 1,
 			only_dump_test_table     => 1,
 			pg_dumpall_globals_clean => 1,
@@ -6576,18 +6588,18 @@ qr/^GRANT SELECT ON TABLE measurement_y2006m2 TO regress_dump_test_role;/m,
 			exclude_test_table_data  => 1,
 			no_blobs                 => 1,
 			no_owner                 => 1,
+			only_dump_test_schema    => 1,
+			only_dump_test_table     => 1,
 			pg_dumpall_dbprivs       => 1,
+			role                     => 1,
 			schema_only              => 1,
 			section_pre_data         => 1,
+			test_schema_plus_blobs   => 1,
 			with_oids                => 1, },
 		unlike => {
-			only_dump_test_schema    => 1,
-			only_dump_test_table     => 1,
 			pg_dumpall_globals_clean => 1,
-			role                     => 1,
 			section_data             => 1,
-			section_post_data        => 1,
-			test_schema_plus_blobs   => 1, }, },
+			section_post_data        => 1, }, },
 
 	'REVOKE commands' => {    # catch-all for REVOKE commands
 		all_runs => 0,               # catch-all