Re: Non-text mode for pg_dumpall

Thanks Jian for the review and testing.

On Wed, 15 Jan 2025 at 14:29, jian he <jian.universality@gmail.com> wrote:
>
> On Sun, Jan 12, 2025 at 5:31 AM Mahendra Singh Thalor
> <mahi6run@gmail.com> wrote:
> >
> > >
> > > 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.
> >
> 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.

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

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

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

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

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

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

Fixed.

> On Sat, 11 Jan 2025 at 14:14, Alvaro Herrera <alvherre@alvh.no-ip.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).

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"

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

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