Thanks Jian for the review and delta patch. I merged delta patch. On Fri, 2 Jan 2026 at 13:35, jian he <[email protected]> wrote: > > On Tue, Dec 9, 2025 at 2:49 AM Mahendra Singh Thalor <[email protected]> > wrote: > > > > > > Here, I am attaching an updated patch for the review and testing. This > > can be applied on commit d0d0ba6cf66c4043501f6f7. > > > > hi. > > more comments about > v12_09122025-Non-text-modes-for-pg_dumpall-correspondingly-change.patch > > + In all other modes, <application>pg_dumpall</application> > first creates two files: > + <filename>toc.dat/toc.dmp/toc.tar</filename> and > <filename>map.dat</filename>, in the directory > + specified by <option>--file</option>. > + The first file contains global data, such as roles and > tablespaces. The second > + contains a mapping between database oids and names. These > files are used by > + <application>pg_restore</application>. Data for individual > databases is placed in > + <filename>databases</filename> subdirectory, named using the > database's <type>oid</type>. > > I tried all these 3 formats, there is no "toc.dmp/toc.tar". > Am I missing something? >
Fixed. Now we have a glo.dat file in custom format.
> -
> + If format is given, then dump will be based on format, default plain.
> <screen>
> <prompt>$</prompt> <userinput>pg_dumpall > db.out</userinput>
> +</screen>
> +
> +<screen>
> +<prompt>$</prompt> <userinput>pg_dumpall --format=d/a/c/p -f
> db.out</userinput>
> </screen>
>
> The text in the <screen> section should work correctly when pasted directly
> into
> the terminal.
> but ``pg_dumpall --format=d/a/c/p -f db.out``
> will error out:
> ``pg_dumpall: error: unrecognized output format "d/a/c/p"; please
> specify "c", "d", "p", or "t"``
>
Fixed. Added 4 examples.
> PGresult *
> -executeQuery(PGconn *conn, const char *query)
> +executeQuery(PGconn *conn, const char *query, bool is_archive)
> {
> PGresult *res;
>
> @@ -287,7 +287,8 @@ executeQuery(PGconn *conn, const char *query)
> {
> pg_log_error("query failed: %s", PQerrorMessage(conn));
> pg_log_error_detail("Query was: %s", query);
> - PQfinish(conn);
> + if (!is_archive)
> + PQfinish(conn);
> exit_nicely(1);
> }
> It would be nice to add some comments explaining why we don't call
> PQfinish for archive format.
Fixed.
>
> +/*
> + * createOneArchiveEntry
> + *
> + * This creates one archive entry based on format.
> + */
> +static void
> +createOneArchiveEntry(const char *query, const char *tag)
> +{
> + CatalogId nilCatalogId = {0, 0};
> + Assert(fout != NULL);
> +
> + ArchiveEntry(fout,
> + nilCatalogId, /* catalog ID */
> + createDumpId(), /* dump ID */
> + ARCHIVE_OPTS(.tag = tag,
> + .description = tag,
> + .section = SECTION_PRE_DATA,
> + .createStmt = query));
> +}
> this is only used when archDumpFormat is not archNull.
> comments can change to
> "This creates one archive entry for non-text archive"
>
Fixed.
>
> +static int
> +restore_one_database(const char *inputFileSpec, RestoreOptions *opts,
> + int numWorkers, bool append_data, int num, bool globals_only)
> I guess, "num" means number of databases, but the name is
> "restore_one_database".
> seems confusing. Similarly, I am confused by
> restore_global_objects parameter "num".
>
Fixed. I removed num.
> + pg_log_error("option %s must be specified when restoring an archive
> created by pg_dumpall",
> + "-C/--create");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + pg_log_error_hint("Individual databases can be restored using their
> specific archives.");
> Here we report that --create must be specified.
> The second pg_log_error_hint() message about restoring individual databases
> seems unrelated to this requirement, and seems confusing in this context.
We are giving some extra info if the user wants to restore one
database from the dump of pg_dumpall.
>
> get_dbnames_list_to_restore
> + if (!conn && db_exclude_patterns.head != NULL)
> + pg_log_info("considering PATTERN as NAME for --exclude-database
> option as no database connection while doing pg_restore");
> is unreachable. because conn is always not NULL,
> Since restore_all_databases "template1", template database "template1" is
> undroppable, see ``dbcommands.c:1734``.
>
yes, this is unreachable but we are keeping this.
> get_dbname_oid_list_from_mfile does not handle database names that contain
> newline characters correctly.
> For example:
> CREATE DATABASE "test
> \r";
>
> I am unable to dump and restore a database with such a name.
We have another thread for this. We have patches also. Last year, we
planned to block these databases at creation time.
>
> It's probably harmless, we connect to the databases further down to do actual
> work. But it's also not nice. The toc.glo seems to have a bunch of extraneous
> entries of type COMMENT and CONNECT. Why is that? As far as poible this
> should have output pretty much identical to a plain pg_dumpall.
>
>
> cheers
>
>
> andrew
If we don't dump those comments in non-text format, then the output of
"pg_restore -f filename dump_non_text" will not be the same as the
plain dump of pg_dumpall.
Here, I am attaching an updated patch for the review and testing.
Note: some of the review comments are still not fixed. I am working on
those and will post an updated patch.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
v13_06012026-Non-text-modes-for-pg_dumpall-correspondingly-change.patch
Description: Binary data
