Subject: Re: Non-text mode for pg_dumpall

From: jian he <[email-masked]>
Date: Thu, 16 Jan 2025 13:17:51 +0800

hi.

in master src/bin/pg_dump/pg_restore.c: main function
    if (opts->tocSummary)
        PrintTOCSummary(AH);
    else
    {
        ProcessArchiveRestoreOptions(AH);
        RestoreArchive(AH);
    }
opts->tocSummary is true (pg_restore --list), no query will be executed.
but your patch (pg_restore --list) may call execute_global_sql_commands,
which executes a query.

        sscanf(line, "%u" , &db_oid);
        sscanf(line, "%s" , db_oid_str);
i think it would be better
        sscanf(line, "%u %s" , &db_oid, db_oid_str);

in doc/src/sgml/ref/pg_dumpall.sgml
Note: This option can be omitted only when --format=p|plain.
maybe change to
Note: This option can be omitted only when <option>--format</option> is plain.

--format=format section:
""
Under this databases subdirectory, there will be subdirectory with
dboid name for each database.
""
this sentence is not correct? because
drwxr-xr-x   databases
.rw-rw-r--    global.dat
.rw-rw-r--    map.dat

"databases" is a directory, and under the "database" directory, it's a
list of files.
each file filename is corresponding to a unique database name
so there is no subdirectory under subdirectory?

in src/bin/pg_dump/meson.build
you need add  'common_dumpall_restore.c', to the pg_dump_common_sources section.
otherwise meson build cannot compile.

$BIN6/pg_restore --dbname=src6 --verbose --list $SRC6/dumpall.custom6
pg_restore: error: option -C/--create should be specified when using
dump of pg_dumpall
this command should not fail?

in doc/src/sgml/ref/pg_restore.sgml
     <varlistentry>
      ...
      <term><option>--format=<replaceable
class="parameter">format</replaceable></option></term>
also need
<term><literal>plain</literal></term>
?

---

From: jian he <[email-masked]>
Date: Thu, 16 Jan 2025 16:44:18 +0800

hi.

$BIN6/pg_dumpall --format=directory --verbose --file=test1
pg_dumpall: executing SELECT pg_catalog.set_config('search_path', '', false);
pg_dumpall: error: could not create directory "test1": File exists

we should first validate --file option, if not ok error out immediately.
if ok then connect to db then run the sql query?

create_or_open_dir also needs to change.

The attached is the minor change I came up with.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Fri, 17 Jan 2025 00:12:32 +0530

Thanks Jian.

> The attached is the minor change I came up with.

As per your comment and suggestions, I merged the delta patch. I
think, many places we are validating files after connection also.

> but your patch (pg_restore --list) may call execute_global_sql_commands,
Okay. I will do more study for this case.

>         sscanf(line, "%u %s" , &db_oid, db_oid_str);

No, we can't use this as dbname can be complex with multiple spaces.
Ex: create database "database db is long string";
If we use %s, it will read only the first string till space.
We can use something like: sscanf("%u %2000[^\n]s", &db_oid, db_oid_str);

> Note: This option can be omitted only when <option>--format</option> is plain.

Fixed.

> so there is no subdirectory under subdirectory?

If it is a directory format, then we will create a subdirectory. I did
some modifications to this para in the latest patch.

> otherwise meson build cannot compile.

I think we should not add under pg_dump_common_sources, rather we
should add it into pg_dumpall and pg_restore only.
I added this.

> this command should not fail?

If a dump has multiple databases, then we should use -C option
otherwise all dumps will be restored in a single db. As of
now I removed this error and changed this to pg_log_info.

> <term><literal>plain</literal></term>

plain format is not supported with pg_restore. I added an error for this format.

Here, I am attaching an updated patch for review and testing.

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