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

On Fri, Apr 04, 2025 at 04:11:05PM -0400, Andrew Dunstan wrote:
> Thanks. I have pushed these now with a few further small tweaks.

This drops all databases:

pg_dumpall --clean -Fd -f /tmp/dump
pg_restore -d template1 --globals-only /tmp/dump

That didn't match my expectations given this help text:

$ pg_restore --help|grep global
  -g, --globals-only           restore only global objects, no databases

This happens in dropDBs().  I found that by searching pg_dumpall.c for "OPF",
which finds all the content we can write to globals.dat.

commit 1495eff wrote:
> --- a/src/bin/pg_dump/pg_dumpall.c
> +++ b/src/bin/pg_dump/pg_dumpall.c

> @@ -1612,9 +1683,27 @@ dumpDatabases(PGconn *conn)
>              continue;
>          }
>  
> +        /*
> +         * If this is not a plain format dump, then append dboid and dbname to
> +         * the map.dat file.
> +         */
> +        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);

Use appendShellString() instead.  Plain mode already does that for the
"pg_dumpall -f" argument, which is part of db_subdir here.  We don't want
weird filename characters to work out differently for plain vs. non-plain
mode.  Also, it's easier to search for appendShellString() than to search for
open-coded shell quoting.

> @@ -1641,19 +1727,30 @@ dumpDatabases(PGconn *conn)
>          if (filename)
>              fclose(OPF);
>  
> -        ret = runPgDump(dbname, create_opts);
> +        ret = runPgDump(dbname, create_opts, dbfilepath, archDumpFormat);
>          if (ret != 0)
>              pg_fatal("pg_dump failed on database \"%s\", exiting", dbname);
>  
>          if (filename)
>          {
> -            OPF = fopen(filename, PG_BINARY_A);
> +            char        global_path[MAXPGPATH];
> +
> +            if (archDumpFormat != archNull)
> +                snprintf(global_path, MAXPGPATH, "%s/global.dat", filename);
> +            else
> +                snprintf(global_path, MAXPGPATH, "%s", filename);
> +
> +            OPF = fopen(global_path, PG_BINARY_A);
>              if (!OPF)
>                  pg_fatal("could not re-open the output file \"%s\": %m",
> -                         filename);
> +                         global_path);

Minor item: plain mode benefits from reopening, because pg_dump appended to
the plain output file.  There's no analogous need to reopen global.dat, since
just this one process writes to global.dat.

> @@ -1672,17 +1770,36 @@ runPgDump(const char *dbname, const char *create_opts)
>      initPQExpBuffer(&connstrbuf);
>      initPQExpBuffer(&cmd);
>  
> -    printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin,
> -                      pgdumpopts->data, create_opts);
> -
>      /*
> -     * If we have a filename, use the undocumented plain-append pg_dump
> -     * format.
> +     * If this is not a plain format dump, then append file name and dump
> +     * format to the pg_dump command to get archive dump.
>       */
> -    if (filename)
> -        appendPQExpBufferStr(&cmd, " -Fa ");
> +    if (archDumpFormat != archNull)
> +    {
> +        printfPQExpBuffer(&cmd, "\"%s\" -f %s %s", pg_dump_bin,
> +                          dbfile, create_opts);
> +
> +        if (archDumpFormat == archDirectory)
> +            appendPQExpBufferStr(&cmd, "  --format=directory ");
> +        else if (archDumpFormat == archCustom)
> +            appendPQExpBufferStr(&cmd, "  --format=custom ");
> +        else if (archDumpFormat == archTar)
> +            appendPQExpBufferStr(&cmd, "  --format=tar ");
> +    }
>      else
> -        appendPQExpBufferStr(&cmd, " -Fp ");
> +    {
> +        printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin,
> +                          pgdumpopts->data, create_opts);

This uses pgdumpopts for plain mode only, so many pg_dumpall options silently
have no effect in non-plain mode.  Example:

strace -f pg_dumpall --lock-wait-timeout=10 2>&1 >/dev/null | grep exec
strace -f pg_dumpall --lock-wait-timeout=10 -Fd -f /tmp/dump3 2>&1 >/dev/null | grep exec

> --- a/src/bin/pg_dump/pg_restore.c
> +++ b/src/bin/pg_dump/pg_restore.c

> +/*
> + * read_one_statement
> + *
> + * This will start reading from passed file pointer using fgetc and read till
> + * semicolon(sql statement terminator for global.dat file)
> + *
> + * EOF is returned if end-of-file input is seen; time to shut down.

What makes it okay to use this particular subset of SQL lexing?

> +/*
> + * get_dbnames_list_to_restore
> + *
> + * This will mark for skipping any entries from dbname_oid_list that pattern match an
> + * entry in the db_exclude_patterns list.
> + *
> + * Returns the number of database to be restored.
> + *
> + */
> +static int
> +get_dbnames_list_to_restore(PGconn *conn,
> +                            SimpleOidStringList *dbname_oid_list,
> +                            SimpleStringList db_exclude_patterns)
> +{
> +    int            count_db = 0;
> +    PQExpBuffer query;
> +    PGresult   *res;
> +
> +    query = createPQExpBuffer();
> +
> +    if (!conn)
> +        pg_log_info("considering PATTERN as NAME for --exclude-database option as no db connection while doing
pg_restore.");

When do we not have a connection here?  We'd need to document this behavior
variation if it stays, but I'd prefer if we can just rely on having a
connection.

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

In released versions, "pg_restore --create" fails if the database exists, and
pg_restore w/o --create fails unless the database exists.  I think we should
continue that pattern in this new feature.  If not, pg_restore should document
how it treats pg_dumpall-sourced dumps with the "create if not exists"
semantics appearing here.