Re: Non-text mode for pg_dumpall - Mailing list pgsql-hackers
| From | Vaibhav Dalvi | 
|---|---|
| Subject | Re: Non-text mode for pg_dumpall | 
| Date | |
| Msg-id | CA+vB=AE4SSSqRdPFWxe0zbq1n5ePo8iVHoXQGsu0Xr2srh_yDA@mail.gmail.com Whole thread Raw  | 
		
| In response to | Re: Non-text mode for pg_dumpall (Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.com>) | 
| List | pgsql-hackers | 
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.dalvi@enterprisedb.com> wrote:
Hi Mahendra,Thank you for your work on this feature.I have just begun reviewing the latest patch andencountered the following errors during the initial setup:```$ ./db/bin/pg_restore testdump_dir -C -d postgres -F d -p 5556pg_restore: error: could not execute query: ERROR: syntax error at or near "\\"LINE 1: \restrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj...^Command was: \restrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hbpg_restore: error: could not execute query: ERROR: syntax error at or near "\\"LINE 1: \unrestrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCj...^Command was: \unrestrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hbpg_restore: error: could not execute query: ERROR: syntax error at or near "\\"LINE 1: \connect template1^Command was: \connect template1pg_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) andrestored(SQL file restore) without patch and didn't get aboveconnection errors.It appears there might be an issue with the dump file itself.Please note that this is my first observation as I have juststarted the review. I will continue with my assessment.Regards,Vaibhav DalviEnterpriseDBOn Fri, Oct 31, 2025 at 2:51 PM Mahendra Singh Thalor <mahi6run@gmail.com> wrote:On Tue, 28 Oct 2025 at 11:32, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Thu, 16 Oct 2025 at 16:24, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> >
> > On Wed, 15 Oct 2025 at 23:05, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> > >
> > > On Sun, 24 Aug 2025 at 22:12, Andrew Dunstan <andrew@dunslane.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
pgsql-hackers by date: