Thread: Non-text mode for pg_dumpall

Tom and Nathan opined recently that providing for non-text mode for 
pg_dumpall would be a Good Thing (TM). Not having it has been a 
long-standing complaint, so I've decided to give it a go.

I think we would need to restrict it to directory mode, at least to 
begin with. I would have a toc.dat with a different magic block (say 
"PGGLO" instead of "PGDMP") containing the global entries (roles, 
tablespaces, databases). Then for each database there would be a 
subdirectory (named for its toc entry) with a standard directory mode 
dump for that database. These could be generated in parallel (possibly 
by pg_dumpall calling pg_dump for each database). pg_restore on 
detecting a global type toc.data would restore the globals and then each 
of the databases (again possibly in parallel).

I'm sure there are many wrinkles I haven't thought of, but I don't see 
any insurmountable obstacles, just a significant amount of code.

Barring the unforeseen my main is to have a preliminary patch by the 
September CF.

Following that I would turn my attention to using it in pg_upgrade.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Jun 10, 2024 at 08:58:49AM -0400, Andrew Dunstan wrote:
> Tom and Nathan opined recently that providing for non-text mode for
> pg_dumpall would be a Good Thing (TM). Not having it has been a
> long-standing complaint, so I've decided to give it a go.

Thank you!

> I think we would need to restrict it to directory mode, at least to begin
> with. I would have a toc.dat with a different magic block (say "PGGLO"
> instead of "PGDMP") containing the global entries (roles, tablespaces,
> databases). Then for each database there would be a subdirectory (named for
> its toc entry) with a standard directory mode dump for that database. These
> could be generated in parallel (possibly by pg_dumpall calling pg_dump for
> each database). pg_restore on detecting a global type toc.data would restore
> the globals and then each of the databases (again possibly in parallel).

I'm curious why we couldn't also support the "custom" format.

> Following that I would turn my attention to using it in pg_upgrade.

+1

-- 
nathan

On 2024-06-10 Mo 10:14, Nathan Bossart wrote:
> On Mon, Jun 10, 2024 at 08:58:49AM -0400, Andrew Dunstan wrote:
>> Tom and Nathan opined recently that providing for non-text mode for
>> pg_dumpall would be a Good Thing (TM). Not having it has been a
>> long-standing complaint, so I've decided to give it a go.
> Thank you!
>
>> I think we would need to restrict it to directory mode, at least to begin
>> with. I would have a toc.dat with a different magic block (say "PGGLO"
>> instead of "PGDMP") containing the global entries (roles, tablespaces,
>> databases). Then for each database there would be a subdirectory (named for
>> its toc entry) with a standard directory mode dump for that database. These
>> could be generated in parallel (possibly by pg_dumpall calling pg_dump for
>> each database). pg_restore on detecting a global type toc.data would restore
>> the globals and then each of the databases (again possibly in parallel).
> I'm curious why we couldn't also support the "custom" format.


We could, but the housekeeping would be a bit harder. We'd need to keep 
pointers to the offsets of the per-database TOCs (I don't want to have a 
single per-cluster TOC). And we can't produce it in parallel, so I'd 
rather start with something we can produce in parallel.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Jun 10, 2024 at 10:51:42AM -0400, Andrew Dunstan wrote:
> On 2024-06-10 Mo 10:14, Nathan Bossart wrote:
>> I'm curious why we couldn't also support the "custom" format.
> 
> We could, but the housekeeping would be a bit harder. We'd need to keep
> pointers to the offsets of the per-database TOCs (I don't want to have a
> single per-cluster TOC). And we can't produce it in parallel, so I'd rather
> start with something we can produce in parallel.

Got it.

-- 
nathan

On Mon, Jun 10, 2024 at 04:52:06PM +0200, Magnus Hagander wrote:
> On Mon, Jun 10, 2024 at 4:14 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> I'm curious why we couldn't also support the "custom" format.
> 
> Or maybe even a combo - a directory of custom format files? Plus that one
> special file being globals? I'd say that's what most use cases I've seen
> would prefer.

Is there a particular advantage to that approach as opposed to just using
"directory" mode for everything?  I know pg_upgrade uses "custom" mode for
each of the databases, so a combo approach would be a closer match to the
existing behavior, but that doesn't strike me as an especially strong
reason to keep doing it that way.

-- 
nathan

On Mon, Jun 10, 2024 at 05:45:19PM +0200, Magnus Hagander wrote:
> On Mon, Jun 10, 2024 at 5:03 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> Is there a particular advantage to that approach as opposed to just using
>> "directory" mode for everything?  I know pg_upgrade uses "custom" mode for
>> each of the databases, so a combo approach would be a closer match to the
>> existing behavior, but that doesn't strike me as an especially strong
>> reason to keep doing it that way.
> 
> A gazillion files to deal with? Much easier to work with individual custom
> files if you're moving databases around and things like that.
> Much easier to monitor eg sizes/dates if you're using it for backups.
> 
> It's not things that are make-it-or-break-it or anything, but there are
> some smaller things that definitely can be useful.

Makes sense, thanks for elaborating.

-- 
nathan

Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Jun 10, 2024 at 5:03 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> Is there a particular advantage to that approach as opposed to just using
>> "directory" mode for everything?

> A gazillion files to deal with? Much easier to work with individual custom
> files if you're moving databases around and things like that.
> Much easier to monitor eg sizes/dates if you're using it for backups.

You can always tar up the directory tree after-the-fact if you want
one file.  Sure, that step's not parallelized, but I think we'd need
some non-parallelized copying to create such a file anyway.

            regards, tom lane

On 2024-06-10 Mo 12:21, Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Mon, Jun 10, 2024 at 5:03 PM Nathan Bossart <nathandbossart@gmail.com>
>> wrote:
>>> Is there a particular advantage to that approach as opposed to just using
>>> "directory" mode for everything?
>> A gazillion files to deal with? Much easier to work with individual custom
>> files if you're moving databases around and things like that.
>> Much easier to monitor eg sizes/dates if you're using it for backups.
> You can always tar up the directory tree after-the-fact if you want
> one file.  Sure, that step's not parallelized, but I think we'd need
> some non-parallelized copying to create such a file anyway.
>
>             


Yeah.

I think I can probably allow for Magnus' suggestion fairly easily, but 
if I have to choose I'm going to go for the format that can be produced 
with the maximum parallelism.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Jan 02, 2025 at 02:05:13AM +0530, Mahendra Singh Thalor wrote:
> Here, I am attaching an updated patch. I fixed some bugs of v01 patch and
> did some code cleanup also.

Thank you for picking this up!  I started to review it, but the
documentation changes didn't build, and a few tests in check-world are
failing.  Would you mind resolving those issues?  Also, if you haven't
already, please add an entry to the next commitfest [0] to ensure that 1)
this feature is tracked and 2) the automated tests will run.

+    if (dbfile)
+    {
+        printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin,
+                          dbfile, create_opts);
+        appendPQExpBufferStr(&cmd, " -F d ");
+    }

Have you given any thought to allowing a directory of custom format files,
as discussed upthread [1]?  Perhaps that is better handled as a follow-up
patch, but it'd be good to understand the plan, anyway.

[0] https://commitfest.postgresql.org
[1] https://postgr.es/m/CABUevExoQ26jo%2BaQ9QZq%2BUMA1aD6gfpm9xBnh_t5e0DhaCeRYA%40mail.gmail.com

-- 
nathan

> 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

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.

On Sun, 26 Jan 2025 at 20:17, jian he <jian.universality@gmail.com> wrote:
>
> 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.

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.

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

On Tue, 28 Jan 2025 at 10:19, Srinath Reddy <srinath2133@gmail.com> wrote:
>
>
> 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
thediscussion in [1] thread i don't think we should create a common api ,as discussed in the thread there might chances
inthe future we might decide that some format is obsolete and desupport it in pg_dumpall ,while support in pg_restore
forcompatibility 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 <srinath2133@gmail.com> wrote:
>
> make check-world fails,i think we don't need $port and $filename instead we can use something like 'xxx'.so fixed it
inthe below patch. 

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


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


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

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)

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?

On Tue, Feb 18, 2025 at 2:10 PM jian he <jian.universality@gmail.com> wrote:
>
> 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.

On Tue, 18 Feb 2025 at 10:00, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> 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

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

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

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.

On Tue, 18 Feb 2025 at 10:00, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> 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

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

Thanks Jian and Srinath for the review and testing.

On Tue, 18 Feb 2025 at 11:41, jian he <jian.universality@gmail.com> wrote:
>
> 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>

Fixed.

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

Fixed.

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

I think we can keep them for pg_dumpall.

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

Fixed.

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

Fixed.

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

Yes, I removed it.

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

Fixed.

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

Okay. Fixed.

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

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

On Tue, 18 Feb 2025 at 14:02, jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Feb 18, 2025 at 2:10 PM jian he <jian.universality@gmail.com> wrote:
> >
> > 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.

As of now, I am keeping this comment as this will be helpful while
implementing parallel pg_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);

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

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

Fixed.

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

Fixed.

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

Fixed.

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

On Wed, 19 Feb 2025 at 17:08, jian he <jian.universality@gmail.com> wrote:
>
> 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.

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

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

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

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

Thank you for detailed testing.

>
> attach is a minor cosmetic change.
Okay.

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

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

Hello,

I think the business with an evergrowing on_exit list needs a different
solution than a gigantic array of entries.  Maybe it would make sense to
restructure that code so that there's a single on_exit item, but there
exists a list of per-database entries to clean up which are all done in
one call of the function.  Then you don't need to change the hardcoded
MAX_ON_EXIT_NICELY array size there.

I think it would be better to have a preparatory 0001 patch that just
moves the code to the new files, without touching anything else, and
then the new feature is introduced as a separate 0002 commit.

You still have a bunch of XXX items here and there which look to me like
they need to be handled before this patch can be considered final, plus
the TODOs in the commit message.  Please pgindent.

Thanks

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

Thanks Álvaro for feedback.

On Thu, 20 Feb 2025 at 02:39, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello,
>
> I think the business with an evergrowing on_exit list needs a different
> solution than a gigantic array of entries.  Maybe it would make sense to
> restructure that code so that there's a single on_exit item, but there
> exists a list of per-database entries to clean up which are all done in
> one call of the function.  Then you don't need to change the hardcoded
> MAX_ON_EXIT_NICELY array size there.
>

In the latest patch, I added one new function to clean
index(on_exit_nicely_index) with each database restore.

> I think it would be better to have a preparatory 0001 patch that just
> moves the code to the new files, without touching anything else, and
> then the new feature is introduced as a separate 0002 commit.

Fixed.

>
> You still have a bunch of XXX items here and there which look to me like
> they need to be handled before this patch can be considered final, plus

Fixed.

> the TODOs in the commit message.  Please pgindent.
I am facing some errors in pgindent. I will run pgindent in the next version.

Here, I am attaching updated patches for review and testing.

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

hi.
about 0001

/*
 * connectDatabase
 *
 * Make a database connection with the given parameters.  An
 * interactive password prompt is automatically issued if required.
 *
 * If fail_on_error is false, we return NULL without printing any message
 * on failure, but preserve any prompted password for the next try.
 *
 * On success, the global variable 'connstr' is set to a connection string
 * containing the options used.
 */
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)
do the comments need to change? since no
global variable 'connstr' in common_dumpall_restore.c
maybe we need some words to explain server_version, (i don't have a
huge opinion though).


/*-------------------------------------------------------------------------
 *
 * common_dumpall_restore.c
 *
 * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * This is a common file for pg_dumpall and pg_restore.
 * src/bin/pg_dump/common_dumpall_restore.c
 *
 *-------------------------------------------------------------------------
 */

may change to

/*-------------------------------------------------------------------------
 *
 * common_dumpall_restore.c
 *     This is a common file for pg_dumpall and pg_restore.
 *
 * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * IDENTIFICATION
 *    src/bin/pg_dump/common_dumpall_restore.c
 *
 *-------------------------------------------------------------------------
 */
so the style aligns with most other files.
(we can apply the same logic to src/bin/pg_dump/common_dumpall_restore.h)


in src/bin/pg_dump/pg_dumpall.c
#include "common_dumpall_restore.h"
imply include "pg_backup.h".
so in src/bin/pg_dump/pg_dumpall.c, we don't need include "pg_backup.h"


attached are minor cosmetic changes for v19.

hi.

v20-0001
in src/bin/pg_dump/pg_dumpall.c, we have:

static const char *connstr = "";
            case 'd':
                connstr = pg_strdup(optarg);
                break;

i am not sure you can declare it as "const" for connstr.
since connstr value can be changed.
``#include "pg_backup.h"`` can be removed from src/bin/pg_dump/pg_dumpall.c
Other than that, v20_0001 looks good to me.


v20_0002
const char *formatName = "p";
formatName should not be declared as "const", since its value can be changed.


+ /* Create a subdirectory with 'databases' name under main directory. */
+ if (mkdir(db_subdir, 0755) != 0)
+ pg_fatal("could not create subdirectory \"%s\": %m", db_subdir);
can change to
        if (mkdir(db_subdir, pg_dir_create_mode) != 0)
            pg_fatal("could not create subdirectory \"%s\": %m", db_subdir);
then in src/bin/pg_dump/pg_dumpall.c need add ``#include "common/file_perm.h"``

similarly
+ else if (mkdir(dirname, 0700) < 0)
+ pg_fatal("could not create directory \"%s\": %m", dirname);
can change to
``
else if (mkdir(dirname, pg_dir_create_mode) != 0)
    pg_fatal("could not create directory \"%s\": %m", dirname);
``


+
+ if (!conn)
+ pg_log_info("considering PATTERN as NAME for --exclude-database
option as no db connection while doing pg_restore.");
"db connection" maybe "database connection" or "connection"


+ /*
+ * We need to reset on_exit_nicely_index with each database so that
we can restore
+ * multiple databases by archive.  See EXIT_NICELY macro for more details.
+ */
+ if (dboid_cell != NULL)
+ reset_exit_nicely_list(n_errors ? 1 : 0);
i don't fully understand this part, anyway, i think EXIT_NICELY, you mean
MAX_ON_EXIT_NICELY?


just found out, parseArchiveFormat looks familiar with parseDumpFormat.


for all the options in pg_restore.
--list option is not applicable to multiple databases, therefore
option --use-list=list-file also not applicable,
in the doc we should mention it.


global.dat comments should not mention "cluster", "global objects"
would be more appropriate.
global.dat comments should not mention "--\n-- Database \"%s\" dump\n--\n\n"
the attached minor patch fixes this issue.

hi.
some documentation issue:

doc/src/sgml/ref/pg_dumpall.sgml
       <variablelist>
        <varlistentry>
         <term><literal>d</literal></term>
         <term><literal>directory</literal></term>
         <listitem>
          <para>
           Output a directory-format archive suitable for input into
pg_restore. Under dboid
           subdirectory, this will create a directory with one file
for each table and large
           object being dumped, plus a so-called Table of Contents
file describing the dumped
           objects in a machine-readable format that pg_restore can
read. A directory format
           archive can be manipulated with standard Unix tools; for
example, files in an
           uncompressed archive can be compressed with the gzip, lz4,
or zstd tools. This
           format is compressed by default using gzip and also
supports parallel dumps.
          </para>
         </listitem>
        </varlistentry>
with the v20 implementation,
"""
For example, files in an
           uncompressed archive can be compressed with the gzip, lz4,
or zstd tools. This
           format is compressed by default using gzip and also
supports parallel dumps.
""
Is this part is wrong?
I think, currently, by default the pg_dumpall directory will use gzip
compress level=-1 to do the compression.
and pg_dumpall format==directory does not support parallel dumps.


-------------------
by default, this is plain format. If non-plain mode is passed, then global.dat
(global sql commands) and map.dat(dboid and dbnames list of all the databases)
files will be created. Apart from these files, one subdirectory with databases
name will be created. Under this databases subdirectory, there will be files
with dboid name for each database and if --format is directory, then toc.dat and
other dump files will be under dboid subdirectory.
-------------------
I think the above message changes to the below, the message is more clear, IMHO.

By default, this uses plain format. If a non-plain mode is specified, two files
will be created: **global.dat** (containing SQL commands for global objects) and
**map.dat** (listing database OIDs and names for all databases). Additionally, a
subdirectory named after each database OID will be created.
If the --format option is set to **directory**, then **toc.dat** and
other dump files
will be stored within the corresponding database Oid subdirectory.


---------------------
doc/src/sgml/ref/pg_restore.sgml
<term><option>--exclude-database=<replaceable
class="parameter">pattern</replaceable></option></term>
we can add:

        When emitting a script, this option is not supported for
wild-card matching,
        the excluded database must exactly match the literal
<replaceable class="parameter">pattern</replaceable> string.