Subject: Re: Non-text mode for pg_dumpall

From: Mahendra Singh Thalor <[email-masked]>
Date: Wed, 8 Jan 2025 20:07:17 +0530

Hi all,

On Wed, 8 Jan 2025 at 00:34, Mahendra Singh Thalor <[email-masked]>
wrote:
>
> On Mon, 6 Jan 2025 at 23:05, Nathan Bossart <[email-masked]>
wrote:
> >
> > 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.
>
> Thanks Nathan for the quick response.
>
> I fixed bugs of documentation changes and check-world in the latest
patch. Now docs are building and check-world is passing.
>
> I added entry into commitfest for this patch.[0]
>
> >
> > +       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.
>
> I will make these changes and will test. I will update my findings after
doing some testing.

In the latest patch, I added dump and restoring for
directory/custom/tar/plain formats. Please consider this patch for review
and testing.

*Design*:
When we give --format=d|c|t then we are dumping all global sql commands in
global.dat in plain sql format and we are making a map.dat file with dbname
and dboid. For each database, we are making separate subdirectory with
dboid under databases directory and dumping as per archive format(d|c|t).
While restoring, first we are restoring all global sql commands from
global.dat and then we are restoring one by one all databases.  As we are
supporting --exclude-database with pg_dumpall, the same we are supporting
with pg_restore also to skip restoring on some specified database patterns.
If we want to restore a single database, then we can specided particular
subdirectory from the databases folder. To get file name, we refer
dbname into map.file.

*TODO*: Now I will work on test cases for these new added options to the
pg_dumpall and pg_restore.

Here, I am attaching the v04 patch for testing and review.

>
> Apart from these bugs, I added code to handle --exclude-database=
PATTERN. Earlier I was using NAME only to skip databases for restore.
>
> TODO: .pl test cases for new added options.
>
> Here, I am attaching an updated patch for review and feedback.
>
> >
> > [0] https://commitfest.postgresql.org
> > [1]
https://postgr.es/m/CABUevExoQ26jo%2BaQ9QZq%2BUMA1aD6gfpm9xBnh_t5e0DhaCeRYA%40mail.gmail.com
> >
> > --
> > nathan
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com

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

---

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

On Wed, 8 Jan 2025 at 20:07, Mahendra Singh Thalor <[email-masked]> wrote:
>
> Hi all,
>
> On Wed, 8 Jan 2025 at 00:34, Mahendra Singh Thalor <[email-masked]> wrote:
> >
> > On Mon, 6 Jan 2025 at 23:05, Nathan Bossart <[email-masked]> wrote:
> > >
> > > > Here, I am attaching an updated patch. I fixed some bugs of v01 patch and
> > > already, please add an entry to the next commitfest [0] to ensure that 1)
> > I fixed bugs of documentation changes and check-world in the latest patch. Now docs are building and check-world is passing.
> >
> > > 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
> > I will make these changes and will test. I will update my findings after doing some testing.
>
> In the latest patch, I added dump and restoring for directory/custom/tar/plain formats. Please consider this patch for review and testing.
>
> Design:
> When we give --format=d|c|t then we are dumping all global sql commands in global.dat in plain sql format and we are making a map.dat file with dbname and dboid. For each database, we are making separate subdirectory with dboid under databases directory and dumping as per archive format(d|c|t).
> While restoring, first we are restoring all global sql commands from global.dat and then we are restoring one by one all databases.  As we are supporting --exclude-database with pg_dumpall, the same we are supporting with pg_restore also to skip restoring on some specified database patterns.
> If we want to restore a single database, then we can specided particular subdirectory from the databases folder. To get file name, we refer dbname into map.file.
>
> TODO: Now I will work on test cases for these new added options to the pg_dumpall and pg_restore.
>

Sorry. My mistake.
v04 was the delta patch on the top of v03.

Here, I am attaching the v05 patch for testing and review.

>
> >
> > Apart from these bugs, I added code to handle --exclude-database= PATTERN. Earlier I was using NAME only to skip databases for restore.
> >
> > > [1] https://postgr.es/m/CABUevExoQ26jo%2BaQ9QZq%2BUMA1aD6gfpm9xBnh_t5e0DhaCeRYA%40mail.gmail.com
> > >
> > > --
> > > nathan
> >
> > --
> > Thanks and Regards
> > Mahendra Singh Thalor

---

From: Guillaume Lelarge <[email-masked]>
Date: Wed, 8 Jan 2025 22:00:02 +0100

Hi,

Le mer. 8 janv. 2025 à 17:41, Mahendra Singh Thalor <[email-masked]> a
écrit :

> > > > +                                                 dbfile,
> create_opts);
> > When we give --format=d|c|t then we are dumping all global sql commands
> in global.dat in plain sql format and we are making a map.dat file with
> dbname and dboid. For each database, we are making separate subdirectory
> with dboid under databases directory and dumping as per archive
> format(d|c|t).
> subdirectory from the databases folder. To get file name, we refer dbname
> into map.file.
> >
Just FWIW, I did a quick test tonight. It applies cleanly, compiles OK. I
did a dump:

$ pg_dumpall -Fd -f dir

and then a restore (after dropping the databases I had):

$ pg_restore -Cd postgres -v dir

It worked really well. That's great.

Quick thing to fix: you've got this error message:
pg_restore: error:  -d/--dbanme should be given when using archive dump of
pg_dumpall

I guess it is --dbname, rather than --dbanme.

Of course, it needs much more testing, but this feature would be great to
have. Thanks for working on this!

-- 
Guillaume.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 9 Jan 2025 08:11:59 +0530

On Thu, 9 Jan 2025 at 02:30, Guillaume Lelarge <[email-masked]> wrote:
>
> Hi,
>
> Le mer. 8 janv. 2025 à 17:41, Mahendra Singh Thalor <[email-masked]> a écrit :
>>
> Just FWIW, I did a quick test tonight. It applies cleanly, compiles OK. I did a dump:

Thanks for testing and review.

> pg_restore: error:  -d/--dbanme should be given when using archive dump of pg_dumpall
>

Fixed.

>
> Of course, it needs much more testing, but this feature would be great to have. Thanks for working on this!
>
>

Apart from above typo, I fixed some review comments those I received
from Andrew in offline discussion. Thanks Andrew for the quick review.

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

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 9 Jan 2025 22:56:42 +0530

On Thu, 9 Jan 2025 at 08:11, Mahendra Singh Thalor <[email-masked]>
wrote:
> >> > > I fixed bugs of documentation changes and check-world in the
latest patch. Now docs are building and check-world is passing.
create_opts);
follow-up
> >> >
> >> > Design:
> >> > When we give --format=d|c|t then we are dumping all global sql
commands in global.dat in plain sql format and we are making a map.dat file
with dbname and dboid. For each database, we are making separate
subdirectory with dboid under databases directory and dumping as per
archive format(d|c|t).
> >> > If we want to restore a single database, then we can specided
particular subdirectory from the databases folder. To get file name, we
refer dbname into map.file.
Hi all,

Based on some testing(dump was shared by Andrew, Thanks Andrew), I fixed
some more bugs in the attached patch.
There are some open points for this patch. I will put those points in
follow-up patches also.

*Point 1*: With pg_dumpall, we have option --exclude-database=PATTERN, and
there we are validating this PATTERN by server because
we have connection but in pg_restore, we don't have some db connection in
some case so how to handle these patterns? or should we use
only NAMES for --exclude-database ?

*Point 2*:
For each database, we are registering entry to on_exit_nicely array due AH
entry but max size of array is MAX_ON_EXIT_NICELY=20,
so after 20 db restoring, we are getting fatal so either my code needs to
reset this array or do we need to increase array size?

---

From: jian he <[email-masked]>
Date: Sat, 11 Jan 2025 13:48:27 +0800

in src/bin/pg_dump/pg_dumpall.c main
i think you need do

    archDumpFormat = parseDumpFormat(formatName);
    /*
     * Open the output file if required, otherwise use stdout.  If required,
     * then create new files with global.dat and map.dat names.
     */
    if (archDumpFormat != archNull)
    {
        char    toc_path[MAXPGPATH];
        /*
         * If directory/tar/custom format is specified then we must provide the
         * file name to create one main directory.
         */
        if (!filename || strcmp(filename, "") == 0)
            pg_fatal("no output directory specified");
        /* TODO: accept the empty existing directory. */
        if (mkdir(filename, 0700) < 0)
            pg_fatal("could not create directory \"%s\": %m",
                     filename);
        snprintf(toc_path, MAXPGPATH, "%s/global.dat", filename);
        OPF = fopen(toc_path, "w");
        if (!OPF)
            pg_fatal("could not open global.dat file: %s", strerror(errno));
    }
    else if (filename)
    {
        OPF = fopen(filename, PG_BINARY_W);
        if (!OPF)
            pg_fatal("could not open output file \"%s\": %m",
                     filename);
    }
    else
        OPF = stdout;

before connectDatabase call.

otherwise if the cluster is not setting up.
``pg_dumpall --format=d``
error would be about connection error, not
"pg_dumpall: error: no output directory specified"

we want ``pg_dumpall --format`` invalid options
to error out even if the cluster is not setting up.

attached are two invalid option test cases.

you also need change
     <varlistentry>
      <term><option>-f <replaceable
class="parameter">filename</replaceable></option></term>
      <term><option>--file=<replaceable
class="parameter">filename</replaceable></option></term>
      <listitem>
       <para>
        Send output to the specified file.  If this is omitted, the
        standard output is used.
       </para>
      </listitem>
     </varlistentry>
?

since if --format=d,
<option>--file=<replaceable class="parameter">filename</replaceable></option>
can not be omitted.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Sat, 11 Jan 2025 13:15:49 +0530

On Sat, 11 Jan 2025 at 11:19, jian he <[email-masked]> wrote:

Thanks Jian for the review and testing.

> before connectDatabase call.

Okay. I will add an error check before connectDatabase call in the next version.

> attached are two invalid option test cases.

Thanks.
I am also working on test cases. I will add all error test cases in
the next version and will include these two also.

> <option>--file=<replaceable class="parameter">filename</replaceable></option>

Okay. I will fix it.