On Thu, 27 Nov 2025 at 13:45, Mahendra Singh Thalor <[email protected]> wrote: > > Thanks Vaibhav for the review. > > On Tue, 18 Nov 2025 at 16:05, Vaibhav Dalvi > <[email protected]> wrote: > > > > Hi Mahendra, > > > > Thanks Mahendra for working on this. > > > > Looks like my previous comment below is not addressed: > > 1. > > > >> ### Use of Dump Options Structure (dopt) > >> Please ensure consistency by utilizing the main dump options > >> structure (`dopt`) instead of declaring and using individual variables > >> where the structure already provides fields. For example, the > >> `output_clean` variable seems redundant here: > >> ```c > >> case 'c': > >> output_clean = true; > >> dopt.outputClean = 1; > >> break; > >> ``` > > > > Fixed. output_clean was a global variable because it was used in 2 > functions. Now I am passing dopt. output_clean as function argument > for another function. > > > > > I agree that the output_clean variable is not added by your patch > > but the introduction of dopt by your patch makes it redundant because > > dopt has dopt.outputClean. Please look at below code from pg_dump.c > > for the reference: > > > > case 'c': /* clean (i.e., drop) schema prior to create */ > > dopt.outputClean = 1; > > break; > > case 25: > > dopt.restrict_key = pg_strdup(optarg); > > break; > > > > 2. > > > >> ### 3\. Missing Example in SGML Documentation > >> The SGML documentation for `pg_dumpall` is missing an explicit > >> example demonstrating its use with non-text formats (e.g., directory > >> format). > >> It would be beneficial to include a clear example for this new feature. > > > > > > I think pg_dumpall should have separate examples similar to pg_dump > > rather than referencing the pg_dump example because pg_dumpall > > doesn't have to mention the database name without -l or --database > > in the command. > > > > Fixed. Added some examples. > > > 3. > >> > >> > 1. Is the following change in `src/bin/pg_dump/connectdb.c` intentional? > >> > >> > > >> > ``` > >> > --- a/src/bin/pg_dump/connectdb.c > >> > +++ b/src/bin/pg_dump/connectdb.c > >> Yes, we need this. If there is any error, then we were trying to > >> disconnect the database in 2 places so we were getting a crash. I will > >> try to reproduce crashe without this patch and will respond. > > > > Have you added a test case in the regression suite which fails if we remove > > this particular change and works well with the change? or if possible could > > you please demonstrate here at least. > > Fixed. With AH(archive), we should not free pointers by this exec call > as we free this by exit_nicely hook. (we register AH by > on_exit_close_archive). > > > > > 4. The variable name append_data doesn't look meaningful to me. > > Instead we can use append_database/append_databases? > > because if this variable is set then we dump the databases along with > > global objects. In case of pg_dump, append_data or data_only does make > > sense to differentiate between schema and data but in case of pg_dumpall > > if this variable is set then we're dumping schema as well as data i.e. > > in-short > > the databases. > > > > As of now, I am keeping this append_data as this was from an already > committed patch. > > > ------------------------------------ pg_dumpall.c > > ---------------------------------------- > > > > 5. The variable name formatName doesn't follow the naming convention of > > variables available around it. I think use of format_name/formatname would > > be better. > > > >> char *use_role = NULL; > >> const char *dumpencoding = NULL; > >> + const char *formatName = "p"; > >> trivalue prompt_password = TRI_DEFAULT; > >> bool data_only = false; > >> bool globals_only = false; > > > > Fixed. > > > > > ------------------------------------ pg_restore.c > > ---------------------------------------- > > > > 6. Fourth parameter (i.e. append_data) to function restore_global_objects() > > is redundant. > > All the time value provided by all callers to this parameter is false. > > > > I would suggest removing this parameter and in the definition of this > > function > > call function restore_one_database() with false as 4th argument. Find diff > > below: > > > > Fixed. > > > --- a/src/bin/pg_dump/pg_restore.c > > +++ b/src/bin/pg_dump/pg_restore.c > > @@ -64,8 +64,7 @@ static int restore_one_database(const char > > *inputFileSpec, RestoreOptions *opts, > > int > > numWorkers, bool append_data, int num, > > bool > > globals_only); > > static int restore_global_objects(const char *inputFileSpec, > > - RestoreOptions *opts, int numWorkers, bool append_data, > > - int num, bool globals_only); > > + RestoreOptions *opts, int numWorkers, int num, bool > > globals_only); > > static int restore_all_databases(const char *inputFileSpec, > > SimpleStringList db_exclude_patterns, RestoreOptions *opts, > > int numWorkers); > > static int get_dbnames_list_to_restore(PGconn *conn, > > @@ -554,7 +553,7 @@ main(int argc, char **argv) > > > > /* Set path for toc.glo file. */ > > snprintf(global_path, MAXPGPATH, "%s/toc.glo", > > inputFileSpec); > > - n_errors = restore_global_objects(global_path, > > opts, numWorkers, false, 0, globals_only); > > + n_errors = restore_global_objects(global_path, > > opts, numWorkers, 0, globals_only); > > > > pg_log_info("database restoring skipped because > > option -g/--globals-only was specified"); > > } > > @@ -602,7 +601,7 @@ main(int argc, char **argv) > > * If globals_only is set, then skip DROP DATABASE commands from restore. > > */ > > static int restore_global_objects(const char *inputFileSpec, > > RestoreOptions *opts, > > - int numWorkers, bool append_data, int num, bool > > globals_only) > > + int numWorkers, int num, bool globals_only) > > { > > int nerror; > > int format = opts->format; > > @@ -610,8 +609,8 @@ static int restore_global_objects(const char > > *inputFileSpec, RestoreOptions *opt > > /* Set format as custom so that toc.glo file can be read. */ > > opts->format = archCustom; > > > > - nerror = restore_one_database(inputFileSpec, opts, numWorkers, > > - append_data, num, globals_only); > > + nerror = restore_one_database(inputFileSpec, opts, numWorkers, > > false, num, > > + > > globals_only); > > > > /* Reset format value. */ > > opts->format = format; > > @@ -1097,7 +1096,7 @@ restore_all_databases(const char *inputFileSpec, > > > > /* If map.dat has no entries, return after processing global > > commands. */ > > if (dbname_oid_list.head == NULL) > > - return restore_global_objects(global_path, opts, > > numWorkers, false, 0, false); > > + return restore_global_objects(global_path, opts, > > numWorkers, 0, false); > > > > pg_log_info(ngettext("found %d database name in \"%s\"", > > "found %d database names > > in \"%s\"", > > @@ -1151,7 +1150,7 @@ restore_all_databases(const char *inputFileSpec, > > PQfinish(conn); > > > > /* Open toc.dat file and execute/append all the global sql > > commands. */ > > - n_errors_total = restore_global_objects(global_path, opts, > > numWorkers, false, 0, false); > > + n_errors_total = restore_global_objects(global_path, opts, > > numWorkers, 0, false); > > > > Regression is successful with these changes. > > > > 7. Fix indentation: > >> > >> static int restore_global_objects(const char *inputFileSpec, > >> RestoreOptions *opts, int numWorkers, bool append_data, > >> int num, bool globals_only); > >> static int restore_all_databases(const char *inputFileSpec, > >> SimpleStringList db_exclude_patterns, RestoreOptions *opts, int > >> numWorkers); > > Fixed some. > > > > > > > 8. Remove extra line: > >> > >> + > >> static void usage(const char *progname); > > > > Fixed. > > > > > 9. Remove extra space after map.dat and before comma: > >> > >> + * databases from map.dat , but skip restoring those matching > > > > Fixed. > > > > > 10. Fix 80 char limits: > > > > + n_errors = restore_one_database(subdirpath, opts, numWorkers, true, 1, > > false); > > > > + num_total_db = get_dbname_oid_list_from_mfile(inputFileSpec, > > &dbname_oid_list); > > > > + return restore_global_objects(global_path, opts, numWorkers, false, 0, > > false); > > > > + n_errors_total = restore_global_objects(global_path, opts, numWorkers, > > false, 0, false); > > > > + pg_log_warning("errors ignored on database \"%s\" restore: %d", > > dbidname->str, n_errors); > > > > Fixed some. > I will do some more cleanup in the coming versions. > > Here, I am attaching an updated patch for the review and testing. > > > > > > Regards, > > Vaibhav > > > > On Mon, Nov 17, 2025 at 10:45 PM Mahendra Singh Thalor <[email protected]> > > wrote: > >> > >> Thanks Andrew for the review. > >> On Tue, 11 Nov 2025 at 20:41, Andrew Dunstan <[email protected]> wrote: > >> > > >> > > >> > On 2025-11-11 Tu 12:59 AM, Mahendra Singh Thalor wrote: > >> > > > >> > > Hi, > >> > > Here, I am attaching an updated patch for the review and testing. > >> > > > >> > > FIX: as suggested by Vaibhav, added error for --restrict-key option > >> > > with non-text format. > >> > > > >> > > >> > > >> > Regarding the name and format of the globals toc file, I'm inclined to > >> > think we should always use custom format, regardless of whether the > >> > individual databases will be in custom, tar or directory formats, and > >> > that it should be called something distinguishable, e.g. toc.glo. > >> > > >> > >> I also agree with your point. Fixed. > >> > >> On Mon, 17 Nov 2025 at 19:38, tushar <[email protected]> wrote: > >> > > >> > > >> > > >> > On Tue, Nov 11, 2025 at 11:29 AM Mahendra Singh Thalor > >> > <[email protected]> wrote: > >> >> > >> >> On Thu, 6 Nov 2025 at 11:03, Mahendra Singh Thalor <[email protected]> > >> >> wrote: > >> >> > > >> >> > Thanks Vaibhav, Tushar and Andrew for the review and testing. > >> >> > >> > > >> > Thanks Mahendra, getting this error against v07 series patch > >> > > >> > [edb@1a1c15437e7c bin]$ ./pg_dumpall -Ft -f tar.dumpc -v > >> > pg_dumpall: executing SELECT pg_catalog.set_config('search_path', '', > >> > false); > >> > pg_dumpall: pg_dumpall.c:2256: createOneArchiveEntry: Assertion `fout != > >> > ((void *)0)' failed. > >> > Aborted > >> > > >> > regards, > >> > >> Thanks Tushar for the report. Fixed. > >> > >> Here, I am attaching an updated patch for the review and testing. > >> > >> -- > >> Thanks and Regards > >> Mahendra Singh Thalor > >> EnterpriseDB: http://www.enterprisedb.com > > > -- > Thanks and Regards > Mahendra Singh Thalor > EnterpriseDB: http://www.enterprisedb.com
Hi,
CI was reporting an error for an unused variable.
[08:37:07.338] user 0m14.312s
[08:37:07.338] sys 0m9.155s
[08:37:07.338] make -s -j${BUILD_JOBS} clean
[08:37:07.850] time make -s -j${BUILD_JOBS} world-bin
[08:37:17.443] pg_restore.c:1080:8: error: variable 'count' set but
not used [-Werror,-Wunused-but-set-variable]
[08:37:17.443] 1080 | int count = 0;
[08:37:17.443] | ^
[08:37:17.443] 1 error generated.
[08:37:17.443] make[3]: *** [<builtin>: pg_restore.o] Error 1
[08:37:17.443] make[3]: *** Waiting for unfinished jobs....
[08:37:17.708] make[2]: *** [Makefile:45: all-pg_dump-recurse] Error 2
[08:37:17.709] make[1]: *** [Makefile:42: all-bin-recurse] Error 2
[08:37:17.709] mak
Fixed. Here, I am attaching an updated patch for the review and testing.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
v10_27112025-Non-text-modes-for-pg_dumpall-correspondingly-change.patch
Description: Binary data
