Re: Non-text mode for pg_dumpall

Hi Mahendra,

I have a few more review comments regarding the patch:

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
@@ -287,7 +287,6 @@ executeQuery(PGconn *conn, const char *query)
{
pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error_detail("Query was: %s", query);
- PQfinish(conn);
exit_nicely(1);
}
```

When I re-added `PQfinish(conn);`, the regression tests passed successfully.
The `git diff` shows:

```
diff --git a/src/bin/pg_dump/connectdb.c b/src/bin/pg_dump/connectdb.c
index f44a8a45fca..d55d53dbeea 100644
--- a/src/bin/pg_dump/connectdb.c
+++ b/src/bin/pg_dump/connectdb.c
@@ -287,6 +287,7 @@ executeQuery(PGconn *conn, const char *query)
{
pg_log_error("query failed: %s", PQerrorMessage(conn));
pg_log_error_detail("Query was: %s", query);
+ PQfinish(conn);
exit_nicely(1);
}
```

If this change is intentional, could you please add a test case to justify
or demonstrate the need for it?

2. Please remove the extra blank line before `static void usage(const char
*progname);`.

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

3. There is an unnecessary line deletion that does not appear to be related
to this feature:

```
opts->cparams.pghost = pg_strdup(optarg);
break;
-
```

Could this deletion be part of a separate cleanup?

Regards,
Vaibhav Dalvi

On Mon, Nov 3, 2025 at 12:05 PM Vaibhav Dalvi <
vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:

> Hi Mahendra,
>
> Thank you for your work on this feature.
> I have just begun reviewing the latest patch and
> encountered the following errors during the initial setup:
>
> ```
> $ ./db/bin/pg_restore testdump_dir -C -d postgres -F d -p 5556
> pg_restore: error: could not execute query: ERROR: syntax error at or near
> "\\"
> LINE 1: \restrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj...
> ^
> Command was: \restrict
> aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hb
>
> pg_restore: error: could not execute query: ERROR: syntax error at or near
> "\\"
> LINE 1: \unrestrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCj...
> ^
> Command was: \unrestrict
> aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hb
>
> pg_restore: error: could not execute query: ERROR: syntax error at or near
> "\\"
> LINE 1: \connect template1
> ^
> Command was: \connect template1
>
> pg_restore: error: could not execute query: ERROR: syntax error at or near
> "\\"
> LINE 1: \connect postgres
> ^
> Command was: \connect postgres
> ```
> To cross-check tried with plain dump(with pg_dumpall) and
> restored(SQL file restore) without patch and didn't get above
> connection errors.
>
> It appears there might be an issue with the dump file itself.
> Please note that this is my first observation as I have just
> started the review. I will continue with my assessment.
>
> Regards,
> Vaibhav Dalvi
> EnterpriseDB
>
> On Fri, Oct 31, 2025 at 2:51 PM Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
> wrote:
>
>> On Tue, 28 Oct 2025 at 11:32, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
>> wrote:
>> >
>> > On Thu, 16 Oct 2025 at 16:24, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
>> wrote:
>> > >
>> > > On Wed, 15 Oct 2025 at 23:05, Mahendra Singh Thalor <
>> mahi6run(at)gmail(dot)com> wrote:
>> > > >
>> > > > On Sun, 24 Aug 2025 at 22:12, Andrew Dunstan <andrew(at)dunslane(dot)net>
>> wrote:
>> > > > >
>> > > > >
>> > > > > On 2025-08-23 Sa 9:08 PM, Noah Misch wrote:
>> > > > >
>> > > > > On Wed, Jul 30, 2025 at 02:51:59PM -0400, Andrew Dunstan wrote:
>> > > > >
>> > > > > OK, now that's reverted we should discuss how to proceed. I had
>> two thoughts
>> > > > > - we could use invent a JSON format for the globals, or we could
>> just use
>> > > > > the existing archive format. I think the archive format is pretty
>> flexible,
>> > > > > and should be able to accommodate this. The downside is it's not
>> humanly
>> > > > > readable. The upside is that we don't need to do anything special
>> either to
>> > > > > write it or parse it.
>> > > > >
>> > > > > I would first try to use the existing archiver API, because that
>> makes it
>> > > > > harder to miss bugs. Any tension between that API and pg_dumpall
>> is likely to
>> > > > > have corresponding tension on the pg_restore side. Resolving
>> that tension
>> > > > > will reveal much of the project's scope that remained hidden
>> during the v18
>> > > > > attempt. Perhaps more important than that, using the archiver
>> API means
>> > > > > future pg_dump and pg_restore options are more likely to
>> cooperate properly
>> > > > > with $SUBJECT. In other words, I want it to be hard to add
>> pg_dump/pg_restore
>> > > > > features that malfunction only for $SUBJECT archives. The
>> strength of the
>> > > > > archiver architecture shows in how rarely new features need
>> format-specific
>> > > > > logic and how rarely format-specific bugs get reported. We've
>> had little or
>> > > > > no trouble with e.g. bugs that appear in -Fd but not in -Fc.
>> > > > >
>> > > > >
>> > > > > Yeah, that's what we're going to try.
>> > > > >
>> > > > >
>> > > > > cheers
>> > > > >
>> > > > >
>> > > > > andrew
>> > > > >
>> > > > > --
>> > > > > Andrew Dunstan
>> > > > > EDB: https://www.enterprisedb.com
>> > > >
>> > > > Thanks Andrew, Noah and all others for feedback.
>> > > >
>> > > > Based on the above suggestions and discussions, I removed sql
>> commands
>> > > > from the global.dat file. For global commands, now we are making
>> > > > toc.dat/toc.dmp/toc.tar file based on format specified and based on
>> > > > format specified, we are making archive entries for these global
>> > > > commands. By this approach, we removed the hard-coded parsing part
>> of
>> > > > the global.dat file and we are able to skip DROP DATABASE with the
>> > > > globals-only option.
>> > > >
>> > > > Here, I am attaching a patch for review, testing and feedback. This
>> is
>> > > > a WIP patch. I will do some more code cleanup and will add some more
>> > > > comments also. Please review this and let me know design level
>> > > > feedback. Thanks Tushar Ahuja for some internal testing and
>> feedback.
>> > > >
>> > >
>> > > Hi,
>> > > Here, I am attaching an updated patch. In offline discussion, Andrew
>> > > reported some test-case failures(Thanks Andrew). I fixed those.
>> > > Please let me know feedback for the patch.
>> > >
>> >
>> > Hi,
>> > Here I am attaching a re-based patch as v02 was failing on head.
>> > Thanks Tushar for the testing.
>> > Please review this and let me know feedback.
>> >
>>
>> Hi all,
>> Here I am attaching an updated patch for review and testing. Based on
>> some offline comments by Andrew, I did some code cleanup.
>> Please consider this patch for feedback.
>>
>> --
>> Thanks and Regards
>> Mahendra Singh Thalor
>> EnterpriseDB: http://www.enterprisedb.com
>>
>