Subject: Re: Non-text mode for pg_dumpall

From: jian he <[email-masked]>
Date: Tue, 21 Jan 2025 00:01:42 +0800

hi.
some minor issues come to my mind when I look at it again.

looking at set_null_conf,
i think "if (archDumpFormat != archNull)" can be:

if (archDumpFormat != archNull)
{
OPF = fopen(toc_path, "w");
if (!OPF)
    pg_fatal("could not open global.dat file: \"%s\" for writing: %m",
toc_path);
}

some places we use ``fopen(filename, PG_BINARY_W)``,
some places we use ``fopen(filename, "w");``
kind of inconsistent...

+    printf(_("  -F, --format=c|d|t|p         output file format
(custom, directory, tar,\n"
+                "                            plain text (default))\n"));
this indentation level is not right?
if we look closely at the surrounding output of `pg_dumpall --help`.

pg_dump.sgml --create option description:
This option is ignored when emitting an archive (non-text) output file. For the
archive formats, you can specify the option when you call pg_restore.

in runPgDump, we have:
/*
* If this is non-plain format dump, then append file name and dump
* format to the pg_dump command to get archive dump.
*/
if (archDumpFormat != archNull)
{
    printfPQExpBuffer(&cmd, "\"%s\" -f %s %s", pg_dump_bin,
                        dbfile, create_opts);
    ...
}

so in here, create_opts is not necessary per pg_dump.sgml above description.
we can simplify it as:

if (archDumpFormat != archNull)
{
    printfPQExpBuffer(&cmd, "\"%s\" --file=%s", pg_dump_bin, dbfile);
}
?

---

From: jian he <[email-masked]>
Date: Tue, 21 Jan 2025 12:05:58 +0800

hi.

$BIN10/pg_restore --globals-only --verbose --file=test.sql x.dump
it will create a "test.sql" file, but it should create file test.sql
(no double quotes).

------<>>>>------
if (archDumpFormat != archNull &&
        (!filename || strcmp(filename, "") == 0))
{
    pg_log_error("options -F/--format=d|c|t requires option
-f/--filename with non-empty string");
    ...
}
here, it should be
pg_log_error("options -F/--format=d|c|t requires option -f/--file with
non-empty string");

------<>>>>------
the following pg_dumpall, pg_restore not working.
$BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
$BIN10/pg_restore --file=3.sql x1.dump

ERROR: pg_restore: error: directory "x1.dump" does not appear to be a
valid archive ("toc.dat" does not exist)

these two also not working:
$BIN10/pg_dumpall --format=custom --file=x1.dump --verbose --globals-only
$BIN10/pg_restore --file=3.sql --format=custom x1.dump

error message:
pg_restore: error: could not read from input file: Is a directory
------<>>>>------
IsFileExistsInDirectory function is the same as _fileExistsInDirectory.
Can we make _fileExistsInDirectory extern function?

+        /* If global.dat and map.dat are exist, then proces them. */
+        if (IsFileExistsInDirectory(pg_strdup(inputFileSpec), "global.dat")
+                && IsFileExistsInDirectory(pg_strdup(inputFileSpec),
"map.dat"))
+        {
comment typo, "proces" should "process".
here, we don't need pg_strdup?
------<>>>>------
 # pg_restore tests
+command_fails_like(
+    [
+        'pg_restore', '-p', $port, '-f', $plainfile,
+        "--exclude-database=grabadge",
+        '--globals-only'
+    ],
+    qr/\Qg_restore: error: option --exclude-database cannot be used
together with -g\/--globals-only\E/,
+    'pg_restore: option --exclude-database cannot be used together
with -g/--globals-only');

We can put the above test on src/bin/pg_dump/t/001_basic.pl,
since validating these conflict options don't need a cluster to be set up.

typedef struct SimpleDatabaseOidListCell
and
typedef struct SimpleDatabaseOidList
need also put into src/tools/pgindent/typedefs.list

---

From: jian he <[email-masked]>
Date: Tue, 21 Jan 2025 17:29:30 +0800

hi.

+ printfPQExpBuffer(query,
+ "SELECT substring ( "
+ " '%s' , "
+ " '%s' ) ", str, ptrn);
+   result = executeQuery(conn, query->data);
+ if (PQresultStatus(result) == PGRES_TUPLES_OK)
+ {
+ if (PQntuples(result) == 1)
+ {
+ const char *outstr;
+
+ outstr = PQgetvalue(result, 0, 0);
i think here you should use PQgetisnull(result, 0, 0)
?

example: pg_dumpall and pg_restore:
$BIN10/pg_dumpall --verbose --format=custom --file=x12.dump
$BIN10/pg_restore --verbose --dbname=src10 x12.dump

some log message for the above command:
pg_restore: found dbname as : "template1" and db_oid:1 in map.dat file
while restoring
pg_restore: found dbname as : "s1" and db_oid:17960 in map.dat file
while restoring
pg_restore: found dbname as : "src10" and db_oid:5 in map.dat file
while restoring
pg_restore: found total 3 database names in map.dat file
pg_restore: needs to restore 3 databases out of 3 databases
pg_restore: restoring dump of pg_dumpall without -C option, there
might be multiple databases in directory.
pg_restore: restoring database "template1"
pg_restore: connecting to database for restore
pg_restore: implied data-only restore
pg_restore: restoring database "s1"
pg_restore: connecting to database for restore
pg_restore: processing data for table "public.t"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3376; 0 17961 TABLE DATA t jian
pg_restore: error: could not execute query: ERROR:  relation
"public.t" does not exist
Command was: COPY public.t (a) FROM stdin;

1. message: "pg_restore: implied data-only restore"
Normally pg_dump and  pg_restore will dump the schema and the data,
then when we are connecting to the same database with pg_restore,
there will be lots of schema elements already exists ERROR.
but the above command case, pg_restore only restores the content/data
not schema, that's why there is very little error happening.
so here pg_restore not restore schema seems not ok?

2. pg_dumpall with non-text mode, we don't have \connect command in
file global.dat or map.dat
I have database "s1" with table "public.t".
if I create a table src10.public.t (database.schema.table)  with column a.
then pg_restore will restore content of s1.public.t (database s1) to
src10.public.t (database src10).

in ConnectDatabase(Archive *AHX,
                const ConnParams *cparams,
                bool isReconnect)
i added
    if (cparams->dbname)
        fprintf(stderr, "pg_backup_db.c:%d %s called connecting to %s
now\n", __LINE__, __func__, cparams->dbname);
to confirm that we are connecting the same database "src10", while
dumping all the contents in x12.dump.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Wed, 22 Jan 2025 00:26:17 +0530

Thanks Jian for the detailed review and testing.

> some places we use ``fopen(filename, "w");``

Fixed. We should use PG_BINARY_W/PG_BINARY_R.

> if we look closely at the surrounding output of `pg_dumpall --help`.

Fixed.

>     printfPQExpBuffer(&cmd, "\"%s\" --file=%s", pg_dump_bin, dbfile);

We are already using the same code without this patch also. I haven't
tested this without create_opts. I think, if your theory is right,
then
you can submit a patch for this change in another thread.

> it will create a "test.sql" file, but it should create file test.sql

Fixed.

Fixed.

Fixed.

> $BIN10/pg_restore --file=3.sql --format=custom x1.dump

Fixed.

> pg_restore: error: could not read from input file: Is a directory

Fixed.

No, we can't make it as we are using this function in different-2 modules.

> comment typo, "proces" should "process".

Fixed.

> here, we don't need pg_strdup?

In most places, we are dumping strings so I kept the same here also.

Done.

> need also put into src/tools/pgindent/typedefs.list

Fixed.

> i think here you should use PQgetisnull(result, 0, 0)

Fixed.

> dumping all the contents in x12.dump.

I will do some more study for this and will update. As of now, I added
the "--create" option in the dump.

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

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

---

From: jian he <[email-masked]>
Date: Thu, 23 Jan 2025 17:28:36 +0800

hi.
The four patches attached are to solve the
TODO1: We need to think for --exclude-database=PATTERN for pg_restore.
it is based on your v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch

0001. pg_dumpall --exclude-database=PATTERN already works,
main function resolve pattern matching is expand_dbname_patterns.
make it an extern function, so pg_restore --exclude-database can also use it.

0002 cosmetic code changes not in pg_restore.c
0003 cosmetic code changes in pg_restore.c

0004 fully implement pg_restore --exclude-database=PATTERN
similar to pg_dumpall.c
declare two file static variables:
static SimpleStringList database_exclude_names = {NULL, NULL};
static SimpleStringList db_exclude_patterns = {NULL, NULL};
I also deleted the function is_full_pattern.

I use
$BIN10/pg_restore --exclude-database=*x* --exclude-database=*s*
--exclude-database=*t* --verbose --file=test.sql x1.dump
the verbose message to verify my changes.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 23 Jan 2025 16:05:31 +0530

> make it an extern function, so pg_restore --exclude-database can also use it.

Hi Jian,
We can't use the same expand_dbname_patterns function pg_restore.

In the 1st patch, by mistake I also used this function but then I
realised that we should not use this function due to some limitation
for pg_restore.

While doing pg_dumpall, we have all the existence database names in
the pg_database catalog but while restoring, we don't have all
databases in the catalog.
Actually, we will read dbnames from map.dat file to skip matching
patterns for restore.

Ex: let say we have a fresh server with postgres and template1
databases. Now we want to restore one backup
and inside the map.dat file, we have dbname=db_123 and dbname=db_234.
If we want to use --exclude-database=db_123, then
your patch will not work as this db hasn't been created.

Please cross verify again and let me know your feedback.
I think, as of now, mine v11 patch is working as per expectation.

---

From: jian he <[email-masked]>
Date: Fri, 24 Jan 2025 23:20:03 +0800

> Actually, we will read dbnames from map.dat file to skip matching

hi.
After some tests and thinking about your reply, I admit that using
expand_dbname_patterns
in pg_restore will not work.
We need to do pattern matching against the map.dat file.
Please check the attached v12 series based on your
v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch

v12-0001 cosmetic change.
v12-0002 implement pg_resore --exclude-database=PATTERN.
main gist of implementation:
for each database name in map.dat file,
check if this database name pattern matches with PATTERN or not.
pattern matching is using processSQLNamePattern.

your substring will not work.
some of the test cases.
$BIN10/pg_restore --exclude-database=* -Cd template1 --verbose dir10 >
dir_format 2>&1
$BIN10/pg_restore --exclude-database=*x* -Cd template1 --verbose dir10
> dir_format 2>&1
$BIN10/pg_restore --exclude-database=?* -Cd template1 --verbose dir10
> dir_format 2>&1

---

From: jian he <[email-masked]>
Date: Sun, 26 Jan 2025 22:46:45 +0800

hi.
attached patching trying to refactor ReadOneStatement
for properly handling the single and double quotes.
the commit message also has some tests on it.

it is based on your
v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Sun, 26 Jan 2025 21:18:16 +0530

> $BIN10/pg_restore --exclude-database=* -Cd template1 --verbose dir10 >

Hi,
As per discussion with Robert Haas and Dilip Kumar, we thought that we
can't assume that
there will be a db connection every time while doing pg_restore but in
attached patch, we are
assuming that we have a db connection.
In my previous updates, I already mentioned this problem. I think, we
should not use connection
for --exclude-database, rather we should use direct functions to
validate patterns or we should
restrict as NAME only.

> v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch.

I think, instead of char, if we read line by line, then we don't need
that much code and need not to worry about double quotes.
In the next version, I will merge some patches and will change it to
read line by line.

---

From: Srinath Reddy <[email-masked]>
Date: Tue, 28 Jan 2025 10:19:11 +0530

Hi mahendra,

I have reviewed the code in the v11 patch and it looks good to me.

But in common_dumpall_restore.c there's  parseDumpFormat which is common
between pg_dumpall and pg_restore ,as per the discussion in [1] thread i
don't think we should create a common api ,as discussed in the thread there
might chances in the future we might decide that some format is obsolete
and desupport it in pg_dumpall ,while support in pg_restore for
compatibility reasons.

[1]
https://www.postgresql.org/message-id/flat/CAFC%2Bb6pfK-BGcWW1kQmtxVrCh-JGjB2X02rLPQs_ZFaDGjZDsQ%40mail.gmail.com

Regards,
Srinath Reddy Sadipiralla,
EDB: http://www.enterprisedb.com

---

From: Srinath Reddy <[email-masked]>
Date: Tue, 28 Jan 2025 11:51:52 +0530

make check-world fails,i think we don't need $port and $filename instead we
can use something like 'xxx'.so fixed it in the below patch.

Regards,

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 28 Jan 2025 11:57:39 +0530

On Tue, 28 Jan 2025 at 10:19, Srinath Reddy <[email-masked]> wrote:
> But in common_dumpall_restore.c there's  parseDumpFormat which is common between pg_dumpall and pg_restore ,as per the discussion in [1] thread i don't think we should create a common api ,as discussed in the thread there might chances in the future we might decide that some format is obsolete and desupport it in pg_dumpall ,while support in pg_restore for compatibility reasons.
>

Oaky. Thanks for review. I will make changes as per discussion in
another thread.

On Tue, 28 Jan 2025 at 11:52, Srinath Reddy <[email-masked]> wrote:
>
> make check-world fails,i think we don't need $port and $filename instead we can use something like 'xxx'.so fixed it in the below patch.

In offline discussion, Andew already reported this test case. I will
fix this in the next version.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 28 Jan 2025 17:32:51 +0530

> $BIN10/pg_restore --exclude-database=?* -Cd template1 --verbose dir10

I merged v12_0001 into the latest patch. There was one bug in v12_001*
which was fixed in v12_0002*.

-extern PGconn **connectDatabase*(const char *dbname,
> - const char *connection_string, const char *pghost,
> - const char *pgport, const char *pguser,
> - trivalue prompt_password, bool fail_on_error,
> - const char *progname, const char **connstr, int *server_version);
> +extern PGconn *v(const char *dbname, const char *connection_string, const
> char *pghost,

As per v12_0002*, I made some changes into the current patch to avoid using
the substring function.

On Tue, 28 Jan 2025 at 11:57, Mahendra Singh Thalor <[email-masked]>
wrote:
> > But in common_dumpall_restore.c there's  parseDumpFormat which is
common between pg_dumpall and pg_restore ,as per the discussion in [1]
thread i don't think we should create a common api ,as discussed in the
thread there might chances in the future we might decide that some format
is obsolete and desupport it in pg_dumpall ,while support in pg_restore for
compatibility reasons.

Fixed. In the latest patch, I removed the parseDumpFormat function.
In older versions, I was using the same function for pg_dumpall and
pg_restore but now some common code is already committed from this patch
and as per discussion, we will keep separate handling for parsing so adding
parseDumpFormat function only in pg_dumpall.c file.

> v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch.

Okay. I am doing some more testing and code review for this type of test
cases. I will merge this delta into the next version.

> > make check-world fails,i think we don't need $port and $filename
instead we can use something like 'xxx'.so fixed it in the below patch.
Fixed.

Thanks Jian and Srinath for the testing and review.

I merged some of the delta patches that are shared by Jian and did some
fixes also.

---

From: jian he <[email-masked]>
Date: Wed, 29 Jan 2025 17:38:36 +0800

hi.

we need to escape the semicolon within the single quotes or double quotes.
I think my patch in [1] is correct.

we can have "ERROR:  role "z" already exists
but
error message like
pg_restore: error: could not execute query: "ERROR:  unterminated
quoted string at or near "';
should not be accepted in execute_global_sql_commands, ReadOneStatement, PQexec

attached is the all the corner test case i come up with against
ReadOneStatement.
your v13 will generate errors like "ERROR:  unterminated quoted string
at or near ..."',
which is not good, i think.

[1] https://www.postgresql.org/message-id/CACJufxEQUcjBocKJQ0Amf3AfiS9wFB7zYSHrj1qqD_oWeaJoGQ%40mail.gmail.com

---

From: jian he <[email-masked]>
Date: Fri, 31 Jan 2025 11:52:42 +0800

hi.

-extern void RestoreArchive(Archive *AHX);
+extern void RestoreArchive(Archive *AHX, bool append_data);
Can we spare some words to explain the purpose of append_data.

in get_dbname_oid_list_from_mfile
        pg_log_info("map.dat file is not present in dump of
pg_dumpall, so nothing to restore.");
maybe we can change it to
        pg_log_info("databases restoring is skipped as map.dat file is
not present in \"%s\"", dumpdirpath);
we can aslo add Assert(dumpdirpath != NULL)

pg_log_info("found dbname as : \"%s\" and db_oid:%u in map.dat file
while restoring", dbname, db_oid);
also need to change. maybe
pg_log_info("found database \"%s\" (OID: %u) in map.dat file while
restoring.", dbname, db_oid);

I also did some minor refactoring, please check attached.

doc/src/sgml/ref/pg_restore.sgml
 <refnamediv>
  <refname>pg_restore</refname>

  <refpurpose>
   restore a <productname>PostgreSQL</productname> database from an
   archive file created by <application>pg_dump</application>
  </refpurpose>
 </refnamediv>
need to change, since now we can restore multiple databases.

doc/src/sgml/ref/pg_dumpall.sgml
 <refnamediv>
  <refname>pg_dumpall</refname>
  <refpurpose>extract a <productname>PostgreSQL</productname> database
cluster into a script file</refpurpose>
 </refnamediv>
also need change.

---

From: jian he <[email-masked]>
Date: Fri, 31 Jan 2025 16:51:26 +0800

hi.
more small issues.

+ count_db++; /* Increment db couter. */
+ dboidprecell = dboid_cell;
+ }
+
typo, "couter" should be "counter".

+
+/*
+ * get_dbname_oid_list_from_mfile
+ *
+ * Open map.dat file and read line by line and then prepare a list of database
+ * names and correspoding db_oid.
+ *
typo, "correspoding" should be "corresponding".

execute_global_sql_commands comments didn't mention ``IF (outfile) ``
branch related code.
We can add some comments saying that
""IF opts->filename is not specified, then copy the content of
global.dat to opts->filename""".

or split it into two functions.

+ while((fgets(line, MAXPGPATH, pfile)) != NULL)
+ {
+ Oid         db_oid;
+ char db_oid_str[MAXPGPATH + 1];
+ char        dbname[MAXPGPATH + 1];
+
+ /* Extract dboid. */
+ sscanf(line, "%u" , &db_oid);
+ sscanf(line, "%s" , db_oid_str);
+
+ /* Now copy dbname. */
+ strcpy(dbname, line + strlen(db_oid_str) + 1);
+
+ /* Remove \n from dbanme. */
+ dbname[strlen(dbname) - 1] = '\0';
+
+ pg_log_info("found dbname as : \"%s\" and db_oid:%u in map.dat file
while restoring", dbname, db_oid);
+
+ /* Report error if file has any corrupted data. */
+ if (!OidIsValid(db_oid) || strlen(dbname) == 0)
+ pg_fatal("invalid entry in map.dat file at line : %d", count + 1);
+
+ /*
+ * XXX : before adding dbname into list, we can verify that this db
+ * needs to skipped for restore or not but as of now, we are making
+ * a list of all the databases.
+ */
+ simple_db_oid_list_append(dbname_oid_list, db_oid, dbname);
+ count++;
+ }

db_oid first should be set to 0, dbname first character first should be set to 0
(char        dbname[0] = '\0') before sscanf call.
so if sscanf fail, the db_oid and dbname value is not undermined)

---

From: Srinath Reddy <[email-masked]>
Date: Sat, 1 Feb 2025 21:36:29 +0530

Hi,
i think we have to change the pg_dumpall "--help" message similar to
pg_dump's specifying that now pg_dumpall dumps cluster into to other
non-text formats.
Need similar "--help" message change in pg_restore to specify that now
pg_restore supports restoring whole cluster from archive created from
pg_dumpall.

diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 3e022ecdeb..728abe841c 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -667,7 +667,7 @@ main(int argc, char *argv[])
 static void
 help(void)
 {
- printf(_("%s extracts a PostgreSQL database cluster into an SQL script
file.\n\n"), progname);
+ printf(_("%s extracts a PostgreSQL database cluster into an SQL script
file or to other formats.\n\n"), progname);

diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index fc248a441e..c4e58c1f3b 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -582,6 +582,8 @@ static void
 usage(const char *progname)
 {
  printf(_("%s restores a PostgreSQL database from an archive created by
pg_dump.\n\n"), progname);
+ printf(_("[or]\n"));
+ printf(_("%s restores a PostgreSQL entire cluster from an archive created
by pg_dumpall.\n\n"), progname); Regards, Srinath Reddy Sadipiralla, EDB:
https://www.enterprisedb.com <http://www.enterprisedb.com/>

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Mon, 3 Feb 2025 01:49:00 +0530

Thanks Jian for review, testing and delta patches.

> which is not good, i think.
https://www.postgresql.org/message-id/CACJufxEQUcjBocKJQ0Amf3AfiS9wFB7zYSHrj1qqD_oWeaJoGQ%40mail.gmail.com

Yes, you are right. We can't read line by line. We should read char by char
and we need some extra handling for double quote names.
I have merged your delta patch into this and now I am doing some more
testing for corner cases of this type of names.
*Ex*: add some comments in names etc or multiple semicolons or other
special characters in name.

> Can we spare some words to explain the purpose of append_data.

Fixed. I added some comments on the top of the RestoreArchive function.

Fixed.

> we can aslo add Assert(dumpdirpath != NULL)

No, we don't need it as we are already checking inputfileSpec!= NULL.

Fixed.

> I also did some minor refactoring, please check attached.

Thanks. I merged it.

> need to change, since now we can restore multiple databases.

Agreed. I added some comments.

As Jian suggested, we need to change docs so I did the same changes into
doc and --help also.

Fixed.

Fixed.

> global.dat to opts->filename""".

We already have some comments on the top of the execute_global_sql_commands
function.

> or split it into two functions.

Done. I added a new function for outfile.

> so if sscanf fail, the db_oid and dbname value is not undermined)

Okay. Fixed.

---

From: Srinath Reddy <[email-masked]>
Date: Mon, 3 Feb 2025 14:23:38 +0530

Hi,
I found a bug ,while using "./pg_restore pdd -f -" actually it has to copy
everything(global sql commands + remaining dump ) into stdout as per the
"-f, --file=FILENAME      output file name (- for stdout)" but it is
copying global sql commands to a file literally naming it as "-" and
remaining dump is written to stdout without those global sql commands."-"
is not a output file it signifies stdout in terminal cmds.so we have to
handle this case.
because of above reason "./pg_restore pdd -g -f -"  also  does the same
creates a file "-" and writes globals to that file instead of stdout.

This is the delta patch to handle this case.please have a look and give
some feedback.

@@ -84,7 +84,7 @@ static int restoreAllDatabases(PGconn *conn, const char
*dumpdirpath,
                                                           SimpleStringList
db_exclude_patterns, RestoreOptions *opts, int numWorkers);
 static void execute_global_sql_commands(PGconn *conn, const char
*dumpdirpath,

    const char *outfile);
-static void copy_global_file_to_out_file(const char *outfile, FILE *pfile);
+static void copy_global_file(const char *outfile, FILE *pfile);
 static int filter_dbnames_for_restore(PGconn *conn,

SimpleDatabaseOidList *dbname_oid_list,
-       ofile = fopen(out_file_path, PG_BINARY_W);
+       if (strcmp(outfile, "-") == 0){
+               int     fn = fileno(stdout);
+               ofile = fdopen(dup(fn), PG_BINARY_W);
+       }
+       else{
+               snprintf(out_file_path, MAXPGPATH, "%s", outfile);
+               ofile = fopen(out_file_path, PG_BINARY_W);
+       }
+

        if (ofile == NULL)
        {

Regards,
Srinath Reddy Sadipiralla,
EDB: https://www.enterprisedb.com <http://www.enterprisedb.com/>

---

From: Srinath Reddy <[email-masked]>
Date: Mon, 3 Feb 2025 14:32:11 +0530

here's the whole version of delta patch

diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 42c4fe3ce2..90e6b71a50 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -84,7 +84,7 @@ static int restoreAllDatabases(PGconn *conn, const char
*dumpdirpath,
    SimpleStringList db_exclude_patterns, RestoreOptions *opts, int
numWorkers);
 static void execute_global_sql_commands(PGconn *conn, const char
*dumpdirpath,
  const char *outfile);
-static void copy_global_file_to_out_file(const char *outfile, FILE *pfile);
+static void copy_global_file(const char *outfile, FILE *pfile);
 static int filter_dbnames_for_restore(PGconn *conn,
   SimpleDatabaseOidList *dbname_oid_list,
   SimpleStringList db_exclude_patterns);
@@ -1178,7 +1178,7 @@ execute_global_sql_commands(PGconn *conn, const char
*dumpdirpath, const char *o
  */
  if (outfile)
  {
- copy_global_file_to_out_file(outfile, pfile);
+ copy_global_file(outfile, pfile);
  return;
  }

@@ -1207,24 +1207,35 @@ execute_global_sql_commands(PGconn *conn, const
char *dumpdirpath, const char *o
 }

 /*
- * copy_global_file_to_out_file
+ * copy_global_file
  *
- * This will copy global.dat file into out file.
+ * This will copy global.dat file into out file, if file is given
+ * else copies to stdout.
+ *
  */
 static void
-copy_global_file_to_out_file(const char *outfile, FILE *pfile)
+copy_global_file(const char *outfile, FILE *pfile)
 {
  char out_file_path[MAXPGPATH];
  FILE *ofile;
  int c;

- snprintf(out_file_path, MAXPGPATH, "%s", outfile);
- ofile = fopen(out_file_path, PG_BINARY_W);
+ if (strcmp(outfile, "-") == 0)
+ {
+ int fn = fileno(stdout);
+ ofile = fdopen(dup(fn), PG_BINARY_W);
+ }
+ else
+ {
+ snprintf(out_file_path, MAXPGPATH, "%s", outfile);
+ ofile = fopen(out_file_path, PG_BINARY_W);
+ }
+

  if (ofile == NULL)
  {
  fclose(pfile);
- pg_fatal("could not open file: \"%s\"", out_file_path);
+ pg_fatal("could not open file: \"%s\"", outfile);
  }

  /* Now append global.dat into out file. */

---

From: jian he <[email-masked]>
Date: Mon, 3 Feb 2025 17:14:06 +0800

hi.

git clean -fdx && $BIN10/pg_dumpall --format=directory --file=dir10
$BIN10/pg_restore --format=directory --file=1.sql --verbose dir10 >
dir_format 2>&1

there is no "\connect dbname" command.
pipe 1.sql to psql will execute all the database dump into a single
database, which is not good.
we need "\connect dbname" in file 1.sql

--------<<<<<<<>>>>>>>>>>>>>>>------------------
$BIN10/pg_dumpall --format=directory --exclude-database=src10 --file=dir12_temp
drop table t from database x
$BIN10/pg_restore --format=directory --dbname=x --verbose dir12_temp >
dir_format 2>&1
--------log info------------------
pg_restore: found database "template1" (OID: 1) in map.dat file while restoring.
pg_restore: found database "x" (OID: 19554) in map.dat file while restoring.
pg_restore: found total 2 database names in map.dat file
pg_restore: needs to restore 2 databases out of 2 databases
pg_restore: restoring dump of pg_dumpall without -C option, there
might be multiple databases in directory.
pg_restore: restoring database "template1"
pg_restore: connecting to database for restore
pg_restore: implied data-only restore
pg_restore: restoring database "x"
pg_restore: connecting to database for restore
pg_restore: processing data for table "public.t"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3374; 0 19555 TABLE DATA t jian
pg_restore: error: could not execute query: ERROR:  relation
"public.t" does not exist
Command was: COPY public.t (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 1
pg_restore: number of restored databases are 2
________________________
$BIN10/pg_restore --format=directory --list dir12_temp
selected output:

; Selected TOC Entries:
;
217; 1259 19555 TABLE public t jian
3374; 0 19555 TABLE DATA public t jian
3228; 2606 19560 CONSTRAINT public t t_pkey jian

As you can see, dir12_temp has TABLE and TABLE DATA.
so the above log message: "pg_restore: implied data-only restore" is
not what we expected.

BTW, add --create option, it works as i expected.
like
$BIN10/pg_restore --format=directory --create --dbname=x --verbose
dir12_temp > dir_format 2>&1
output is what i expected.

--------<<<<<<<>>>>>>>>>>>>>>>------------------
with the changes in filter_dbnames_for_restore.
so <option>--exclude-database=<replaceable
class="parameter">pattern</replaceable></option>
will behave differently when you specify the --file option or not.

* --file option specified
-exclude-database=pattern not allow any special wildcard character.
it does not behave the same as the doc mentioned.
* --file option not specified, it behaves the same as the doc mentioned.

That's kind of tricky, either more words in the doc explain the
scarenio where --file option is specified
or disallow --file option when --exclude-database is specified.

we need to update pg_restore.sgml about MAX_ON_EXIT_NICELY 100?

there is some corner like  num_db_restore == 0, num_db_restore >= 100
in that scarenio, the execute_global_sql_commands already executed,
which is not ideal, since you have pg_fatal and some sql commands
already executed.
maybe we can be if 0 < num_db_restore < 100 then
call execute_global_sql_commands and restoreAllDatabases.

the attached patch trying to do that.
attached patch also doing some cosmetic changes.

---

From: jian he <[email-masked]>
Date: Mon, 3 Feb 2025 20:49:15 +0800

> attached patch also doing some cosmetic changes.

hi.
please ignore the previous patch. see this email attached patch.
previously I complained that the ``pg_restore --list`` needed a db
connection and also called execute_global_sql_commands in [1]
this email attached patch fixes the problem, now pg_restore --list no
need db connection.

now the logic is:
if num_db_restore value is ok (0 < num_db_restore < MAX_ON_EXIT_NICELY)
*AND* we didn't specify --list option
then call execute_global_sql_commands.

[1] https://postgr.es/m/CACJufxHUDGWe=2ZukvMfuwEcSK8CsVYm=[email-masked]

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Mon, 3 Feb 2025 23:34:39 +0530

On Mon, 3 Feb 2025 at 14:23, Srinath Reddy <[email-masked]> wrote:
>
> Hi,
> I found a bug ,while using "./pg_restore pdd -f -" actually it has to copy everything(global sql commands + remaining dump ) into stdout as per the "-f, --file=FILENAME      output file name (- for stdout)" but it is copying global sql commands to a file literally naming it as "-" and remaining dump is written to stdout without those global sql commands."-" is not a output file it signifies stdout in terminal cmds.so we have to handle this case.
> because of above reason "./pg_restore pdd -g -f -"  also  does the same creates a file "-" and writes globals to that file instead of stdout.

I also tested this but in my testing, I can see that all globals are
printed into the console also. Patch was creating a "-" file that was
wrong.
Yes, we should consider "-" as a stdout. In the latest patch, I have
fixed this issue.

> we need "\connect dbname" in file 1.sql

We can't add this command directly to the dump file. We need to add
some TOC entry for this command. I will try to make a TOC entry for
this command.

I will do some tests with pg_dump and -t option.

> output is what i expected.

We will do some more doc changes for this in next versions.

> we need to update pg_restore.sgml about MAX_ON_EXIT_NICELY 100?

Temporary, we increased this size. Based on other opinions, we will do
more changes for this.

> call execute_global_sql_commands and restoreAllDatabases.

Got it. Fixed it as per delta patch and added some extra condition to
the IF clause.

---

From: jian he <[email-masked]>
Date: Tue, 4 Feb 2025 10:04:36 +0800

hi.

just a quick response for v15.

the pg_restore man page says option --list as "List the table of
contents of the archive".
but
$BIN10/pg_restore --format=directory --list --file=1.sql dir10
also output the contents of "global.dat", we should not output it.

in restoreAllDatabases, we can do the following change:
```
    /* Open global.dat file and execute/append all the global sql commands. */
    if (!opts->tocSummary)
        process_global_sql_commands(conn, dumpdirpath, opts->filename);
```

what should happen with
$BIN10/pg_restore --format=directory --globals-only --verbose dir10 --list

Should we error out saying "--globals-only" and "--list" are conflict options?
if so then in main function we can do the following change:

```
if (globals_only)
{
    process_global_sql_commands(conn, inputFileSpec, opts->filename);
    if (conn)
        PQfinish(conn);
    pg_log_info("databases restoring is skipped as -g/--globals-only
option is specified");
}
```

in restoreAllDatabases, if num_db_restore == 0, we will still call
process_global_sql_commands.
I am not sure this is what we expected.

---

From: jian he <[email-masked]>
Date: Tue, 4 Feb 2025 15:42:28 +0800

hi.
This attached patch solves problems mentioned in [1].
so pg_restore --file restoring multiple databases will have the
```\connect dbname``` command in it.
the output plain text file can be used in psql.

pg_restore --file output will be:

--
-- Database "template1" dump
--

-- Dumped from database version 18devel_debug_build_622f678c10
-- Dumped by pg_dump version 18devel_debug_build_622f678c10

-- Started on 2025-02-04 14:34:44 CST

\connect template1

.....

-- Completed on 2025-02-04 14:34:53 CST

--
-- Database "template1" dump complete
--

[1] https://postgr.es/m/CACJufxFrzYJ0oZNm=[email-masked]

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 11 Feb 2025 11:10:17 +0530

Thanks Jian.

> also output the contents of "global.dat", we should not output it.

I think we can add an error for --list option if used with the dump of
pg_dumpall. If a user wants to use --list option, then they can use a
single dump file.

Fixed.

> I am not sure this is what we expected.

This is correct. We should run global commands as we are dumping those
even if we don't dump any database.

Apart from these, I merged v15 delta to print db names. Either we can
print the db name or we can remove also but as of now, I merged delta
patch.

---

From: jian he <[email-masked]>
Date: Tue, 11 Feb 2025 23:09:40 +0800

hi.
review based on v16.

because of
https://postgr.es/m/[email-masked]

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.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 11 Feb 2025 22:47:29 +0530

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

> here we should use pg_fatal?

Yes, we should use pg_fatal.

> message would be more similar to the next pg_log_error(...) message.

Okay.

> + pg_fatal("option -l/--list cannot be used when restoring multiple
databases");

okay.

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

> + pg_log_info("restoring multiple databases without -C option.");

okay.

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

---

From: jian he <[email-masked]>
Date: Wed, 12 Feb 2025 15:14:34 +0800

> > There are some tests per https://commitfest.postgresql.org/52/5495, I

hi.
the cfbot failure is related to function _tocEntryRequired

  if (strcmp(te->desc, "DATABASE") == 0 ||
  strcmp(te->desc, "DATABASE PROPERTIES") == 0)
  {
- if (ropt->createDB)
+ if (ropt->createDB || AH->format != archNull)
  return REQ_SCHEMA;
  else
  return 0;

for restoring multiple databases:
in v16 implementation: pg_restore even if you do not specify --create,
it actually did what pg_restore --create option does.

if there are multiple databases in the archive:
to make the pg_restore --file output is usable, the output file need
have \connect and CREATE DATABASE
command. that is exactly what  --create option would do.
pg_restore --file behavior need align with pg_restore --dbname.
therefore pg_restore restoring multiple databases will use --create option.

we can either error out (pg_fatal) saying
restoring multiple databases requires the pg_restore --create option.
Or we can add a pg_log_info saying
pg_restore --create option will be set to true while restoring
multiple databases.

for restoring one database, the master behavior is fine.
so we don't need to change _tocEntryRequired.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 13 Feb 2025 16:55:56 +0530

Thanks Jian.

> pg_restore --create option will be set to true while restoring

In my earlier version, I was giving an error if --create option was
not specified.

I think it will be good and more preferable if we give an error
without the --create option if dump was taken from pg_dumpall. Even
though there is a single database in the dump of pg_dumpall, it is
possible that a particular database hasn't been created.
Ex: -d postgres and we have db1 dump in file. In this case, we have
only one database dump but this database has not been created.
If the user wants to restore a single database, then the user should
use a single database dump file. Forcefully adding --create option is
not a good idea, instead we will give an error to the user and let him
correct the inputs.

Apart from the above handling, I fixed all the pending review comments
in this patch and made some more changes.

---

From: Srinath Reddy <[email-masked]>
Date: Tue, 18 Feb 2025 10:00:11 +0530

Hi,
i think during restore we should not force user to use -C during cases like
./pg_restore pdd -g -f -
./pg_restore pdd -a -f -
./pg_restore pdd -s -f -
because its not good to use -C to create database every time when we are
using these options individually.
latest patch throws following error for all the above cases

pg_restore: error: -C/--create option should be specified when restoring
multiple databases by archive of pg_dumpall
pg_restore: hint: Try "pg_restore --help" for more information.
pg_restore: hint: If db is already created and dump has single db dump,
then use particular dump file.

Thanks and Regards
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com <http://www.enterprisedb.com/>
Srinath Reddy Sadipiralla,

---

From: jian he <[email-masked]>
Date: Tue, 18 Feb 2025 14:10:52 +0800

hi.

 <refnamediv>
  <refname>pg_restore</refname>
  <refpurpose>
   restore a <productname>PostgreSQL</productname> database from an
   archive file created by <application>pg_dump</application>
   or restore multiple <productname>PostgreSQL</productname> database from an
   archive directory created by <application>pg_dumpall</application>
  </refpurpose>
 </refnamediv>

i think it's way too verbose. we can change it to:
<refpurpose>
   restore <productname>PostgreSQL</productname> database from an
   archive file created by <application>pg_dump</application> or
<application>pg_dumpall</application>
  </refpurpose>

  <para>
   <application>pg_restore</application> is a utility for restoring a
   <productname>PostgreSQL</productname> database from an archive
   created by <xref linkend="app-pgdump"/> in one of the non-plain-text
   formats.
we can change it to
  <para>
   <application>pg_restore</application> is a utility for restoring
   <productname>PostgreSQL</productname> databases from an archive
   created by <xref linkend="app-pgdump"/> or <xref
linkend="app-pgdumpall"/> in one of the non-plain-text
   formats.

similarly, pg_dumpall first 3 sentences in the description section
needs to change.

in pg_restore.sgml <option>--create</option section,
maybe we can explicitly mention that restoring multiple databases,
<option>--create</option> is required.
like: "This option is required when restoring multiple databases."

restoreAllDatabases
+ if (!conn)
+ pg_log_info("there is no database connection so consider pattern as
simple name for --exclude-database");
filter_dbnames_for_restore
+ if (!conn)
+ pg_log_info("considering PATTERN as NAME for --exclude-database
option as no db connection while doing pg_restore.");

these two log messages sent out the same information.
maybe we can remove the first one, and change the second to
    if (!conn && db_exclude_patterns.head != NULL)
        pg_log_info("considering PATTERN as NAME for
--exclude-database option as no db connection while doing
pg_restore.");

as mentioned in the previous thread, there is no need to change PrintTOCSummary.

another minor issue about comments.
I guess we can tolerate this minor issue.
$BIN10/pg_restore --format=tar --create --file=1.sql
--exclude-database=src10 --verbose tar10 > dir_format 2>&1
1.sql file will copy tar10/global.dat as is. but we already excluded
src10. but 1.sql will still have comments as
--
-- Database "src10" dump
--

$BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
$BIN10/pg_dumpall --format=custom --file=x2.dump

Currently x1.dump/global.dat is differ from x2.dump/global.dat
if we dump multiple databases using pg_dumpall we have
"
--
-- Databases
--
--
-- Database "template1" dump
--
--
-- Database "src10" dump
--
--
-- Database "x" dump
--
"
maybe there are not need, since we already have map.dat file

I am not sure if the following is as expected or not.
$BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
$BIN10/pg_restore --create --file=3.sql --globals-only x1.dump --verbose
$BIN10/pg_restore --create --file=3.sql x1.dump --verbose

the first pg_restore command  will copy x1.dump/global.dat as is to 3.sql,
the second pg_restore will not copy anything to 3.sql.
but the second command implies copying global dumps to 3.sql?

---

From: jian he <[email-masked]>
Date: Tue, 18 Feb 2025 16:31:40 +0800

> hi.

hi. more cosmetic minor issues.

+static int
+get_dbname_oid_list_from_mfile(const char *dumpdirpath,
SimpleDatabaseOidList *dbname_oid_list)
...
+ /*
+ * XXX : before adding dbname into list, we can verify that this db
+ * needs to skipped for restore or not but as of now, we are making
+ * a list of all the databases.
+ */
i think the above comment in get_dbname_oid_list_from_mfile is not necessary.
we already have comments in filter_dbnames_for_restore.

in get_dbname_oid_list_from_mfile:
```
    pfile = fopen(map_file_path, PG_BINARY_R);
    if (pfile == NULL)
        pg_fatal("could not open map.dat file: \"%s\"", map_file_path);
```
file does not exist, we use pg_fatal, so if the directory does not
exist, we should also use pg_fatal.
so
    if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
    {
        pg_log_info("databases restoring is skipped as map.dat file is
not present in \"%s\"", dumpdirpath);
        return 0;
    }
can be
    if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
        pg_fatal("map.dat file: \"%s\"/map.dat does not exists", dumpdirpath);

+ /* Report error if file has any corrupted data. */
+ if (!OidIsValid(db_oid) || strlen(dbname) == 0)
+ pg_fatal("invalid entry in map.dat file at line : %d", count + 1);
i think the comments should be
+ /* Report error and exit if the file has any corrupted data. */

+/*
+ * filter_dbnames_for_restore
+ *
+ * This will remove names from all dblist those can
+ * be constructed from database_exclude_pattern list.
+ *
+ * returns number of dbnames those will be restored.
+ */
+static int
+filter_dbnames_for_restore(PGconn *conn,
+   SimpleDatabaseOidList *dbname_oid_list,
there is no "database_exclude_pattern" list, so the above comments are
slightly wrong.

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

  /* sync the resulting file, errors are not fatal */
- if (dosync)
+ if (dosync && (archDumpFormat == archNull))
  (void) fsync_fname(filename, false);
does this mean pg_dumpall --no-sync option only works for plain format.
if so, we need to update the pg_dumpall --no-sync section.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Wed, 19 Feb 2025 17:02:17 +0530

On Tue, 18 Feb 2025 at 10:00, Srinath Reddy <[email-masked]> wrote:
> because its not good to use -C to create database every time when we are using these options individually.

-g => we can allow this case without the -C option.
-a and -s => user should use this option with a single database (i
mean user should use a particular dump file to restore, not full dump
directory of all the databases.)

As pg_dumpall dumps all the databases in create mode, we should either
use --create option in our code or we should give an error. I think,
error is a good option if the user is using a dump of pg_dumpall.
If the user wants to use all the options, then the user should use a
single database dump path.
If we allow users without the --create option, then pg_restore will
create all the tables under a single database even if those tables are
in different databases.

I will fix the -g option(1st test case) in the next patch.

---

From: jian he <[email-masked]>
Date: Wed, 19 Feb 2025 19:37:54 +0800

hi.

Currently, pg_retore says
--exit-on-error
Exit if an error is encountered while sending SQL commands to the
database. The default is to continue and to display a count of errors
at the end of the restoration.
Do we need to apply this to restore executing global commands (create
role, create tablespace)?
If not then we need to put some words in pg_restoe --exit-on-error
option saying that while restoring global objects --exit-on-error
option is ignored.

IMHO, in pg_restore.sgml, we need words explicitly saying that
when restoring multiple databases, all the specified options will
apply to each individual database.

I tested the following options for restoring multiple databases. The
results look good to me.
--index=index
--table=table
--schema-only
--transaction-size
--no-comments
some part of (--filter=filename)
--exclude-schema=schema

attach is a minor cosmetic change.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Wed, 19 Feb 2025 18:07:25 +0530

> latest patch throws following error for all the above cases

Fixed. (./pg_restore pdd -g -f -)

Thanks Jian and Srinath for the review and testing.

Fixed.

Fixed.

> similarly, pg_dumpall first 3 sentences in the description section

I think we can keep them for pg_dumpall.

Fixed.

Fixed.

> as mentioned in the previous thread, there is no need to change PrintTOCSummary.

Yes, I removed it.

Fixed.

Okay. Fixed.

> but the second command implies copying global dumps to 3.sql?

We should copy global.dat. I fixed this in the v18 patch.

> we already have comments in filter_dbnames_for_restore.

As of now, I am keeping this comment as this will be helpful while
implementing parallel pg_restore.

>         pg_fatal("map.dat file: \"%s\"/map.dat does not exists", dumpdirpath);

No, we can't add FATAL here as in case  of global-only dump, we will
not have a map.dat file.

Fixed.

Fixed.

Fixed.

> if so, we need to update the pg_dumpall --no-sync section.
As of now, we are using this option with plain format as we dump
server commands in different db file. We can test this more.

> option saying that while restoring global objects --exit-on-error

I think this is the same for all pg_restore commands. Still if we want
to add some docs, we can put.

> apply to each individual database.

We can skip this extra info. I will try in the next version if we can
add something in doc.

> some part of (--filter=filename)

Thank you for detailed testing.

Okay.