On Thu, Aug 27, 2026 at 2:23 PM Mahendra Singh Thalor
<[email protected]> wrote:
>
> Thanks Noah and Andrew for the feedback. Based on review comments, I
> re-implemented these patches.
>
at [1], Noah mentioned that
"""
There should be one appendPQExpBuffer(delQry, "DROP ROLE ..."), not one for
plain format and another for non-plain formats. Having two creates excess
risk of format-specific bugs, something pg_dump.c has long avoided well. (I'm
echoing my postgr.es/m/[email protected] review. I wrote,
"The strength of the archiver architecture shows in how rarely new features
need format-specific logic and how rarely format-specific bugs get reported."
That holds for the way pg_dump.c uses the archiver, but it doesn't hold for
the way pg_dumpall.c now uses the archiver.)
"""
That means we cannot use
if (archDumpFormat == archNull)
fprintf(OPF, "DROP ROLE"....)
else
ArchiveEntry(fout, ....)
All of the below is wrong.
+static void dropDBsText(PGconn *conn);
+static void dropTablespacesText(PGconn *conn);
+static void dropRolesText(PGconn *conn)
Therefore, we should unify it using ArchiveEntry, because we cannot
use fprintf for non-plain text formats.
With that, plain-text format also needs CreateArchive(). We must be
careful with fprintf(OPF): both RestoreArchive() and fprintf(OPF)
write to the same output, so their writes must be ordered correctly.
I also refactored check_for_invalid_global_names. if one database name
contained \r\n, call pg_fatal.
[1]
https://www.postgresql.org/message-id/20260607000218.96.noahmisch%40microsoft.com
--
jian
https://www.enterprisedb.com/
From 53e18987a274b262ee0ef764469d70be3074a081 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Mon, 5 Oct 2026 13:34:06 +0800
Subject: [PATCH v2 1/2] refactor check_for_invalid_global_names and pgindent
---
src/bin/pg_dump/pg_dumpall.c | 110 ++++++++++++++++++-----------------
1 file changed, 57 insertions(+), 53 deletions(-)
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index d1714cb7447..f1152f46a42 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -776,12 +776,12 @@ main(int argc, char *argv[])
if (!data_only && !statistics_only && !no_schema)
{
/*
- * For plain-text output with --clean, print the Drop
- * databases/Drop tablespaces/Drop roles sections first, ahead
of
- * any creates (see dropDBsText() for why the order matters).
For
- * non-plain formats,
dumpRoles()/dumpTablespaces()/dumpDatabases()
- * instead attach a drop statement to each archive entry, and
the
- * archiver decides at restore time whether to use it.
+ * For plain-text output with --clean, print the Drop
databases/Drop
+ * tablespaces/Drop roles sections first, ahead of any creates
(see
+ * dropDBsText() for why the order matters). For non-plain
formats,
+ * dumpRoles()/dumpTablespaces()/dumpDatabases() instead attach
a drop
+ * statement to each archive entry, and the archiver decides at
+ * restore time whether to use it.
*
* For non-text formats, pg_dumpall unconditionally process
--clean
* option. In contrast, pg_restore only applies it if the user
@@ -1762,7 +1762,7 @@ dumpTablespaces(PGconn *conn)
ArchiveEntry(fout,
nilCatalogId, /* catalog ID */
- spcDumpId, /* dump
ID */
+ spcDumpId, /* dump ID */
ARCHIVE_OPTS(.tag = tag,
.description = "TABLESPACE",
.section = SECTION_PRE_DATA,
@@ -2231,58 +2231,62 @@ static void
check_for_invalid_global_names(PGconn *conn,
SimpleStringList
*excluded_names)
{
+ PQExpBuffer query = createPQExpBuffer();
+ PQExpBuffer escaped;
PGresult *res;
- int i;
- PQExpBuffer names;
- int count = 0;
-
- res = executeQuery(conn,
- "SELECT datname AS objname,
'database' AS objtype "
- "FROM pg_catalog.pg_database "
- "WHERE datallowconn AND datconnlimit
!= -2 "
- "UNION ALL "
- "SELECT rolname AS objname, 'role'
AS objtype "
- "FROM pg_catalog.pg_roles "
- "UNION ALL "
- "SELECT spcname AS objname,
'tablespace' AS objtype "
- "FROM pg_catalog.pg_tablespace");
-
- names = createPQExpBuffer();
-
- for (i = 0; i < PQntuples(res); i++)
+ char *objname;
+ char *objtype;
+
+ appendPQExpBufferStr(query,
+ "(SELECT datname AS objname,
'database' AS objtype "
+ "FROM pg_catalog.pg_database "
+ "WHERE datallowconn AND
datconnlimit != -2 "
+ "AND datname ~ E'[\\n\\r]'");
+
+ for (SimpleStringListCell *cell = excluded_names->head; cell; cell =
cell->next)
{
- char *objname = PQgetvalue(res, i, 0);
- char *objtype = PQgetvalue(res, i, 1);
-
- /* Skip excluded databases since they won't be in map.dat */
- if (strcmp(objtype, "database") == 0 &&
- simple_string_list_member(excluded_names, objname))
- continue;
-
- if (strpbrk(objname, "\n\r"))
- {
- appendPQExpBuffer(names, " %s: \"", objtype);
- for (char *p = objname; *p; p++)
- {
- if (*p == '\n')
- appendPQExpBufferStr(names, "\\n");
- else if (*p == '\r')
- appendPQExpBufferStr(names, "\\r");
- else
- appendPQExpBufferChar(names, *p);
- }
- appendPQExpBufferStr(names, "\"\n");
- count++;
- }
+ appendPQExpBufferStr(query, " AND datname <> ");
+ appendStringLiteralConn(query, cell->val, conn);
}
- PQclear(res);
+ appendPQExpBufferStr(query,
+ " LIMIT 1) "
+ "UNION ALL "
+ "(SELECT rolname, 'role' "
+ "FROM pg_catalog.pg_roles "
+ "WHERE rolname ~ E'[\\n\\r]'
LIMIT 1) "
+ "UNION ALL "
+ "(SELECT spcname, 'tablespace'
"
+ "FROM pg_catalog.pg_tablespace
"
+ "WHERE spcname ~ E'[\\n\\r]'
LIMIT 1) "
+ "LIMIT 1");
+
+ res = executeQuery(conn, query->data);
+ destroyPQExpBuffer(query);
+
+ if (PQntuples(res) == 0)
+ {
+ PQclear(res);
+ return;
+ }
- if (count > 0)
- pg_fatal("database, role, or tablespace names contain a newline
or carriage return character, which is not supported in non-plain-text
dumps:\n%s",
- names->data);
+ objname = PQgetvalue(res, 0, 0);
+ objtype = PQgetvalue(res, 0, 1);
+
+ /* Show newline and carriage return as \n and \r */
+ escaped = createPQExpBuffer();
+ for (char *p = objname; *p; p++)
+ {
+ if (*p == '\n')
+ appendPQExpBufferStr(escaped, "\\n");
+ else if (*p == '\r')
+ appendPQExpBufferStr(escaped, "\\r");
+ else
+ appendPQExpBufferChar(escaped, *p);
+ }
- destroyPQExpBuffer(names);
+ pg_fatal("%s name \"%s\" contains a newline or carriage return
character, which is not supported in non-plain-text dumps",
+ objtype, escaped->data);
}
--
2.34.1
From dc2d24282a2fefb990d1e1e0a75e5a27ebce3d45 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Thu, 8 Oct 2026 15:39:49 +0800
Subject: [PATCH v2 2/2] using ArchiveEntry not using fprintf for global
objects
* Run pgindent on pg_backup_archiver.c and pg_dumpall.c.
* To keep the diff small, some former if/else branches are kept as bare
{ ... } blocks instead of being re-indented; they can be flattened later.
* Keep OPF open in append mode and fflush() it before the archiver or
pg_dump writes to the file, instead of repeatedly closing and reopening it.
* As requested in [1], pg_dumpall.c should not write global objects with
fprintf(OPF); they should be emitted through ArchiveEntry() in all output
formats, in a uniform way.
[1]
https://www.postgresql.org/message-id/20260607000218.96.noahmisch%40microsoft.com
---
src/bin/pg_dump/pg_backup_archiver.c | 26 +-
src/bin/pg_dump/pg_dumpall.c | 478 ++++++++-------------------
src/bin/pg_dump/t/007_pg_dumpall.pl | 7 +-
3 files changed, 167 insertions(+), 344 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 0a755126838..c24c8bbc399 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -644,7 +644,9 @@ RestoreArchive(Archive *AHX, bool append_data)
* object type. Most
of the time this matches
* te->desc, so search
for that; however for the
* different kinds of
CONSTRAINTs, we know to
- * search for hardcoded
"DROP CONSTRAINT" instead.
+ * search for hardcoded
"DROP CONSTRAINT" instead,
+ * and for pg_dumpall's
DROP_GLOBAL entries, "DROP
+ * DATABASE".
*/
if (strcmp(te->desc,
"DEFAULT") == 0 ||
strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
@@ -659,6 +661,8 @@ RestoreArchive(Archive *AHX, bool append_data)
strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
strcmp(te->desc, "FK CONSTRAINT") == 0)
strcpy(buffer, "DROP CONSTRAINT");
+ else if
(strcmp(te->desc, "DROP_GLOBAL") == 0)
+
strcpy(buffer, "DROP DATABASE");
else
snprintf(buffer, sizeof(buffer), "DROP %s",
te->desc);
@@ -1670,7 +1674,7 @@ archputs(const char *s, Archive *AH)
/* Public */
int
-archprintf(Archive *AH, const char *fmt, ...)
+archprintf(Archive *AH, const char *fmt,...)
{
int save_errno = errno;
char *p;
@@ -1774,7 +1778,7 @@ RestoreOutput(ArchiveHandle *AH, CompressFileHandle
*savedOutput)
* Print formatted text to the output file (usually stdout).
*/
int
-ahprintf(ArchiveHandle *AH, const char *fmt, ...)
+ahprintf(ArchiveHandle *AH, const char *fmt,...)
{
int save_errno = errno;
char *p;
@@ -1912,7 +1916,7 @@ ahwrite(const void *ptr, size_t size, size_t nmemb,
ArchiveHandle *AH)
/* on some error, we may decide to go on... */
void
-warn_or_exit_horribly(ArchiveHandle *AH, const char *fmt, ...)
+warn_or_exit_horribly(ArchiveHandle *AH, const char *fmt,...)
{
/* Stay quiet if this is a result of our own cancellation. */
if (!is_cancel_in_progress())
@@ -3020,7 +3024,8 @@ _tocEntryRequired(TocEntry *te, teSection curSection,
ArchiveHandle *AH)
/* These items are treated specially */
if (strcmp(te->desc, "ENCODING") == 0 ||
strcmp(te->desc, "STDSTRINGS") == 0 ||
- strcmp(te->desc, "SEARCHPATH") == 0)
+ strcmp(te->desc, "SEARCHPATH") == 0 ||
+ strcmp(te->desc, "DEFAULT_TRANSACTION_READ_ONLY") == 0)
return REQ_SPECIAL;
if ((strcmp(te->desc, "STATISTICS DATA") == 0) ||
@@ -3054,9 +3059,9 @@ _tocEntryRequired(TocEntry *te, teSection curSection,
ArchiveHandle *AH)
* Global object TOC entries (e.g., ROLEs or TABLESPACEs) must not be
* ignored. They have no te->namespace, so without this, the
* --schema/--exclude-schema checks below would exclude them entirely.
- * DROP_GLOBAL (pg_dumpall's cross-database drops) is included here
- * rather than above with DATABASE/DATABASE PROPERTIES because it must
- * be gated on dropSchema, not createDB.
+ * DROP_GLOBAL (pg_dumpall's cross-database drops) is included here
rather
+ * than above with DATABASE/DATABASE PROPERTIES because it must be gated
+ * on dropSchema, not createDB.
*/
if (strcmp(te->desc, "ROLE") == 0 ||
strcmp(te->desc, "ROLE PROPERTIES") == 0 ||
@@ -3484,6 +3489,11 @@ _doSetFixedOutputState(ArchiveHandle *AH)
{
RestoreOptions *ropt = AH->public.ropt;
+ /* pg_dumpall archives: the restore must be able to write */
+ for (TocEntry *te = AH->toc->next; te != AH->toc; te = te->next)
+ if (strcmp(te->desc, "DEFAULT_TRANSACTION_READ_ONLY") == 0)
+ ahprintf(AH, "%s", te->defn);
+
/*
* Disable timeouts to allow for slow commands, idle parallel workers,
etc
*/
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index f1152f46a42..38d3c8ca3e4 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -66,14 +66,12 @@ typedef struct
static void help(void);
-static void dropDBsText(PGconn *conn);
-static void dropTablespacesText(PGconn *conn);
-static void dropRolesText(PGconn *conn);
static void dumpRoles(PGconn *conn);
static void dumpRoleMembership(PGconn *conn);
static void dumpRoleGUCPrivs(PGconn *conn);
static void dumpTablespaces(PGconn *conn);
static void dumpUserConfig(PGconn *conn, const char *username);
+static void dropDatabases(PGconn *conn);
static void dumpDatabases(PGconn *conn);
static void dumpTimestamp(const char *msg);
static int runPgDump(const char *dbname, const char *create_opts, char
*dbfile);
@@ -567,6 +565,17 @@ main(int argc, char *argv[])
if (!OPF)
pg_fatal("could not open output file \"%s\": %m",
filename);
+ fclose(OPF);
+
+ /*
+ * Reopen in append mode, so that our writes always go to the
end of
+ * the file even after the archiver or pg_dump has appended to
it.
+ * Flushing OPF is then enough; we needn't close and reopen it
around
+ * those writes.
+ */
+ OPF = fopen(filename, PG_BINARY_A);
+ if (!OPF)
+ pg_fatal("could not open output file \"%s\": %m",
filename);
}
else
OPF = stdout;
@@ -658,40 +667,60 @@ main(int argc, char *argv[])
executeCommand(conn, "SET quote_all_identifiers = true");
/* Create an archive file for global commands. */
- if (archDumpFormat != archNull)
+ if (archDumpFormat == archNull)
+ {
+ fprintf(OPF, "--\n-- PostgreSQL database cluster dump\n--\n\n");
+
+ if (verbose)
+ dumpTimestamp("Started on");
+
+ fflush(OPF);
+ }
+
+ /*
+ * Check that no global object names contain newlines or carriage
returns,
+ * which would break the map.dat file format. This is only needed for
+ * servers older than v19, which started prohibiting such names.
+ */
+ if (server_version < 190000 && archDumpFormat != archNull)
+ check_for_invalid_global_names(conn, &database_exclude_names);
+
{
PQExpBuffer qry = createPQExpBuffer();
- char global_path[MAXPGPATH];
const char *encname;
pg_compress_specification compression_spec = {0};
- /*
- * Check that no global object names contain newlines or
carriage
- * returns, which would break the map.dat file format. This is
only
- * needed for servers older than v19, which started prohibiting
such
- * names.
- */
- if (server_version < 190000)
- check_for_invalid_global_names(conn,
&database_exclude_names);
-
- /* Set file path for global sql commands. */
- snprintf(global_path, MAXPGPATH, "%s/toc.glo", filename);
-
/* Open the output file */
- fout = CreateArchive(global_path, archCustom, compression_spec,
- dosync, archModeWrite,
NULL, DATA_DIR_SYNC_METHOD_FSYNC);
+ if (archDumpFormat != archNull)
+ {
+ char global_path[MAXPGPATH];
+
+ /* Set file path for global sql commands. */
+ snprintf(global_path, MAXPGPATH, "%s/toc.glo",
filename);
+
+ fout = CreateArchive(global_path, archCustom,
compression_spec,
+ dosync,
archModeWrite, NULL, DATA_DIR_SYNC_METHOD_FSYNC);
+ }
+ else
+ fout = CreateArchive(filename, archNull,
compression_spec,
+ dosync,
archModeWrite, NULL, DATA_DIR_SYNC_METHOD_FSYNC);
+
+ fout->std_strings = true;
+ fout->encoding = encoding;
/* Make dump options accessible right away */
SetArchiveOptions(fout, &dopt, NULL);
- ((ArchiveHandle *) fout)->connection = conn;
((ArchiveHandle *) fout)->public.numWorkers = 1;
/* Register the cleanup hook */
on_exit_close_archive(fout);
- /* Let the archiver know how noisy to be */
- fout->verbose = verbose;
+ /*
+ * Don't output TOC entry comments when dumping globals; they're
+ * purely informational and just add noise ahead of the SQL.
+ */
+ ((ArchiveHandle *) fout)->noTocComments = 1;
/*
* We allow the server to be back to 9.2, and up to any minor
release
@@ -703,12 +732,12 @@ main(int argc, char *argv[])
fout->numWorkers = 1;
/* Dump default_transaction_read_only. */
- appendPQExpBufferStr(qry, "SET default_transaction_read_only =
off;\n\n");
+ appendPQExpBufferStr(qry, "SET default_transaction_read_only =
off;\n");
ArchiveEntry(fout,
nilCatalogId, /* catalog ID */
createDumpId(), /* dump ID */
- ARCHIVE_OPTS(.tag =
"default_transaction_read_only",
- .description
= "default_transaction_read_only",
+ ARCHIVE_OPTS(.tag =
"DEFAULT_TRANSACTION_READ_ONLY",
+ .description
= "DEFAULT_TRANSACTION_READ_ONLY",
.section =
SECTION_PRE_DATA,
.createStmt =
qry->data));
resetPQExpBuffer(qry);
@@ -739,68 +768,9 @@ main(int argc, char *argv[])
.createStmt =
qry->data));
destroyPQExpBuffer(qry);
}
- else
- {
- fprintf(OPF, "--\n-- PostgreSQL database cluster dump\n--\n\n");
-
- if (verbose)
- dumpTimestamp("Started on");
-
- /*
- * Enter restricted mode to block any unexpected psql
meta-commands. A
- * malicious source might try to inject a variety of things via
bogus
- * responses to queries. While we cannot prevent such sources
from
- * affecting the destination at restore time, we can block psql
- * meta-commands so that the client machine that runs psql with
the
- * dump output remains unaffected.
- */
- fprintf(OPF, "\\restrict %s\n\n", restrict_key);
-
- /*
- * We used to emit \connect postgres here, but that served no
purpose
- * other than to break things for installations without a
postgres
- * database. Everything we're restoring here is a global, so
- * whichever database we're connected to at the moment is fine.
- */
-
- /* Restore will need to write to the target cluster */
- fprintf(OPF, "SET default_transaction_read_only = off;\n\n");
-
- /* Replicate encoding and standard_conforming_strings in output
*/
- fprintf(OPF, "SET client_encoding = '%s';\n",
- pg_encoding_to_char(encoding));
- fprintf(OPF, "SET standard_conforming_strings = on;\n");
- fprintf(OPF, "\n");
- }
if (!data_only && !statistics_only && !no_schema)
{
- /*
- * For plain-text output with --clean, print the Drop
databases/Drop
- * tablespaces/Drop roles sections first, ahead of any creates
(see
- * dropDBsText() for why the order matters). For non-plain
formats,
- * dumpRoles()/dumpTablespaces()/dumpDatabases() instead attach
a drop
- * statement to each archive entry, and the archiver decides at
- * restore time whether to use it.
- *
- * For non-text formats, pg_dumpall unconditionally process
--clean
- * option. In contrast, pg_restore only applies it if the user
- * explicitly provides the flag. This discrepancy resolves
corner
- * cases where pg_restore requires cleanup instructions that
may be
- * missing from a standard pg_dumpall output.
- */
- if (archDumpFormat == archNull && output_clean)
- {
- if (!globals_only && !roles_only && !tablespaces_only)
- dropDBsText(conn);
-
- if (!roles_only && !no_tablespaces)
- dropTablespacesText(conn);
-
- if (!tablespaces_only)
- dropRolesText(conn);
- }
-
if (!tablespaces_only)
{
/* Dump roles (users) */
@@ -817,24 +787,34 @@ main(int argc, char *argv[])
/* Dump tablespaces */
if (!roles_only && !no_tablespaces)
dumpTablespaces(conn);
+
+ if (!globals_only && !roles_only && !tablespaces_only)
+ dropDatabases(conn);
}
if (archDumpFormat == archNull)
{
- /*
- * Exit restricted mode just before dumping the databases.
pg_dump
- * will handle entering restricted mode again as appropriate.
- */
- fprintf(OPF, "\\unrestrict %s\n\n", restrict_key);
+ RestoreOptions *ropt = NewRestoreOptions();
+
+ ropt->filename = filename;
+ ropt->dropSchema = output_clean;
+ ropt->if_exists = if_exists;
+ ropt->restrict_key = restrict_key;
+ SetArchiveOptions(fout, &dopt, ropt);
+ ProcessArchiveRestoreOptions(fout);
+
+ /* write out all global objects before any database */
+ RestoreArchive(fout, true);
+ CloseArchive(fout);
}
if (!globals_only && !roles_only && !tablespaces_only)
dumpDatabases(conn);
+ PQfinish(conn);
+
if (archDumpFormat == archNull)
{
- PQfinish(conn);
-
if (verbose)
dumpTimestamp("Completed on");
fprintf(OPF, "--\n-- PostgreSQL database cluster dump
complete\n--\n\n");
@@ -941,125 +921,6 @@ help(void)
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
-
-/*
- * Print the "Drop databases"/"Drop tablespaces"/"Drop roles" plain-text
- * sections, in that order, ahead of any "create" sections. The order
- * matters: a database may depend on a tablespace or role, so those must
- * go first. Non-plain formats need no such helper; the archiver's
- * restore-time drop pass already runs all drops before any creates,
- * regardless of registration order.
- */
-static void
-dropDBsText(PGconn *conn)
-{
- PGresult *res;
- int i;
-
- Assert(archDumpFormat == archNull);
-
- res = executeQuery(conn,
- "SELECT datname "
- "FROM pg_database d "
- "WHERE datallowconn AND datconnlimit
!= -2 "
- "ORDER BY datname");
-
- if (PQntuples(res) > 0)
- fprintf(OPF, "--\n-- Drop databases (except postgres and
template1)\n--\n\n");
-
- for (i = 0; i < PQntuples(res); i++)
- {
- char *dbname = PQgetvalue(res, i, 0);
-
- if (strcmp(dbname, "template1") != 0 &&
- strcmp(dbname, "template0") != 0 &&
- strcmp(dbname, "postgres") != 0)
- fprintf(OPF, "DROP DATABASE %s%s;\n",
- if_exists ? "IF EXISTS " : "",
- fmtId(dbname));
- }
-
- PQclear(res);
-
- if (PQntuples(res) > 0)
- fprintf(OPF, "\n\n");
-}
-
-
-/*
- * Drop tablespaces (see dropDBsText() for why this is plain-text only).
- */
-static void
-dropTablespacesText(PGconn *conn)
-{
- PGresult *res;
- int i;
-
- Assert(archDumpFormat == archNull);
-
- res = executeQuery(conn, "SELECT spcname "
- "FROM pg_catalog.pg_tablespace "
- "WHERE spcname !~ '^pg_' "
- "ORDER BY 1");
-
- if (PQntuples(res) > 0)
- fprintf(OPF, "--\n-- Drop tablespaces\n--\n\n");
-
- for (i = 0; i < PQntuples(res); i++)
- {
- char *spcname = PQgetvalue(res, i, 0);
-
- fprintf(OPF, "DROP TABLESPACE %s%s;\n",
- if_exists ? "IF EXISTS " : "",
- fmtId(spcname));
- }
-
- PQclear(res);
-
- if (PQntuples(res) > 0)
- fprintf(OPF, "\n\n");
-}
-
-
-/*
- * Drop roles (see dropDBsText() for why this is plain-text only).
- */
-static void
-dropRolesText(PGconn *conn)
-{
- PQExpBuffer buf = createPQExpBuffer();
- PGresult *res;
- int i;
-
- Assert(archDumpFormat == archNull);
-
- printfPQExpBuffer(buf,
- "SELECT rolname "
- "FROM %s "
- "WHERE rolname !~ '^pg_' "
- "ORDER BY 1", role_catalog);
-
- res = executeQuery(conn, buf->data);
- destroyPQExpBuffer(buf);
-
- if (PQntuples(res) > 0)
- fprintf(OPF, "--\n-- Drop roles\n--\n\n");
-
- for (i = 0; i < PQntuples(res); i++)
- {
- char *rolename = PQgetvalue(res, i, 0);
-
- fprintf(OPF, "DROP ROLE %s%s;\n",
- if_exists ? "IF EXISTS " : "",
- fmtId(rolename));
- }
-
- PQclear(res);
-
- if (PQntuples(res) > 0)
- fprintf(OPF, "\n\n");
-}
-
/*
* Drop and dump roles.
*/
@@ -1119,9 +980,6 @@ dumpRoles(PGconn *conn)
i_rolcomment = PQfnumber(res, "rolcomment");
i_is_current_user = PQfnumber(res, "is_current_user");
- if (PQntuples(res) > 0 && archDumpFormat == archNull)
- fprintf(OPF, "--\n-- Roles\n--\n\n");
-
for (i = 0; i < PQntuples(res); i++)
{
const char *rolename;
@@ -1229,15 +1087,6 @@ dumpRoles(PGconn *conn)
"ROLE", rolename,
seclabel_buf);
- if (archDumpFormat == archNull)
- {
- fprintf(OPF, "%s", buf->data);
- fprintf(OPF, "%s", comment_buf->data);
-
- if (seclabel_buf->data[0] != '\0')
- fprintf(OPF, "%s", seclabel_buf->data);
- }
- else
{
char *tag = psprintf("ROLE %s", fmtId(rolename));
DumpId roleDumpId = createDumpId();
@@ -1279,17 +1128,10 @@ dumpRoles(PGconn *conn)
* We do it this way because config settings for roles could mention the
* names of other roles.
*/
- if (PQntuples(res) > 0 && archDumpFormat == archNull)
- fprintf(OPF, "\n--\n-- User Configurations\n--\n");
-
for (i = 0; i < PQntuples(res); i++)
dumpUserConfig(conn, PQgetvalue(res, i, i_rolname));
PQclear(res);
-
- if (archDumpFormat == archNull)
- fprintf(OPF, "\n\n");
-
destroyPQExpBuffer(buf);
destroyPQExpBuffer(delQry);
destroyPQExpBuffer(comment_buf);
@@ -1370,9 +1212,6 @@ dumpRoleMembership(PGconn *conn)
i_inherit_option = PQfnumber(res, "inherit_option");
i_set_option = PQfnumber(res, "set_option");
- if (PQntuples(res) > 0 && archDumpFormat == archNull)
- fprintf(OPF, "--\n-- Role memberships\n--\n\n");
-
/*
* We can't dump these GRANT commands in arbitrary order, because a role
* that is named as a grantor must already have ADMIN OPTION on the role
@@ -1545,16 +1384,13 @@ dumpRoleMembership(PGconn *conn)
appendPQExpBuffer(querybuf, " GRANTED
BY %s", fmtId(grantor));
appendPQExpBufferStr(querybuf, ";\n");
- if (archDumpFormat == archNull)
- fprintf(OPF, "%s", querybuf->data);
- else
- ArchiveEntry(fout,
- nilCatalogId,
/* catalog ID */
-
createDumpId(), /* dump ID */
-
ARCHIVE_OPTS(.tag = psprintf("ROLE %s", fmtId(role)),
-
.description = "ROLE PROPERTIES",
-
.section = SECTION_PRE_DATA,
-
.createStmt = querybuf->data));
+ ArchiveEntry(fout,
+ nilCatalogId, /*
catalog ID */
+ createDumpId(),
/* dump ID */
+ ARCHIVE_OPTS(.tag =
psprintf("ROLE %s", fmtId(role)),
+
.description = "ROLE PROPERTIES",
+
.section = SECTION_PRE_DATA,
+
.createStmt = querybuf->data));
}
}
@@ -1567,9 +1403,6 @@ dumpRoleMembership(PGconn *conn)
destroyPQExpBuffer(buf);
destroyPQExpBuffer(querybuf);
destroyPQExpBuffer(optbuf);
-
- if (archDumpFormat == archNull)
- fprintf(OPF, "\n\n");
}
@@ -1596,9 +1429,6 @@ dumpRoleGUCPrivs(PGconn *conn)
"FROM pg_catalog.pg_parameter_acl "
"ORDER BY 1");
- if (PQntuples(res) > 0 && archDumpFormat == archNull)
- fprintf(OPF, "--\n-- Role privileges on configuration
parameters\n--\n\n");
-
for (i = 0; i < PQntuples(res); i++)
{
PQExpBuffer buf = createPQExpBuffer();
@@ -1621,25 +1451,19 @@ dumpRoleGUCPrivs(PGconn *conn)
exit_nicely(1);
}
- if (archDumpFormat == archNull)
- fprintf(OPF, "%s", buf->data);
- else
- ArchiveEntry(fout,
- nilCatalogId, /* catalog ID */
- createDumpId(), /* dump
ID */
- ARCHIVE_OPTS(.tag =
psprintf("ROLE %s", fmtId(parowner)),
-
.description = "ROLE PROPERTIES",
-
.section = SECTION_PRE_DATA,
-
.createStmt = buf->data));
+ ArchiveEntry(fout,
+ nilCatalogId, /* catalog ID */
+ createDumpId(), /* dump ID */
+ ARCHIVE_OPTS(.tag = psprintf("ROLE
%s", fmtId(parowner)),
+ .description
= "ROLE PROPERTIES",
+ .section =
SECTION_PRE_DATA,
+ .createStmt =
buf->data));
pg_free(fparname);
destroyPQExpBuffer(buf);
}
PQclear(res);
-
- if (archDumpFormat == archNull)
- fprintf(OPF, "\n\n");
}
@@ -1669,9 +1493,6 @@ dumpTablespaces(PGconn *conn)
"WHERE spcname !~ '^pg_' "
"ORDER BY 1");
- if (PQntuples(res) > 0 && archDumpFormat == archNull)
- fprintf(OPF, "--\n-- Tablespaces\n--\n\n");
-
for (i = 0; i < PQntuples(res); i++)
{
PQExpBuffer buf = createPQExpBuffer();
@@ -1745,17 +1566,6 @@ dumpTablespaces(PGconn *conn)
"TABLESPACE", spcname,
seclabel_buf);
- if (archDumpFormat == archNull)
- {
- fprintf(OPF, "%s", buf->data);
-
- if (comment_buf->data[0] != '\0')
- fprintf(OPF, "%s", comment_buf->data);
-
- if (seclabel_buf->data[0] != '\0')
- fprintf(OPF, "%s", seclabel_buf->data);
- }
- else
{
char *tag = psprintf("TABLESPACE %s",
fmtId(fspcname));
DumpId spcDumpId = createDumpId();
@@ -1800,9 +1610,6 @@ dumpTablespaces(PGconn *conn)
destroyPQExpBuffer(delQry);
destroyPQExpBuffer(comment_buf);
destroyPQExpBuffer(seclabel_buf);
-
- if (archDumpFormat == archNull)
- fprintf(OPF, "\n\n");
}
@@ -1824,15 +1631,6 @@ dumpUserConfig(PGconn *conn, const char *username)
res = executeQuery(conn, buf->data);
- if (PQntuples(res) > 0 && archDumpFormat == archNull)
- {
- char *sanitized;
-
- sanitized = sanitize_line(username, true);
- fprintf(OPF, "\n--\n-- User Config \"%s\"\n--\n\n", sanitized);
- free(sanitized);
- }
-
for (int i = 0; i < PQntuples(res); i++)
{
resetPQExpBuffer(buf);
@@ -1840,16 +1638,13 @@ dumpUserConfig(PGconn *conn, const char *username)
"ROLE", username,
NULL, NULL,
buf);
- if (archDumpFormat == archNull)
- fprintf(OPF, "%s", buf->data);
- else
- ArchiveEntry(fout,
- nilCatalogId, /* catalog ID */
- createDumpId(), /* dump
ID */
- ARCHIVE_OPTS(.tag =
psprintf("ROLE %s", fmtId(username)),
-
.description = "ROLE PROPERTIES",
-
.section = SECTION_PRE_DATA,
-
.createStmt = buf->data));
+ ArchiveEntry(fout,
+ nilCatalogId, /* catalog ID */
+ createDumpId(), /* dump ID */
+ ARCHIVE_OPTS(.tag = psprintf("ROLE
%s", fmtId(username)),
+ .description
= "ROLE PROPERTIES",
+ .section =
SECTION_PRE_DATA,
+ .createStmt =
buf->data));
}
PQclear(res);
@@ -1911,6 +1706,55 @@ expand_dbname_patterns(PGconn *conn,
destroyPQExpBuffer(query);
}
+/*
+ * Register a drop-only archive entry for each database that dumpDatabases()
+ * will recreate. A database may depend on a tablespace or role, so it must
+ * be dropped before them. The archiver's drop pass processes entries in
+ * reverse registration order, so this must be called after dumpRoles() and
+ * dumpTablespaces().
+ *
+ * template1 and postgres are skipped: they are assumed to exist in the target
+ * cluster, and with --clean dumpDatabases() has pg_dump drop and recreate
+ * them instead. IF EXISTS, if requested, is added by the archiver.
+ */
+static void
+dropDatabases(PGconn *conn)
+{
+ PQExpBuffer delQry = createPQExpBuffer();
+ PGresult *res;
+
+ /* This must agree with dumpDatabases() */
+ res = executeQuery(conn,
+ "SELECT datname "
+ "FROM pg_catalog.pg_database "
+ "WHERE datallowconn AND datconnlimit
!= -2 "
+ "AND datname NOT IN ('template0',
'template1', 'postgres') "
+ "ORDER BY datname");
+
+ for (int i = 0; i < PQntuples(res); i++)
+ {
+ char *dbname = PQgetvalue(res, i, 0);
+
+ /* Excluded databases are not recreated, so don't drop them
either */
+ if (simple_string_list_member(&database_exclude_names, dbname))
+ continue;
+
+ resetPQExpBuffer(delQry);
+ appendPQExpBuffer(delQry, "DROP DATABASE %s;\n", fmtId(dbname));
+
+ ArchiveEntry(fout,
+ nilCatalogId, /* catalog ID */
+ createDumpId(), /* dump ID */
+ ARCHIVE_OPTS(.tag = psprintf("DATABASE
%s", fmtId(dbname)),
+ .description
= "DROP_GLOBAL",
+ .section =
SECTION_PRE_DATA,
+ .dropStmt =
delQry->data));
+ }
+
+ PQclear(res);
+ destroyPQExpBuffer(delQry);
+}
+
/*
* Dump contents of databases.
*/
@@ -1918,7 +1762,6 @@ static void
dumpDatabases(PGconn *conn)
{
PGresult *res;
- PQExpBuffer delQry = createPQExpBuffer();
int i;
char db_subdir[MAXPGPATH];
char dbfilepath[MAXPGPATH];
@@ -2009,11 +1852,11 @@ dumpDatabases(PGconn *conn)
/*
* We assume that "template1" and "postgres" already exist in
the
- * target installation and are never dropped above, for fear of
- * dropping the DB the restore script is initially connected
to. If
- * --clean was specified, tell pg_dump to drop and recreate
them;
- * otherwise we'll merely restore their contents. Other
databases
- * should simply be created.
+ * target installation and are never dropped by
dropDatabases(), for
+ * fear of dropping the DB the restore script is initially
connected
+ * to. If --clean was specified, tell pg_dump to drop and
recreate
+ * them; otherwise we'll merely restore their contents. Other
+ * databases should simply be created.
*/
if (strcmp(dbname, "template1") == 0 || strcmp(dbname,
"postgres") == 0)
{
@@ -2028,9 +1871,6 @@ dumpDatabases(PGconn *conn)
else
create_opts = "--create";
- if (filename && archDumpFormat == archNull)
- fclose(OPF);
-
if (archDumpFormat != archNull)
{
/* Compute this database's own archive file path. */
@@ -2043,36 +1883,11 @@ dumpDatabases(PGconn *conn)
/* Put one line entry for dboid and dbname in map file.
*/
fprintf(map_file, "%s %s\n", oid, dbname);
-
- /* Let the archiver drop this database first, if
--clean. */
- if (!data_only && !statistics_only && !no_schema &&
- strcmp(dbname, "template1") != 0 &&
strcmp(dbname, "postgres") != 0)
- {
- resetPQExpBuffer(delQry);
- appendPQExpBuffer(delQry, "DROP DATABASE IF
EXISTS %s;\n",
-
fmtId(dbname));
-
- ArchiveEntry(fout,
- nilCatalogId, /*
catalog ID */
- createDumpId(),
/* dump ID */
- ARCHIVE_OPTS(.tag =
psprintf("DATABASE %s", fmtId(dbname)),
-
.description = "DROP_GLOBAL",
-
.section = SECTION_PRE_DATA,
-
.dropStmt = delQry->data));
- }
}
ret = runPgDump(dbname, create_opts, dbfilepath);
if (ret != 0)
pg_fatal("pg_dump failed on database \"%s\", exiting",
dbname);
-
- if (filename && archDumpFormat == archNull)
- {
- OPF = fopen(filename, PG_BINARY_A);
- if (!OPF)
- pg_fatal("could not re-open the output file
\"%s\": %m",
- filename);
- }
}
/* Close map file */
@@ -2090,7 +1905,6 @@ dumpDatabases(PGconn *conn)
}
PQclear(res);
- destroyPQExpBuffer(delQry);
}
diff --git a/src/bin/pg_dump/t/007_pg_dumpall.pl
b/src/bin/pg_dump/t/007_pg_dumpall.pl
index debaf1e770b..7b261ae4867 100644
--- a/src/bin/pg_dump/t/007_pg_dumpall.pl
+++ b/src/bin/pg_dump/t/007_pg_dumpall.pl
@@ -445,8 +445,7 @@ unlike(
'commented out database in map.dat is not restored');
# Test 12: --clean without --if-exists should not add IF EXISTS (no
-# auto-implication). DROP DATABASE always includes IF EXISTS, unlike
-# ROLE/TABLESPACE.
+# auto-implication).
$node->command_ok(
[
'pg_restore', '-C',
@@ -469,8 +468,8 @@ like(
);
like(
$clean_output,
- qr/DROP DATABASE IF EXISTS/,
- '--clean without --if-exists: DROP DATABASE IF EXISTS in output');
+ qr/DROP DATABASE(?! IF EXISTS)/,
+ '--clean without --if-exists: DROP DATABASE without IF EXISTS in
output');
# Test 13: --clean --if-exists adds IF EXISTS to all object types.
$node->command_ok(
--
2.34.1
