public inbox for [email protected]
help / color / mirror / Atom feedFrom: Álvaro Herrera <[email protected]>
To: Mahendra Singh Thalor <[email protected]>
Cc: jian he <[email protected]>
Cc: Srinath Reddy <[email protected]>
Cc: [email protected]
Subject: Re: Non-text mode for pg_dumpall
Date: Wed, 5 Mar 2025 16:12:14 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAKYtNAof2Rj0a7SZ3zCWQwNJSsciuPxQdJ4GAsX8GqDVKgf+9A@mail.gmail.com>
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 <[email protected]> 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/
view thread (29+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Non-text mode for pg_dumpall
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox