Re: Non-text mode for pg_dumpall

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