Re: Non-text mode for pg_dumpall - Mailing list pgsql-hackers

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