Subject: Re: Non-text mode for pg_dumpall

From: Mahendra Singh Thalor <[email-masked]>
Date: Sat, 11 Jan 2025 20:08:12 +0530

Thanks Alvaro for quick feedback.

On Sat, 11 Jan 2025 at 2:14 PM, Alvaro Herrera <[email-masked]>
wrote:

> Hmm, this patch adds a function connectDatabase() to pg_restore, but a
> function that's almost identical already exists in pg_dumpall.

Yes, you are right. Both functions are same as I copied this function from
pg_dumpall.c.

> suggest they should be unified.  Maybe create a new file for connection
> management routines? (since this clearly doesn't fit common.c nor
> dumputils.c).

Sure. I will create a new file and I will move these common functions into
that.

Thanks and regards
Mahendra Singh Thalor
https://www.EnterpriseDB.com/

>
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> "In Europe they call me Niklaus Wirth; in the US they call me Nickel's
> worth.
>  That's because in Europe they call me by name, and in the US by value!"
>

---

From: jian he <[email-masked]>
Date: Sat, 11 Jan 2025 23:59:37 +0800

hi.
the following two tests, you can add to src/bin/pg_dump/t/001_basic.pl

command_fails_like(
    [ 'pg_restore', '--globals-only', '-f', 'xxx' ],
    qr/\Qpg_restore: error: option -g\/--globals-only requires option
-d\/--dbname\E/,
    'pg_restore: error: option -g/--globals-only requires option -d/--dbname'
);
command_fails_like(
    [ 'pg_restore', '--globals-only', '--file=xxx', '--exclude-database=x',],
    qr/\Qpg_restore: error: option --exclude-database cannot be used
together with -g\/--globals-only\E/,
    'pg_restore: error: option --exclude-database cannot be used
together with -g/--globals-only'
);

in pg_restore.sgml.
     <varlistentry>
      <term><option>--exclude-database=<replaceable
class="parameter">pattern</replaceable></option></term>
      <listitem>
the position should right after
     <varlistentry>
      <term><option>-d <replaceable
class="parameter">dbname</replaceable></option></term>
      <term><option>--dbname=<replaceable
class="parameter">dbname</replaceable></option></term>

should
pg_restore --globals-only
pg_restore --exclude-database=pattern
be in a separate patch?

i am also wondering what will happen:
pg_restore --exclude-database=pattern --dbname=pattern

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Sat, 11 Jan 2025 21:38:12 +0530

> pg_restore --exclude-database=pattern --dbname=pattern

For restore, we will make server connection with ‘pattern’ database and we
will skip restoring for ‘pattern’ database as we are giving ‘pattern’ with
—exclude-database.
With server connection, we will restore global.dat at the start of
pg_restore.

Thanks and regards
Mahendra Singh Thalor
 EDB postgres

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Sun, 12 Jan 2025 03:00:52 +0530

Thanks Alvaro and Jian for the review.

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

Fixed. Apart from this, added handling to support empty directory also
with --file option.

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

No, we don't need this change. With --fromat=d, we can omit the --file option.

> On Sat, 11 Jan 2025 at 14:14, Alvaro Herrera <[email-masked]> wrote:
>
> function that's almost identical already exists in pg_dumpall.  I

I will make a new file in follow-up patches.

> On Sat, 11 Jan 2025 at 21:38, Mahendra Singh Thalor <[email-masked]> wrote:
>
>

I removed this error form code as we can dump global sql commands in file also.

>> together with -g/--globals-only'

Fixed.

>> class="parameter">dbname</replaceable></option></term>

Fixed.

>> pg_restore --exclude-database=pattern

I think we can keep these 2 options in one patch only as both are for
pg_restore and there are not many code changes.
If we want, we can make separate patches for pg_dumpall and pg_restore options.

>> pg_restore --exclude-database=pattern --dbname=pattern

For restore, we will make server connection with ‘pattern’ database
and we will skip restoring for ‘pattern’ database as we are giving
‘pattern’ with --exclude-database.
With server connection, we will restore global.dat at the start of
pg_restore and for each database, we will fire the db creation command
from 'pattern' db.

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: Wed, 15 Jan 2025 16:58:45 +0800

> No, we don't need this change. With --fromat=d, we can omit the --file option.
I think this is not correct. since the following three will fail.

$BIN6/pg_dumpall --format=custom --exclude-database=*template* --schema-only
$BIN6/pg_dumpall --format=directory --exclude-database=*template* --schema-only
$BIN6/pg_dumpall --format=tar --exclude-database=*template* --schema-only

that means, pg_dumpall, when format is {custom|directory|tar} --file
option cannot be omitted.

you introduced a format p(plain) for pg_restore? since
$BIN6/pg_restore --dbname=src6 --format=p
will not error out.
but doc/src/sgml/ref/pg_restore.sgml didn't mention this format.

+ if (archDumpFormat == archDirectory)
+ appendPQExpBufferStr(&cmd, " -F d ");
+ else if (archDumpFormat == archCustom)
+ appendPQExpBufferStr(&cmd, " -F c ");
+ else if (archDumpFormat == archTar)
+ appendPQExpBufferStr(&cmd, " -F t ");
can we use long format, i think that would improve the readability.
like changing from
appendPQExpBufferStr(&cmd, " -F d ");
to
appendPQExpBufferStr(&cmd, " --format=directory");

------------------------<<>>>------
I have tested {pg_dump && pg_restore --list}. pg_restore --list works
fine with format {directory|custom|tar}
but it seems there may be some problems with {pg_dumpall && pg_restore
--list} where format is not plain.

with your v08 patch, in my local environment.
$BIN6/pg_dumpall --format=custom --exclude-database=*template*
--schema-only --file=dumpall_src6.custom

$BIN6/pg_restore --dbname=src6 --verbose --schema-only --list
$SRC6/dumpall_src6.custom
error:
pg_restore: error: option -C/--create should be specified when using
dump of pg_dumpall

$BIN6/pg_restore --dbname=src6 --create --verbose --schema-only --list
$SRC6/dumpall_src6.custom
following is some of the output:

pg_restore: found dbname as : "`s3or" and db_oid:1 in map.dat file
while restoring
pg_restore: found dbname as : "`s3or" and db_oid:5 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 database "`s3or"
pg_restore: error: could not open input file
"/home/jian/Desktop/pg_src/src6/postgres/dumpall_src6.custom/databases/1":
No such file or directory

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 16 Jan 2025 01:40:27 +0530

Thanks Jian for the review and testing.

> option cannot be omitted.

Thanks. I got your point. I added one note for this case in the attached patch.

> but doc/src/sgml/ref/pg_restore.sgml didn't mention this format.

Yes, I will do more doc changes and will modify some comments in code
as per new options.

> appendPQExpBufferStr(&cmd, " --format=directory");

Fixed. In the whole file, we are using shortcuts for other options
also but as per your comment, I made the changes.

Fixed.

> management routines? (since this clearly doesn't fit common.c nor

Fixed. I made a new file with common_dumpall_restore.c and have moved
all common functions into the new file.

Apart from this, I added handling for some special database names in
the map.dat file. ex: "database name is one"

Thanks and Regards
Mahendra Singh Thalor