Re: Non-text mode for pg_dumpall

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 <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Hmm, this patch adds a function connectDatabase() to pg_restore, but a
> function that's almost identical already exists in pg_dumpall. I
> 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).

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

> On Sat, 11 Jan 2025 at 21:38, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
>
>
> On Sat, 11 Jan 2025 at 9:30 PM, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>>
>> 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'

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

>> );
>> 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'
>> );

Fixed.

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

Fixed.

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

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.

>>
>>
>> i am also wondering what will happen:
>> 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