Re: Non-text mode for pg_dumpall

From: jian he <jian(dot)universality(at)gmail(dot)com>
To: Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
Cc: Srinath Reddy <srinath2133(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Non-text mode for pg_dumpall
Date: 2025-02-11 15:09:40
Message-ID: CACJufxEAWxWovH_dys5A2z53OGpftX_jJRTd1y8eB3HaojY+1A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

+ /* 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?

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.

+ /*
+ * 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");

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

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

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

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.

Attachment Content-Type Size
v16_misc_changes.nocfbot application/octet-stream 6.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2025-02-11 15:14:04 Re: should we have a fast-path planning for OLTP starjoins?
Previous Message Matheus Alcantara 2025-02-11 14:51:28 Re: explain analyze rows=%.0f