Re: Non-text mode for pg_dumpall

Thanks Alvaro and Jian for the review and feedback.

On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Disclaimer: I didn't review these patches fully.
>
> On 2025-Mar-05, Mahendra Singh Thalor wrote:
>
> > On Wed, 5 Mar 2025 at 01:02, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > > A database name containing a newline breaks things for this patch:
> > >
> > > CREATE DATABASE "foo
> > > bar";
>
> > I also reported this issue on 29-01-2025. This breaks even without this
> > patch also.
>
> Okay, we should probably fix that, but I think the new map.dat file your
> patch adds is going to make the problem worse, because it doesn't look
> like you handled that case in any particular way that would make it not
> fail. I think it would be good to avoid digging us up even deeper in
> that hole. More generally, the pg_upgrade tests contain some code to
> try database names with almost all possible ascii characters (see
> generate_db in pg_upgrade/t/002_pg_upgrade.pl); it would be good to
> ensure that this new functionality also works correctly for that --
> perhaps add an equivalent test to the pg_dumpall test suite.

As Jian also pointed out, we should not allow \n\r in dbnames. I am
keeping dbanames as single line names only.

I am doing testing using the pg_upgrade/t/002_pg_upgrade.pl file to
check different-2 dbnames.

>
> Looking at 0001:
>
> I'm not sure that the whole common_dumpall_restore.c thing is properly
> structured. First, the file name shouldn't presume which programs
> exactly are going to use the funcionality there. Second, it looks like
> there's another PQconnectdbParams() in pg_backup_db.c and I don't
> understand what the reason is for that one to be separate. In my mind,
> there should be a file maybe called connection.c or connectdb.c or
> whatever that's in charge of establishing connection for all the
> src/bin/pg_dump programs, for cleanliness sake. (This is probably also
> the place where to put an on_exit callback that cleans up any leftover
> connections.)

I did some more refactoring and made a connectdb.c file.

> Looking at 0002 I see it mentions looking at the EXIT_NICELY macro for
> documentation. No such macro exists. But also I think the addition
> (and use) of reset_exit_nicely_list() is not a good idea. It seems to
> assume that the only entries in that list are ones that can be cleared
> and reinstated whenever. This makes too much of an assumption about how
> the program works. It may work today, but it'll get in the way of any
> other patch that wants to set up some different on-exit clean up. In
> other words, we shouldn't reset the on_exit list at all. Also, this is
> just a weird addition:

Based on some discussions, I added handling for cleanup. for 1st
database, I am saving index of array and then I am using same index
for rest of the databases as we are closing archive file in
CloseArchive so we can use same index for next database.

>
> #define exit_nicely(code) exit(code)

Fixed.

>
> You added "A" as an option to the getopt_long() call in pg_restore, but
> no handling for it is added.
Fixed.

>
> I think the --globals-only option to pg_restore should be a separate
> commit.
I will make this 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