Re: Non-text mode for pg_dumpall

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.

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

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:

#define exit_nicely(code) exit(code)

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

I think the --globals-only option to pg_restore should be a separate
commit.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/