Thanks Alvaro and Jian for the review and feedback. 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.
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
v22_0001_move-common-code-of-pg_dumpall-and-pg_restore-to-new_file.patch
Description: Binary data
v22_0002_pg_dumpall-with-non-text_format-11th_march.patch
Description: Binary data
