Re: Non-text mode for pg_dumpall - Mailing list pgsql-hackers

Looks like my previous comment below is not addressed:

1.

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.

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.

3.

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.

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.

------------------------------------ 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.

------------------------------------ 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:

--- 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:

8. Remove extra line:

9. Remove extra space after map.dat and before comma:

 

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);

Regards,

Vaibhav