Re: Non-text mode for pg_dumpall

On Thu, Feb 26, 2026 at 09:02:48AM -0500, Andrew Dunstan wrote:
> pushed with a slight tweak.

Having now reviewed commit 763aaa0, I don't think it's ready to remain part of
v19. While some points from my v18 review are now resolved, other points
still seem unresolved. I didn't find discussion of the unresolved points. I
also see new issues.

Wider issues:

> @@ -796,24 +964,45 @@ dropRoles(PGconn *conn)

> + if (archDumpFormat == archNull)
> + {
> + appendPQExpBuffer(delQry, "DROP ROLE %s%s;\n",
> + if_exists ? "IF EXISTS " : "",
> + fmtId(rolename));
> + fprintf(OPF, "%s", delQry->data);
> + }
> + else
> + {
> + appendPQExpBuffer(delQry, "DROP ROLE IF EXISTS %s;\n",
> + fmtId(rolename));
> +
> + ArchiveEntry(fout,
> + nilCatalogId, /* catalog ID */
> + createDumpId(), /* dump ID */
> + ARCHIVE_OPTS(.tag = psprintf("ROLE %s", fmtId(rolename)),
> + .description = "DROP_GLOBAL",
> + .section = SECTION_PRE_DATA,
> + .createStmt = delQry->data));
> + }

pg_dumpall.c should call ArchiveEntry() in the same way pg_dump.c does. In
pg_dump.c, per-object-class support code calls ArchiveEntry() unconditionally,
and the object-independent infra of pg_backup_archiver.c deals with the
difference between plain and non-plain formats.

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.)

Separately, DROP should be in dropStmt, not in createStmt. This likely
entails refactoring to merge dumpRoles() and dropRoles() into one function,
per the style of pg_dump.c.

> + /*
> + * For pg_dumpall archives, --clean implies --if-exists since global
> + * objects may not exist in the target cluster.
> + */
> + if (opts->dropSchema && !opts->if_exists)
> + {
> + opts->if_exists = 1;
> + pg_log_info("--if-exists is implied by --clean for pg_dumpall archives");
> + }

The last comment of postgr.es/m/20250708212819.09.nmisch@google.com disagreed
with this decision, so that remains unresolved.

> + /* If database is already created, then don't set createDB flag. */
> + if (tmpopts->cparams.dbname)
> + {
> + PGconn *test_conn;
> +
> + test_conn = ConnectDatabase(dbidname->str, NULL, tmpopts->cparams.pghost,
> + tmpopts->cparams.pgport, tmpopts->cparams.username, TRI_DEFAULT,
> + false, progname, NULL, NULL, NULL, NULL);
> + if (test_conn)
> + {
> + PQfinish(test_conn);
> +
> + /* Use already created database for connection. */
> + tmpopts->createDB = 0;
> + tmpopts->cparams.dbname = dbidname->str;
> + }
> + else
> + {
> + /* We'll have to create it */
> + tmpopts->createDB = 1;
> + tmpopts->cparams.dbname = connected_db;
> + }

postgr.es/m/20250708212819.09.nmisch@google.com called for this to change.

None of this is meant to say the feature is impossible. But I don’t think this
commit is at the point where post-commit fixups are the right workflow. I
recommend reverting, posting a new version, and letting commitfest review
finish.

Localized issues:

There's new code to write map and globals files, but I don't see corresponding
code to fsync those files, like we fsync a plain-format dump.

> @@ -3027,6 +3049,16 @@ _tocEntryRequired(TocEntry *te, teSection curSection, ArchiveHandle *AH)
> return 0;
> }
>
> + /*
> + * Global object TOC entries (e.g., ROLEs or TABLESPACEs) must not be
> + * ignored.
> + */
> + if (strcmp(te->desc, "ROLE") == 0 ||
> + strcmp(te->desc, "ROLE PROPERTIES") == 0 ||
> + strcmp(te->desc, "TABLESPACE") == 0 ||
> + strcmp(te->desc, "DROP_GLOBAL") == 0)
> + return REQ_SCHEMA;
> +

If I delete this addition, no src/bin/pg_dump test fails. Would you explain
the rationale for this addition?

> + if (comment_buf->data[0] != '\0')
> + ArchiveEntry(fout,
> + nilCatalogId, /* catalog ID */
> + createDumpId(), /* dump ID */
> + ARCHIVE_OPTS(.tag = tag,
> + .description = "COMMENT",
> + .section = SECTION_PRE_DATA,
> + .createStmt = comment_buf->data));
> +
> + if (seclabel_buf->data[0] != '\0')
> + ArchiveEntry(fout,
> + nilCatalogId, /* catalog ID */
> + createDumpId(), /* dump ID */
> + ARCHIVE_OPTS(.tag = tag,
> + .description = "SECURITY LABEL",
> + .section = SECTION_PRE_DATA,
> + .createStmt = seclabel_buf->data));

COMMENT and SECURITY LABEL should use .deps and .nDeps to record a dependency
on the main object. Representative example from pg_dump.c:

ArchiveEntry(fout, nilCatalogId, createDumpId(),
ARCHIVE_OPTS(.tag = target->data,
.namespace = tbinfo->dobj.namespace->dobj.name,
.owner = tbinfo->rolname,
.description = "SECURITY LABEL",
.section = SECTION_NONE,
.createStmt = query->data,
.deps = &(tbinfo->dobj.dumpId),
.nDeps = 1));

This probably has no user-visible consequences, since "/* Parallel execution
is not supported for global object restoration. */". Still, best to follow
the standard.

> + /*
> + * If this is not a plain format dump, then append dboid and dbname to
> + * the map.dat file.
> + */

The first six lines inside the block aren't for the purpose described here.
The one line that is for this purpose has its own comment. Please delete this
comment.

> + if (archDumpFormat != archNull)
> + {
> + if (archDumpFormat == archCustom)
> + snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".dmp", db_subdir, oid);
> + else if (archDumpFormat == archTar)
> + snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".tar", db_subdir, oid);
> + else
> + snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid);

My review in postgr.es/m/20250708212819.09.nmisch@google.com called for this
to change. I'm finding no discussion of the rationale for not changing it.

> +
> + /* Put one line entry for dboid and dbname in map file. */
> + fprintf(map_file, "%s %s\n", oid, dbname);
> + }

> +runPgDump(const char *dbname, const char *create_opts, char *dbfile)

> + if (archDumpFormat != archNull)
> + {
> + printfPQExpBuffer(&cmd, "\"%s\" %s -f %s %s", pg_dump_bin,
> + pgdumpopts->data, dbfile, create_opts);

A different "-f" argument is already in pgdumpopts. That works, but it's
untidy.

> + pg_fatal("option %s cannot exclude %s when restoring a pg_dumpall archive",
> + "--section", "--pre-data");

s/--pre-data/pre-data/

> + /*
> + * Always restore global objects, even if --exclude-database results
> + * in zero databases to process. If 'globals-only' is set, exit
> + * immediately.

I think this comment is out of date, because the code doesn't have an exit:

> + */
> + snprintf(global_path, MAXPGPATH, "%s/toc.glo", inputFileSpec);
> +
> + n_errors = restore_global_objects(global_path, tmpopts);
> +
> + if (globals_only)
> + pg_log_info("database restoring skipped because option %s was specified",
> + "-g/--globals-only");
> + else
> + {
> + /* Now restore all the databases from map.dat */
> + n_errors = n_errors + restore_all_databases(inputFileSpec, db_exclude_patterns,
> + opts, numWorkers);
> + }
> +
> + /* Free db pattern list. */
> + simple_string_list_destroy(&db_exclude_patterns);
> + }

> + /* Don't output TOC entry comments when restoring globals */
> + ((ArchiveHandle *) AH)->noTocComments = 1;

Why not?

> +static int
> +get_dbnames_list_to_restore(PGconn *conn,
> + SimplePtrList *dbname_oid_list,
> + SimpleStringList db_exclude_patterns)
> +{
...
> + if (pg_strcasecmp(dbidname->str, pat_cell->val) == 0)
> + skip_db_restore = true;
> + /* Otherwise, try a pattern match if there is a connection */

The code assumes a connection unconditionally (which seems right to me):

> + else
> + {
> + int dotcnt;
> +
> + appendPQExpBufferStr(query, "SELECT 1 ");
> + processSQLNamePattern(conn, query, pat_cell->val, false,
> + false, NULL, db_lit->data,
> + NULL, NULL, NULL, &dotcnt);
> +
> + if (dotcnt > 0)
> + {
> + pg_log_error("improper qualified name (too many dotted names): %s",
> + dbidname->str);
> + PQfinish(conn);
> + exit_nicely(1);
> + }
> +
> + res = executeQuery(conn, query->data);