Thanks Alvaro for feedback and review. On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvhe...@alvh.no-ip.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 <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.
In the attached patch, I tried to solve the problem of the map.dat file. I will do more analysis based on dbnames in 002_pg_upgrade.pl file. > > 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.) Okay. I will do these changes. > > 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: I will do more study for this case and will update here. > > #define exit_nicely(code) exit(code) Okay. I will fix this. > > 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. Okay. > > -- > Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Here, I am attaching updated patches for review and testing. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
v21_0002_pg_dumpall-with-non-text_format-5th_mar.patch
Description: Binary data
v21_0001_move-common-code-of-pg_dumpall-and-pg_restore-to-new_file.patch
Description: Binary data
