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 <alvhe...@alvh.no-ip.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/
