Re: Non-text mode for pg_dumpall

On Mon, 12 Oct 2026 at 11:14, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
wrote:
>
> On Thu, 8 Oct 2026 at 13:39, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >
> > On Thu, Aug 27, 2026 at 2:23 PM Mahendra Singh Thalor
> > <mahi6run(at)gmail(dot)com> 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/20250708212819.09.nmisch@google.com 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/
>
> Hi Jian, Noah,
>
> Thanks, Jian, for the v2 patches. I've merged them, fixed a couple of
> bugs I found while testing, and attached v03. Patches 0001-0003 are
> the check_mut_excl_opts() cleanups for pg_restore, pg_dumpall and
> pg_dump, unchanged. 0004 is the feature, with your v2 changes and
> the fixes below folded in.
>
> On Thu, Oct 8, 2026 at 15:39, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> > 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.
>
> Agreed, and that's what v3 does. It addresses Noah's point [1] that
> there should be one place building each DROP statement, not one for
> plain format and another for the other formats:
>
> - Global objects go through ArchiveEntry() in every output format.
> For plain format they go into an in-memory archNull archive, which
> is written out with RestoreArchive() before the first per-database
> pg_dump call. dropDBsText(), dropTablespacesText() and
> dropRolesText() are gone, as are all the
> "if (archDumpFormat == archNull) fprintf(OPF, ...)" branches in the
> dump functions.
>
> - The archiver's drop pass now handles --clean for every format,
> dropping databases, then tablespaces, then roles. --if-exists is
> also applied the same way everywhere. Before, DROP DATABASE always
> had IF EXISTS in archive formats, DROP ROLE and DROP TABLESPACE
> never did, and plain format followed --if-exists.
>
> I tested the write ordering with -f and with stdout, with and without
> -v. The globals are fully written before pg_dump -Fa appends, and the
> \restrict/\unrestrict lines wrap them correctly. The plain output
> contains exactly the same SQL statements as before.
>
> Changes on top of v2:
>
> - The archive handle has a new flag marking it as pg_dumpall's globals
> archive. pg_dumpall sets it for plain output, and pg_restore sets it
> when restoring toc.glo. With it set, the archiver leaves out its
> "PostgreSQL database dump" header and footer, the "Dumped by pg_dump
> version" line and its own timestamps, since pg_dumpall prints its
> own. It also emits "SET default_transaction_read_only = off;"
> itself, which replaces the TOC scan in _doSetFixedOutputState() and
> the DEFAULT_TRANSACTION_READ_ONLY TOC entry.
>
> - Reverted the "fmt,..." formatting changes in pg_backup_archiver.c.
> The current pgindent keeps "fmt, ...", so I think your pgindent is
> out of date. I then ran the current pgindent over the changed files.
>
> - Flattened the bare { } blocks left in main(), dumpRoles() and
> dumpTablespaces().
>
> - Put back fout->verbose for archive formats, and the comment saying
> that drop statements are always included in archive formats and
> only used if pg_restore is given --clean.
>
> - check_for_invalid_global_names() keeps your SQL-side filtering but
> goes back to listing every offending name, not just the first.
> Otherwise users have to fix one name, rerun, and hit the next.
>
> - Fixed the tablespace TOC tag, which was quoted twice
> (fmtId(fspcname), where fspcname is already quoted).
>
> Two bugs found while testing (both were already in v1):
>
> 1. pg_restore -C --clean dropped every database twice: once by its
> DROP_GLOBAL entry in toc.glo, and again by its own per-database
> archive, which pg_dump wrote with --create. The second drop always
> failed with 'database "..." does not exist', one error per database
> on every restore.
>
> 2. Worse, toc.glo was restored before --exclude-database was
> processed. So "pg_restore -C --clean --exclude-database=db1"
> dropped db1 and then never restored it, losing its data. I
> reproduced this on v1.
>
> In v3, each DROP_GLOBAL entry records its database's OID (as in
> map.dat) in its catalogId. pg_restore reads map.dat and applies
> --exclude-database before restoring the globals, runs only the
> DROP_GLOBAL entries for databases it is about to restore, and turns
> off --clean for the per-database restore of any database the
> globals pass already dropped. With --no-globals, the per-database
> archives still drop their own databases. t/007_pg_dumpall.pl has
> new tests for both cases.
>
> Behaviour changes worth pointing out:
>
> - Plain output no longer has the "-- Roles", "-- Drop roles",
> "-- User Config ..." section comments, consistent with how
> pg_restore already writes the globals (noTocComments). It also has
> a few more SET lines at the top (statement_timeout, lock_timeout,
> check_function_bodies, row_security, ...), which are harmless.
>
> - With plain format, --clean no longer drops databases excluded with
> --exclude-database. Before, they were dropped and never recreated.
>
> - For archive formats, DROP DATABASE no longer gets IF EXISTS unless
> --if-exists is given, matching pg_dump. Restoring with --clean into
> a cluster that doesn't have those databases gives one error per
> database without --if-exists, as it does with pg_dump.
>
> Noah's earlier review [2] is addressed:
>
> - pg_restore -g with a --clean dump dropping all databases: -g and
> --clean are now rejected together for pg_dumpall archives.
> - appendShellString() for the per-database file path: done.
> - No need to reopen global.dat: only plain mode reopens its file.
> - pg_dumpall options ignored in non-plain modes: pgdumpopts is now
> passed to pg_dump for every format.
> - read_one_statement() and its partial SQL lexing: gone, since the
> globals are now an archive.
> - --exclude-database without a connection: pg_restore now always
> connects, or fails.
> - "create the database if it doesn't exist": gone. A full restore
> requires -C, and CREATE DATABASE fails if the database exists, as
> in released versions.
>
> Open questions:
>
> a) pg_restore -g --clean is currently refused for pg_dumpall
> archives. Should it instead drop and recreate only the roles and
> tablespaces?
>
> b) With -C --clean, restoring the database pg_restore is connected to
> (normally "postgres") always reports "cannot drop the currently
> open database" and then "already exists". Plain pg_dumpall
> --clean has the same problem, because it also has pg_dump drop and
> recreate postgres and template1, so whichever one psql is
> connected to fails. Is this worth handling in pg_restore, e.g. by
> skipping the drop of the database it is connected to, or should we
> just document it?
>
> The pg_dump and pg_upgrade TAP tests pass.
>
> [1] https://postgr.es/m/20260607000218.96.noahmisch@microsoft.com
> [2] https://postgr.es/m/20250708212819.09.nmisch@google.com
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com
>
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com

Hi all,

Attached is v04. Patches 0001-0003 are unchanged from v03. 0004 has
two changes to how pg_restore handles a pg_dumpall archive, both
found while testing --clean restores into a live cluster.

1. *Restoring the database pg_restore is connected to*

With --clean, pg_dumpall archives drop and recreate postgres and
template1, as plain pg_dumpall --clean does. In v03, restoring with
"-C --clean -d postgres" failed for postgres:

pg_restore: error: could not execute query: ERROR: cannot drop the
currently open database
Command was: DROP DATABASE postgres;
pg_restore: error: could not execute query: ERROR: database "postgres"
already exists

and then restored postgres's contents on top of its existing ones.
If postgres already had tables, their data was loaded a second time.
Using "-d template1" doesn't help, since template1 hits the same
error.

In v04, when the database being restored is the one pg_restore is
connected to, its restore connects to template1 instead (or to
postgres, when restoring template1) to drop and recreate it. CREATE
DATABASE always uses template0 as its template, so that connection
doesn't get in its way. The globals pass likewise leaves that
database's drop to its own restore. This also covers "-d db1" when
db1 is in the dump. With -v, pg_restore logs:

pg_restore: connecting to database "template1" to restore database
"postgres"

*2. --exit-on-error and errors in global objects*

Errors while restoring the global objects never stop the restore,
even with -e/--exit-on-error, because some are expected: the role
pg_restore is connected as can't be dropped, and creating a role that
already exists fails, though the ALTER ROLE that follows still sets
its properties. Each global statement runs in its own transaction, so
a failure doesn't affect the others.

v03 did this silently, which looked like -e being ignored. v04
documents it under --exit-on-error in pg_restore.sgml, and reports the
global errors separately, as it already did for each database:

pg_restore: warning: errors ignored on restore of global objects: 2
pg_restore: hint: Option -e/--exit-on-error does not apply to global
objects, because some errors are expected while restoring them.

The hint is printed only with -e. -e still applies to every database
restored afterward.

Tests for both are in t/007_pg_dumpall.pl. The pg_dump TAP tests
pass.

*Still open:*

a)* Order of drops*. postgres and template1, and now the -d database,
are dropped by their own restore, after the globals pass has
already tried to drop the roles and tablespaces. A role or
tablespace that one of those databases depends on therefore fails
to drop ("cannot be dropped because some objects depend on it",
or "tablespace is not empty"), and its CREATE then reports
"already exists". Plain pg_dumpall --clean behaves the same way.
A complete fix would drop every database first, each from a
suitable connection, and only then drop roles and tablespaces. Is
that worth doing in this patch, or as a follow-up?

b) *pg_restore -g --clean* is still refused for pg_dumpall archives.
Should it instead drop and recreate only the roles and
tablespaces?

c) *If -d is given* as a connection string ("dbname=postgres
host=..."), the check in 1 doesn't recognise the database, and
the old error comes back.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com