Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1vFtEO-0031IY-GL for pgsql-hackers@arkaria.postgresql.org; Mon, 03 Nov 2025 12:00:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1vFtEN-002rQt-1z for pgsql-hackers@arkaria.postgresql.org; Mon, 03 Nov 2025 12:00:14 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1vFtEM-002rQk-Lf for pgsql-hackers@lists.postgresql.org; Mon, 03 Nov 2025 12:00:13 +0000 Received: from mail-ua1-x936.google.com ([2607:f8b0:4864:20::936]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vFtEI-005k0q-0L for pgsql-hackers@lists.postgresql.org; Mon, 03 Nov 2025 12:00:12 +0000 Received: by mail-ua1-x936.google.com with SMTP id a1e0cc1a2514c-934f0e9d4afso1920800241.1 for ; Mon, 03 Nov 2025 04:00:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; t=1762171207; x=1762776007; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=IXzVZDanmHFYGc8gZ4bc2LrVJGoU2ur5YnHQRKxxg1M=; b=iF25mIu5lfI05SFNfST662wortR09qJvgeZL7FPjnv5WVRnHYTcGC5WJuNPOkUCSjL 95wOWonUbtawaL/XQftpjsBCnE8u8NsVw0rxbmU7fOrUdpZjzdWqlxIlGJUfROwLDmYO MODZlh1/6uIMq54ygZ438mNvG99NaiXjqmpM4rT0y8tYzrWJf+DERnHeX2/KeMdrJtc5 KX8LyF45b3xu/GC3odyy3bEPY3TiyMVg0wjgcgMkvVnQEaOyAEu9Ogos2eoLkS1sAroO rZf3hqnzWQw12EKpiG5EbwTN7JoDINn6UmkuNzgp0gK/TiLU+i+pK2QazOYDH/pq/seF TvAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762171207; x=1762776007; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=IXzVZDanmHFYGc8gZ4bc2LrVJGoU2ur5YnHQRKxxg1M=; b=aMEQvPJ+P8MyIi/HimjMsWTVsyUIUQTnj6+TTEEv+FP7Qhw2JP9r/CrXCl8AXV3irj DxuN1NCZ+oZ3DzYtuKCdkKraNduvi3OEo87U+tBNI6x3Zn6FULiNiUTcgGG6w1Eg41jW TIqipykRTPY/bhkERkialwexOUX8SF9wPMx0eBS78yVvSd63sZ94RxxjHldZZVIYrmDU TeF66DEHyU2KzCwSkRGRvmL/B227RsyXMElc2Xo3vIBGRC9ORPzVlIFLYRagtgzrTw83 YwhIvmad654oAskxTV6pofCJeSPbmEzgyR8ljKlKrQwsPt3Rem8twO7JFZsIEDT+ynLY BZew== X-Gm-Message-State: AOJu0Yw+lCeZQEpoapKREjzfSfXWhsEQwN6kxuF/iIHlJDDsDtzskexY VYVGNLcAbfEGB+ej0FmrlYojfo+F0/Mo/CorlNaoDmexriz9Xkr5gnoiGvRhH9Grbc6CphQuUrh /UpuPqqKdQiIeD4kU2ZSj8uu7gBmkdzLz4kneigZd X-Gm-Gg: ASbGncsUe0xyHyFjMB7HKYZvbp8n1lIKXAuISIdUJJ2OSlCfGGmWTP1hf8CaPiHjJ8G 7ZTjjilAylifc+F+xF6ukb7P5I+rrux37tP91NBSM+19sKb3z9ncdLQo6rbwgLyrHkYjEHBnr28 d5CNo6elXbrhX4OwjUMwvOWIymO+NCUF+KZ87uTssWKEuaKvtJyg062FmBX7PVnmD4hRm3U2o2M jSsbafwZfKvdwW/HDMGBb8mm5n1+eHDBL3bUGhvBVQMuhgI2qYNCTH5QcWyjaDUPHQIq/mA X-Google-Smtp-Source: AGHT+IHC5tY3BRPis5oqlSospw6wfmLQNJzCyR4k68+Vd40PAVRfrbtC0i0BvuPT80c4Tv2izvwi/JjgP6yaTkmExuU= X-Received: by 2002:a05:6122:3c8b:b0:559:65d6:1678 with SMTP id 71dfb90a1353d-55965d61df7mr798965e0c.16.1762171207070; Mon, 03 Nov 2025 04:00:07 -0800 (PST) MIME-Version: 1.0 References: <3f22a8bb-29e8-40cc-97a1-309181da2c13@dunslane.net> <20250722005339.ca.nmisch@google.com> <20250725162141.6f.nmisch@google.com> <2225040.1753477169@sss.pgh.pa.us> <20250727235628.e2.nmisch@google.com> <8f2ad50b-ebb7-4adc-997e-25e0ad96ff34@dunslane.net> <2bed001a-462c-42da-9a6b-3c7884502932@dunslane.net> <20250824010811.4d.nmisch@google.com> <82eb35b8-7f07-493b-b689-0934919e1dc3@dunslane.net> In-Reply-To: From: Vaibhav Dalvi Date: Mon, 3 Nov 2025 17:29:30 +0530 X-Gm-Features: AWmQ_blV4SqNoQgmqktzmh_ZRwrNJNtU3k4O6soh9D37PgfXZnILEVhGkJdo8zU Message-ID: Subject: Re: Non-text mode for pg_dumpall To: Mahendra Singh Thalor Cc: pgsql-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000d4c7110642af746f" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000d4c7110642af746f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 =3D pg_strdup(optarg); break; - ``` Could this deletion be part of a separate cleanup? Regards, Vaibhav Dalvi On Mon, Nov 3, 2025 at 12:05=E2=80=AFPM 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 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 nea= r > "\\" > LINE 1: \restrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj... > ^ > Command was: \restrict > aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hb > > pg_restore: error: could not execute query: ERROR: syntax error at or nea= r > "\\" > LINE 1: \unrestrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCj... > ^ > Command was: \unrestrict > aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hb > > pg_restore: error: could not execute query: ERROR: syntax error at or nea= r > "\\" > LINE 1: \connect template1 > ^ > Command was: \connect template1 > > pg_restore: error: could not execute query: ERROR: syntax error at or nea= r > "\\" > 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=E2=80=AFPM Mahendra Singh Thalor > wrote: > >> On Tue, 28 Oct 2025 at 11:32, Mahendra Singh Thalor >> wrote: >> > >> > On Thu, 16 Oct 2025 at 16:24, Mahendra Singh Thalor >> 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 >> 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 prett= y >> 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 specia= l >> 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_dumpal= l >> 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 o= n >> > > > 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. Thi= s >> is >> > > > a WIP patch. I will do some more code cleanup and will add some mo= re >> > > > 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 >> > --000000000000d4c7110642af746f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Mahendra,

I have a few mo= re review comments regarding the patch:

1. Is the= following change in `src/bin/pg_dump/connectdb.c` intentional?
<= br>
```
--- 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);
}<= /div>
```

When I re-added `PQfinish(co= nn);`, the regression tests passed successfully.
The `git diff` s= hows:

```
diff --git a/src/bin/p= g_dump/connectdb.c b/src/bin/pg_dump/connectdb.c
index f44a8a= 45fca..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", P= QerrorMessage(conn));
pg_log_error_detail(&qu= ot;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 `s= tatic void usage(const char *progname);`.

```<= /div>
+
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 =3D pg_strdu= p(optarg);
break;
-
```

Could this deletion be part of a separate cleanup= ?

Regards,
Vaibhav Dalvi

=
On Mon, No= v 3, 2025 at 12:05=E2=80=AFPM Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.com= > wrote:
Hi Mahendra,

Thank you for your = work on this feature.
I have just begun reviewing the latest patc= h and
encountered the following errors during the initial setup:<= /div>

```
$ ./db/bin/pg_restore testdump_dir -= C -d postgres -F d -p 5556
pg_restore: error: could not execute q= uery: ERROR: syntax error at or near "\\"
LINE 1: \rest= rict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCjCj...
= ^
Command was: \restrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qs= kGaFDygTCjCj9vg3Xxys1b3hb

pg_restore: error: could= not execute query: ERROR: syntax error at or near "\\"
LINE 1: \unrestrict aO9K1gzVZTlafidF5fWx8ADGzUnIiAcguFz5qskGaFDygTCj...
^
Command was: \unrestrict aO9K1gzVZTlafidF5fWx8A= DGzUnIiAcguFz5qskGaFDygTCjCj9vg3Xxys1b3hb

pg_resto= re: error: could not execute query: ERROR: syntax error at or near "\\= "
LINE 1: \connect template1
^
C= ommand 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 d= ump(with pg_dumpall) and
=C2=A0restored(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
<= div>started the review. I will continue with my assessment.

<= /div>
Regards,
Vaibhav Dalvi
EnterpriseDB

On F= ri, Oct 31, 2025 at 2:51=E2=80=AFPM 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> w= rote:
> > > >
> > > >
> > > > On 2025-08-23 Sa 9:08 PM, Noah Misch wrote:
> > > >
> > > > On Wed, Jul 30, 2025 at 02:51:59PM -0400, Andrew Dunsta= n wrote:
> > > >
> > > > OK, now that's reverted we should discuss how to pr= oceed. 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 an= ything special either to
> > > > write it or parse it.
> > > >
> > > > I would first try to use the existing archiver API, bec= ause that makes it
> > > > harder to miss bugs.=C2=A0 Any tension between that API= and pg_dumpall is likely to
> > > > have corresponding tension on the pg_restore side.=C2= =A0 Resolving that tension
> > > > will reveal much of the project's scope that remain= ed hidden during the v18
> > > > attempt.=C2=A0 Perhaps more important than that, using = the archiver API means
> > > > future pg_dump and pg_restore options are more likely t= o cooperate properly
> > > > with $SUBJECT.=C2=A0 In other words, I want it to be ha= rd to add pg_dump/pg_restore
> > > > features that malfunction only for $SUBJECT archives.= =C2=A0 The strength of the
> > > > archiver architecture shows in how rarely new features = need format-specific
> > > > logic and how rarely format-specific bugs get reported.= =C2=A0 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 sq= l commands
> > > from the global.dat file. For global commands, now we are ma= king
> > > toc.dat/toc.dmp/toc.tar file based on format specified and b= ased on
> > > format specified, we are making archive entries for these gl= obal
> > > commands. By this approach, we removed the hard-coded parsin= g part of
> > > the global.dat file and we are able to skip DROP DATABASE wi= th the
> > > globals-only option.
> > >
> > > Here, I am attaching a patch for review, testing and feedbac= k. This is
> > > a WIP patch. I will do some more code cleanup and will add s= ome more
> > > comments also. Please review this and let me know design lev= el
> > > feedback. Thanks Tushar Ahuja for some internal testing and = feedback.
> > >
> >
> > Hi,
> > Here, I am attaching an updated patch. In offline discussion, And= rew
> > 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
--000000000000d4c7110642af746f--