Re: Non-text mode for pg_dumpall

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

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.

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.

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.

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;

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

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

8. Remove extra line:

> +
> static void usage(const char *progname);

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

> + * databases from map.dat , but skip restoring those matching

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

On Mon, Nov 17, 2025 at 10:45 PM Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
wrote:

> Thanks Andrew for the review.
> On Tue, 11 Nov 2025 at 20:41, Andrew Dunstan <andrew(at)dunslane(dot)net> 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 <tushar(dot)ahuja(at)enterprisedb(dot)com>
> wrote:
> >
> >
> >
> > On Tue, Nov 11, 2025 at 11:29 AM Mahendra Singh Thalor <
> mahi6run(at)gmail(dot)com> wrote:
> >>
> >> On Thu, 6 Nov 2025 at 11:03, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
> wrote:
> >> >
> >> > Thanks Vaibhav, Tushar and Andrew for the review and testing.
> >>
> >
> > Thanks Mahendra, getting this error against v07 series patch
> >
> > [edb(at)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
>