Re: Non-text mode for pg_dumpall

On Tue, 11 Feb 2025 at 20:40, jian he <jian.universality@gmail.com> wrote:
>
> hi.
> review based on v16.
>
> because of
>
https://postgr.es/m/CAFC+b6pWQiSL+3rvLxN9vhC8aONp4OV9c6u+BVD6kmWmDbd1WQ@mail.gmail.com
>
> in copy_global_file_to_out_file, now it is:
>     if (strcmp(outfile, "-") == 0)
>         OPF = stdout;
> I am confused, why "-" means stdout.
> ``touch ./- `` command works fine.
> i think dash is not special character, you may see
> https://stackoverflow.com/a/40650391/15603477

"-" is used for stdout. This is mentioned in the doc.
pg_restore link <https://www.postgresql.org/docs/current/app-pgrestore.html>

> -f *filename*
> --file=*filename*
>
> Specify output file for generated script, or for the listing when used
> with -l. Use - for stdout.
>

>
>
> + /* Create a subdirectory with 'databases' name under main directory. */
> + if (mkdir(db_subdir, 0755) != 0)
> + pg_log_error("could not create subdirectory \"%s\": %m", db_subdir);
> here we should use pg_fatal?

Yes, we should use pg_fatal.

>
>
> pg_log_info("executing %s", sqlstatement.data);
> change to
> pg_log_info("executing query: %s", sqlstatement.data);
> message would be more similar to the next pg_log_error(...) message.

Okay.

>
>
> + /*
> + * User is suggested to use single database dump for --list option.
> + */
> + if (opts->tocSummary)
> + pg_fatal("option -l/--list cannot be used when using dump of
pg_dumpall");
> maybe change to
> + pg_fatal("option -l/--list cannot be used when restoring multiple
databases");

okay.

>
> $BIN10/pg_restore --format=directory --list dir10_x
> if the directory only has one database, then we can actually print out
> the tocSummary.
> if the directory has more than one database then pg_fatal.
> To tolerate this corner case (only one database) means that pg_restore
> --list requires a DB connection,
> but I am not sure that is fine.
> anyway, the attached patch allows this corner case.

No, we don't need this corner case. If a user wants to restore a
single database with --list option, then the user should give a particular
dump file with pg_restore.

>
>
> PrintTOCSummary can only print out summary for a single database.
> so we don't need to change PrintTOCSummary.
>
>
> + /*
> + * To restore multiple databases, -C (create database) option should
> be specified
> + * or all databases should be created before pg_restore.
> + */
> + if (opts->createDB != 1)
> + pg_log_info("restoring dump of pg_dumpall without -C option, there
> might be multiple databases in directory.");
>
> we can change it to
> + if (opts->createDB != 1 && num_db_restore > 0)
> + pg_log_info("restoring multiple databases without -C option.");

okay.

>
>
> Bug.
> when pg_restore --globals-only can be applied when we are restoring a
> single database (can be an output of pg_dump).

As of now, we are ignoring this option. We can add an error in the "else"
part of the global.dat file.
Ex: option --globals-only is only supported with dump of pg_dumpall.
Similarly --exclude-database also.

>
>
> There are some tests per https://commitfest.postgresql.org/52/5495, I
> will check it later.
> The attached patch is the change for the above reviews.


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