Move handling of database properties from pg_dumpall into pg_dump.

This patch rearranges the division of labor between pg_dump and pg_dumpall
so that pg_dump itself handles all properties attached to a single
database.  Notably, a database's ACL (GRANT/REVOKE status) and local GUC
settings established by ALTER DATABASE SET and ALTER ROLE IN DATABASE SET
can be dumped and restored by pg_dump.  This is a long-requested
improvement.

"pg_dumpall -g" will now produce only role- and tablespace-related output,
nothing about individual databases.  The total output of a regular
pg_dumpall run remains the same.

pg_dump (or pg_restore) will restore database-level properties only when
creating the target database with --create.  This applies not only to
ACLs and GUCs but to the other database properties it already handled,
that is database comments and security labels.  This is more consistent
and useful, but does represent an incompatibility in the behavior seen
without --create.

(This change makes the proposed patch to have pg_dump use "COMMENT ON
DATABASE CURRENT_DATABASE" unnecessary, since there is no case where
the command is issued that we won't know the true name of the database.
We might still want that patch as a feature in its own right, but pg_dump
no longer needs it.)

pg_dumpall with --clean will now drop and recreate the "postgres" and
"template1" databases in the target cluster, allowing their locale and
encoding settings to be changed if necessary, and providing a cleaner
way to set nondefault tablespaces for them than we had before.  This
means that such a script must now always be started in the "postgres"
database; the order of drops and reconnects will not work otherwise.
Without --clean, the script will not adjust any database-level properties
of those two databases (including their comments, ACLs, and security
labels, which it formerly would try to set).

Another minor incompatibility is that the CREATE DATABASE commands in a
pg_dumpall script will now always specify locale and encoding settings.
Formerly those would be omitted if they matched the cluster's default.
While that behavior had some usefulness in some migration scenarios,
it also posed a significant hazard of unwanted locale/encoding changes.
To migrate to another locale/encoding, it's now necessary to use pg_dump
without --create to restore into a database with the desired settings.

Commit 4bd371f6's hack to emit "SET default_transaction_read_only = off"
is gone: we now dodge that problem by the expedient of not issuing ALTER
DATABASE SET commands until after reconnecting to the target database.
Therefore, such settings won't apply during the restore session.

In passing, improve some shaky grammar in the docs, and add a note pointing
out that pg_dumpall's output can't be expected to load without any errors.
(Someday we might want to fix that, but this is not that patch.)

Haribabu Kommi, reviewed at various times by Andreas Karlsson,
Vaishnavi Prabakaran, and Robert Haas; further hacking by me.

Discussion: https://postgr.es/m/CAJrrPGcUurV0eWTeXODwsOYFN=Ekq36t1s0YnFYUNzsmRfdAyA@mail.gmail.com

doc/src/sgml/ref/pg_dump.sgml

@@ -46,9 +46,10 @@ PostgreSQL documentation
   </para>
 
   <para>
-   <application>pg_dump</application> only dumps a single database.  To backup
-   global objects that are common to all databases in a cluster, such as roles
-   and tablespaces, use <xref linkend="app-pg-dumpall"/>.
+   <application>pg_dump</application> only dumps a single database.
+   To back up an entire cluster, or to back up global objects that are
+   common to all databases in a cluster (such as roles and tablespaces),
+   use <xref linkend="app-pg-dumpall"/>.
   </para>
 
   <para>
@@ -142,7 +143,8 @@ PostgreSQL documentation
         switch is therefore only useful to add large objects to dumps
         where a specific schema or table has been requested.  Note that
         blobs are considered data and therefore will be included when
-        --data-only is used, but not when --schema-only is.
+        <option>--data-only</option> is used, but not
+        when <option>--schema-only</option> is.
        </para>
       </listitem>
      </varlistentry>
@@ -196,6 +198,17 @@ PostgreSQL documentation
         recreates the target database before reconnecting to it.
        </para>
 
+       <para>
+        With <option>--create</option>, the output also includes the
+        database's comment if any, and any configuration variable settings
+        that are specific to this database, that is,
+        any <command>ALTER DATABASE ... SET ...</command>
+        and <command>ALTER ROLE ... IN DATABASE ... SET ...</command>
+        commands that mention this database.
+        Access privileges for the database itself are also dumped,
+        unless <option>--no-acl</option> is specified.
+       </para>
+
        <para>
         This option is only meaningful for the plain-text format.  For
         the archive formats, you can specify the option when you
@@ -1231,10 +1244,6 @@ CREATE DATABASE foo WITH TEMPLATE template0;
    <command>ANALYZE</command> after restoring from a dump file
    to ensure optimal performance; see <xref linkend="vacuum-for-statistics"/>
    and <xref linkend="autovacuum"/> for more information.
-   The dump file also does not
-   contain any <command>ALTER DATABASE ... SET</command> commands;
-   these settings are dumped by <xref linkend="app-pg-dumpall"/>,
-   along with database users and other installation-wide settings.
   </para>
 
   <para>
@@ -1325,6 +1334,15 @@ CREATE DATABASE foo WITH TEMPLATE template0;
 </screen>
   </para>
 
+  <para>
+   To reload an archive file into the same database it was dumped from,
+   discarding the current contents of that database:
+
+<screen>
+<prompt>$</prompt> <userinput>pg_restore -d postgres --clean --create db.dump</userinput>
+</screen>
+  </para>
+
   <para>
    To dump a single table named <literal>mytab</literal>:
 

doc/src/sgml/ref/pg_dumpall.sgml

@@ -36,13 +36,10 @@ PostgreSQL documentation
    of a cluster into one script file.  The script file contains
    <acronym>SQL</acronym> commands that can be used as input to <xref
    linkend="app-psql"/> to restore the databases.  It does this by
-   calling <xref linkend="app-pgdump"/> for each database in a cluster.
+   calling <xref linkend="app-pgdump"/> for each database in the cluster.
    <application>pg_dumpall</application> also dumps global objects
-   that are common to all databases.
+   that are common to all databases, that is, database roles and tablespaces.
    (<application>pg_dump</application> does not save these objects.)
-   This currently includes information about database users and
-   groups, tablespaces, and properties such as access permissions
-   that apply to databases as a whole.
   </para>
 
   <para>
@@ -50,7 +47,7 @@ PostgreSQL documentation
    databases you will most likely have to connect as a database
    superuser in order to produce a complete dump.  Also you will need
    superuser privileges to execute the saved script in order to be
-   allowed to add users and groups, and to create databases.
+   allowed to add roles and create databases.
   </para>
 
   <para>
@@ -308,7 +305,7 @@ PostgreSQL documentation
       <listitem>
        <para>
         Use conditional commands (i.e. add an <literal>IF EXISTS</literal>
-        clause) to clean databases and other objects.  This option is not valid
+        clause) to drop databases and other objects.  This option is not valid
         unless <option>--clean</option> is also specified.
        </para>
       </listitem>
@@ -500,10 +497,11 @@ PostgreSQL documentation
        <para>
         The option is called <literal>--dbname</literal> for consistency with other
         client applications, but because <application>pg_dumpall</application>
-        needs to connect to many databases, database name in the connection
-        string will be ignored. Use <literal>-l</literal> option to specify
-        the name of the database used to dump global objects and to discover
-        what other databases should be dumped.
+        needs to connect to many databases, the database name in the
+        connection string will be ignored.  Use the <literal>-l</literal>
+        option to specify the name of the database used for the initial
+        connection, which will dump global objects and discover what other
+        databases should be dumped.
        </para>
       </listitem>
      </varlistentry>
@@ -657,6 +655,17 @@ PostgreSQL documentation
    messages will refer to <application>pg_dump</application>.
   </para>
 
+  <para>
+   The <option>--clean</option> option can be useful even when your
+   intention is to restore the dump script into a fresh cluster.  Use of
+   <option>--clean</option> authorizes the script to drop and re-create the
+   built-in <literal>postgres</literal> and <literal>template1</literal>
+   databases, ensuring that those databases will retain the same properties
+   (for instance, locale and encoding) that they had in the source cluster.
+   Without the option, those databases will retain their existing
+   database-level properties, as well as any pre-existing contents.
+  </para>
+
   <para>
    Once restored, it is wise to run <command>ANALYZE</command> on each
    database so the optimizer has useful statistics. You
@@ -664,6 +673,18 @@ PostgreSQL documentation
    databases.
   </para>
 
+  <para>
+   The dump script should not be expected to run completely without errors.
+   In particular, because the script will issue <command>CREATE ROLE</command>
+   for every role existing in the source cluster, it is certain to get a
+   <quote>role already exists</quote> error for the bootstrap superuser,
+   unless the destination cluster was initialized with a different bootstrap
+   superuser name.  This error is harmless and should be ignored.  Use of
+   the <option>--clean</option> option is likely to produce additional
+   harmless error messages about non-existent objects, although you can
+   minimize those by adding <option>--if-exists</option>.
+  </para>
+
   <para>
    <application>pg_dumpall</application> requires all needed
    tablespace directories to exist before the restore;  otherwise,
@@ -688,10 +709,13 @@ PostgreSQL documentation
 <screen>
 <prompt>$</prompt> <userinput>psql -f db.out postgres</userinput>
 </screen>
-   (It is not important to which database you connect here since the
+   It is not important to which database you connect here since the
    script file created by <application>pg_dumpall</application> will
    contain the appropriate commands to create and connect to the saved
-   databases.)
+   databases.  An exception is that if you specified <option>--clean</option>,
+   you must connect to the <literal>postgres</literal> database initially;
+   the script will attempt to drop other databases immediately, and that
+   will fail for the database you are connected to.
   </para>
  </refsect1>
 

doc/src/sgml/ref/pg_restore.sgml

@@ -126,6 +126,17 @@
         recreate the target database before connecting to it.
        </para>
 
+       <para>
+        With <option>--create</option>, <application>pg_restore</application>
+        also restores the database's comment if any, and any configuration
+        variable settings that are specific to this database, that is,
+        any <command>ALTER DATABASE ... SET ...</command>
+        and <command>ALTER ROLE ... IN DATABASE ... SET ...</command>
+        commands that mention this database.
+        Access privileges for the database itself are also restored,
+        unless <option>--no-acl</option> is specified.
+       </para>
+
        <para>
         When this option is used, the database named with <option>-d</option>
         is used only to issue the initial <command>DROP DATABASE</command> and

src/bin/pg_dump/dumputils.c

@@ -807,3 +807,54 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
 		printfPQExpBuffer(init_racl_subquery, "NULL");
 	}
 }
+
+/*
+ * Helper function for dumping "ALTER DATABASE/ROLE SET ..." commands.
+ *
+ * Parse the contents of configitem (a "name=value" string), wrap it in
+ * a complete ALTER command, and append it to buf.
+ *
+ * type is DATABASE or ROLE, and name is the name of the database or role.
+ * If we need an "IN" clause, type2 and name2 similarly define what to put
+ * there; otherwise they should be NULL.
+ * conn is used only to determine string-literal quoting conventions.
+ */
+void
+makeAlterConfigCommand(PGconn *conn, const char *configitem,
+					   const char *type, const char *name,
+					   const char *type2, const char *name2,
+					   PQExpBuffer buf)
+{
+	char	   *mine;
+	char	   *pos;
+
+	/* Parse the configitem.  If we can't find an "=", silently do nothing. */
+	mine = pg_strdup(configitem);
+	pos = strchr(mine, '=');
+	if (pos == NULL)
+	{
+		pg_free(mine);
+		return;
+	}
+	*pos++ = '\0';
+
+	/* Build the command, with suitable quoting for everything. */
+	appendPQExpBuffer(buf, "ALTER %s %s ", type, fmtId(name));
+	if (type2 != NULL && name2 != NULL)
+		appendPQExpBuffer(buf, "IN %s %s ", type2, fmtId(name2));
+	appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
+
+	/*
+	 * Some GUC variable names are 'LIST' type and hence must not be quoted.
+	 * XXX this list is incomplete ...
+	 */
+	if (pg_strcasecmp(mine, "DateStyle") == 0
+		|| pg_strcasecmp(mine, "search_path") == 0)
+		appendPQExpBufferStr(buf, pos);
+	else
+		appendStringLiteralConn(buf, pos, conn);
+
+	appendPQExpBufferStr(buf, ";\n");
+
+	pg_free(mine);
+}

src/bin/pg_dump/dumputils.h

@@ -56,4 +56,9 @@ extern void buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
 				const char *acl_column, const char *acl_owner,
 				const char *obj_kind, bool binary_upgrade);
 
+extern void makeAlterConfigCommand(PGconn *conn, const char *configitem,
+					   const char *type, const char *name,
+					   const char *type2, const char *name2,
+					   PQExpBuffer buf);
+
 #endif							/* DUMPUTILS_H */

src/bin/pg_dump/pg_backup_archiver.c

@@ -489,16 +489,19 @@ RestoreArchive(Archive *AHX)
 			 * whole.  Issuing drops against anything else would be wrong,
 			 * because at this point we're connected to the wrong database.
 			 * Conversely, if we're not in createDB mode, we'd better not
-			 * issue a DROP against the database at all.
+			 * issue a DROP against the database at all.  (The DATABASE
+			 * PROPERTIES entry, if any, works like the DATABASE entry.)
 			 */
 			if (ropt->createDB)
 			{
-				if (strcmp(te->desc, "DATABASE") != 0)
+				if (strcmp(te->desc, "DATABASE") != 0 &&
+					strcmp(te->desc, "DATABASE PROPERTIES") != 0)
 					continue;
 			}
 			else
 			{
-				if (strcmp(te->desc, "DATABASE") == 0)
+				if (strcmp(te->desc, "DATABASE") == 0 ||
+					strcmp(te->desc, "DATABASE PROPERTIES") == 0)
 					continue;
 			}
 
@@ -558,6 +561,8 @@ RestoreArchive(Archive *AHX)
 							 * we simply emit the original command for DEFAULT
 							 * objects (modulo the adjustment made above).
 							 *
+							 * Likewise, don't mess with DATABASE PROPERTIES.
+							 *
 							 * If we used CREATE OR REPLACE VIEW as a means of
 							 * quasi-dropping an ON SELECT rule, that should
 							 * be emitted unchanged as well.
@@ -570,6 +575,7 @@ RestoreArchive(Archive *AHX)
 							 * search for hardcoded "DROP CONSTRAINT" instead.
 							 */
 							if (strcmp(te->desc, "DEFAULT") == 0 ||
+								strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
 								strncmp(dropStmt, "CREATE OR REPLACE VIEW", 22) == 0)
 								appendPQExpBufferStr(ftStmt, dropStmt);
 							else
@@ -750,11 +756,19 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
 	reqs = te->reqs;
 
 	/*
-	 * Ignore DATABASE entry unless we should create it.  We must check this
-	 * here, not in _tocEntryRequired, because the createDB option should not
-	 * affect emitting a DATABASE entry to an archive file.
+	 * Ignore DATABASE and related entries unless createDB is specified.  We
+	 * must check this here, not in _tocEntryRequired, because !createDB
+	 * should not prevent emitting these entries to an archive file.
 	 */
-	if (!ropt->createDB && strcmp(te->desc, "DATABASE") == 0)
+	if (!ropt->createDB &&
+		(strcmp(te->desc, "DATABASE") == 0 ||
+		 strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
+		 (strcmp(te->desc, "ACL") == 0 &&
+		  strncmp(te->tag, "DATABASE ", 9) == 0) ||
+		 (strcmp(te->desc, "COMMENT") == 0 &&
+		  strncmp(te->tag, "DATABASE ", 9) == 0) ||
+		 (strcmp(te->desc, "SECURITY LABEL") == 0 &&
+		  strncmp(te->tag, "DATABASE ", 9) == 0)))
 		reqs = 0;
 
 	/* Dump any relevant dump warnings to stderr */
@@ -2917,8 +2931,8 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
 		 * Special Case: If 'SEQUENCE SET' or anything to do with BLOBs, then
 		 * it is considered a data entry.  We don't need to check for the
 		 * BLOBS entry or old-style BLOB COMMENTS, because they will have
-		 * hadDumper = true ... but we do need to check new-style BLOB
-		 * comments.
+		 * hadDumper = true ... but we do need to check new-style BLOB ACLs,
+		 * comments, etc.
 		 */
 		if (strcmp(te->desc, "SEQUENCE SET") == 0 ||
 			strcmp(te->desc, "BLOB") == 0 ||
@@ -3598,6 +3612,7 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData)
 		else if (strcmp(te->desc, "CAST") == 0 ||
 				 strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
 				 strcmp(te->desc, "CONSTRAINT") == 0 ||
+				 strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
 				 strcmp(te->desc, "DEFAULT") == 0 ||
 				 strcmp(te->desc, "FK CONSTRAINT") == 0 ||
 				 strcmp(te->desc, "INDEX") == 0 ||

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

@@ -1516,11 +1516,14 @@ qr/^ALTER (?!EVENT TRIGGER|LARGE OBJECT|PUBLICATION|SUBSCRIPTION)(.*) OWNER TO .
 		all_runs  => 1,
 		catch_all => 'COMMENT commands',
 		regexp    => qr/^COMMENT ON DATABASE postgres IS .*;/m,
-		like      => {
+		# Should appear in the same tests as "CREATE DATABASE postgres"
+		like   => { createdb => 1, },
+		unlike => {
 			binary_upgrade           => 1,
 			clean                    => 1,
 			clean_if_exists          => 1,
-			createdb                 => 1,
+			column_inserts           => 1,
+			data_only                => 1,
 			defaults                 => 1,
 			exclude_dump_test_schema => 1,
 			exclude_test_table       => 1,
@@ -1528,18 +1531,18 @@ qr/^ALTER (?!EVENT TRIGGER|LARGE OBJECT|PUBLICATION|SUBSCRIPTION)(.*) OWNER TO .
 			no_blobs                 => 1,
 			no_privs                 => 1,
 			no_owner                 => 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_pre_data         => 1,
-			with_oids                => 1, },
-		unlike => {
-			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, }, },
+			section_data             => 1,
+			section_post_data        => 1,
+			test_schema_plus_blobs   => 1,
+			with_oids                => 1, }, },
 
 	'COMMENT ON EXTENSION plpgsql' => {
 		all_runs  => 1,

src/bin/pg_upgrade/pg_upgrade.c

@@ -46,7 +46,7 @@
 #endif
 
 static void prepare_new_cluster(void);
-static void prepare_new_databases(void);
+static void prepare_new_globals(void);
 static void create_new_objects(void);
 static void copy_xact_xlog_xid(void);
 static void set_frozenxids(bool minmxid_only);
@@ -124,7 +124,7 @@ main(int argc, char **argv)
 	/* -- NEW -- */
 	start_postmaster(&new_cluster, true);
 
-	prepare_new_databases();
+	prepare_new_globals();
 
 	create_new_objects();
 
@@ -271,7 +271,7 @@ prepare_new_cluster(void)
 
 
 static void
-prepare_new_databases(void)
+prepare_new_globals(void)
 {
 	/*
 	 * We set autovacuum_freeze_max_age to its maximum value so autovacuum
@@ -283,20 +283,11 @@ prepare_new_databases(void)
 
 	prep_status("Restoring global objects in the new cluster");
 
-	/*
-	 * We have to create the databases first so we can install support
-	 * functions in all the other databases.  Ideally we could create the
-	 * support functions in template1 but pg_dumpall creates database using
-	 * the template0 template.
-	 */
 	exec_prog(UTILITY_LOG_FILE, NULL, true, true,
 			  "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
 			  new_cluster.bindir, cluster_conn_opts(&new_cluster),
 			  GLOBALS_DUMP_FILE);
 	check_ok();
-
-	/* we load this to get a current list of databases */
-	get_db_and_rel_infos(&new_cluster);
 }
 
 
@@ -312,33 +303,40 @@ create_new_objects(void)
 		char		sql_file_name[MAXPGPATH],
 					log_file_name[MAXPGPATH];
 		DbInfo	   *old_db = &old_cluster.dbarr.dbs[dbnum];
-		PQExpBufferData connstr,
-					escaped_connstr;
-
-		initPQExpBuffer(&connstr);
-		appendPQExpBuffer(&connstr, "dbname=");
-		appendConnStrVal(&connstr, old_db->db_name);
-		initPQExpBuffer(&escaped_connstr);
-		appendShellString(&escaped_connstr, connstr.data);
-		termPQExpBuffer(&connstr);
+		const char *create_opts;
+		const char *starting_db;
 
 		pg_log(PG_STATUS, "%s", old_db->db_name);
 		snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
 		snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);
 
 		/*
-		 * pg_dump only produces its output at the end, so there is little
-		 * parallelism if using the pipe.
+		 * template1 and postgres databases will already exist in the target
+		 * installation, so tell pg_restore to drop and recreate them;
+		 * otherwise we would fail to propagate their database-level
+		 * properties.
 		 */
+		if (strcmp(old_db->db_name, "template1") == 0 ||
+			strcmp(old_db->db_name, "postgres") == 0)
+			create_opts = "--clean --create";
+		else
+			create_opts = "--create";
+
+		/* When processing template1, we can't connect there to start with */
+		if (strcmp(old_db->db_name, "template1") == 0)
+			starting_db = "postgres";
+		else
+			starting_db = "template1";
+
 		parallel_exec_prog(log_file_name,
 						   NULL,
-						   "\"%s/pg_restore\" %s --exit-on-error --verbose --dbname %s \"%s\"",
+						   "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
+						   "--dbname %s \"%s\"",
 						   new_cluster.bindir,
 						   cluster_conn_opts(&new_cluster),
-						   escaped_connstr.data,
+						   create_opts,
+						   starting_db,
 						   sql_file_name);
-
-		termPQExpBuffer(&escaped_connstr);
 	}
 
 	/* reap all children */
@@ -355,7 +353,7 @@ create_new_objects(void)
 	if (GET_MAJOR_VERSION(old_cluster.major_version) < 903)
 		set_frozenxids(true);
 
-	/* regenerate now that we have objects in the databases */
+	/* update new_cluster info now that we have objects in the databases */
 	get_db_and_rel_infos(&new_cluster);
 }