Re: Non-text mode for pg_dumpall

Thanks Jian.

On Thu, 16 Jan 2025 at 14:14, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> 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.

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

> }
> 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.
>
Okay. I will do more study for this case.

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

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

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

Fixed.

>
> --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?

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

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

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.

>
> $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?

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.

>
>
> 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>
> ?

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