public inbox for [email protected]
help / color / mirror / Atom feedFrom: Vaibhav Dalvi <[email protected]>
To: Mahendra Singh Thalor <[email protected]>
Cc: [email protected]
Subject: Re: Non-text mode for pg_dumpall
Date: Wed, 5 Nov 2025 12:29:53 +0530
Message-ID: <CA+vB=AE9ypeQV-hhXscJ6T75BrAFfZh56GX_eM=eVG2n1o_Beg@mail.gmail.com> (raw)
In-Reply-To: <CAKYtNAqJqDmKcqCzpHg2SO=2MTxvE7rOWCACsoWsO7520tUWKw@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAKYtNAr+OMjcGKwd+AgWA+s=8EWGtg5PkVV3O7X6d3eCv=MUeQ@mail.gmail.com>
<CAKYtNApzLLeCqt5fHDzZOTnzCdCnBt3Y_fytFmJ0LMNHDPY-yA@mail.gmail.com>
<CAKYtNArSYJdjez541C0qPZ9R3_yzPDpScAd=-RU4kPRCJh4viQ@mail.gmail.com>
<CAKYtNArGUTzrfTBxpftL_yAgkKE+hcDgXVfQvjB2HFO9rGhE5g@mail.gmail.com>
<CA+vB=AEQc3Xqz+KMh35Zsa-SsRP-n=HXGT4RwwNou4-__Prx+A@mail.gmail.com>
<CAKYtNAqJqDmKcqCzpHg2SO=2MTxvE7rOWCACsoWsO7520tUWKw@mail.gmail.com>
Hi Mahendra,
Thank you for the fix. Please find my further review comments below.
### Restrict-Key Option
The `--restrict-key` option is currently being accepted by
`pg_dumpall` even when non-plain formats are specified,
which contradicts its intended use only with the plain format.
For example:
```
$ ./db/bin/pg_dump --format=d -f testdump_dir --restrict-key=RESTRICT_KEY
pg_dump: error: option --restrict-key can only be used with --format=plain
$ ./db/bin/pg_dumpall --format=d -f testdump_dir --restrict-key=RESTRICT_KEY
pg_dumpall: error: invalid restrict key
```
I have attached a delta patch that addresses the issue with the
`--restrict-key` option. It would be beneficial to include a dedicated
test case for this check.
### 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;
```
In my attached delta file, I have replaced the unnecessary
`restrict_key` variable with `dopt.restrict_key`.
### Cosmetic Issues
1. Please review the spacing around the pointer:
```c
+ ((ArchiveHandle * )fout) ->connection = conn;
+ ((ArchiveHandle * ) fout) -> public.numWorkers = 1;
```
2. Please be consistent with the punctuation of single-line comments;
some end with a full stop (`.`) and others do not.
3. In the SGML documentation changes, some new statements start
with one space, and others start with two. Please adhere to a single
standard for indentation across the patch.
Regards,
Vaibhav
EnterpriseDB
On Mon, Nov 3, 2025 at 5:24 PM Mahendra Singh Thalor <[email protected]>
wrote:
> On Mon, 3 Nov 2025 at 12:06, Vaibhav Dalvi <[email protected]>
> 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
>
> Thanks Vaibhav for the review.
> This change was added by me in v04. Only in the case of a file, we should
> restore these commands. Attached patch is fixing the same.
>
> If we dump and restore the same file with the same user, then we will get
> an error of ROLE CREATE as the same role is already created. I think,
> either we can ignore this error, or we can keep it as a restore can be done
> with different users.
>
>> mst@localhost bin]$ ./pg_restore d1 -C -d postgres
>> pg_restore: error: could not execute query: ERROR: role "mst" already
>> exists
>> Command was: CREATE ROLE mst;
>> ALTER ROLE mst WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN
>> REPLICATION BYPASSRLS;
>>
>>
>> pg_restore: warning: errors ignored on restore: 1
>
>
>
> >
> > On Fri, Oct 31, 2025 at 2:51 PM Mahendra Singh Thalor <
> [email protected]> wrote:
> >>
> >> On Tue, 28 Oct 2025 at 11:32, Mahendra Singh Thalor <[email protected]>
> wrote:
> >> >
> >> > On Thu, 16 Oct 2025 at 16:24, Mahendra Singh Thalor <
> [email protected]> wrote:
> >> > >
> >> > > On Wed, 15 Oct 2025 at 23:05, Mahendra Singh Thalor <
> [email protected]> wrote:
> >> > > >
> >> > > > On Sun, 24 Aug 2025 at 22:12, Andrew Dunstan <[email protected]>
> 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
>
>
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com
>
Attachments:
[application/octet-stream] delta-v05-non-text-modes-for-pg_dumpall.patch (3.7K, 3-delta-v05-non-text-modes-for-pg_dumpall.patch)
download | inline diff:
From 4debd839b5dbbfd188ad2422112f8e303c5d7a71 Mon Sep 17 00:00:00 2001
From: Vaibhav Dalvi <[email protected]>
Date: Wed, 5 Nov 2025 06:22:00 +0000
Subject: [PATCH v1 1/1] delta v05 non-text modes for pg_dumpall
This delta patch is to fix --restrict-key
with non-text dump format.
Vaibhav Dalvi
---
src/bin/pg_dump/pg_dumpall.c | 52 +++++++++++++-----------------------
1 file changed, 19 insertions(+), 33 deletions(-)
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 601b9f9738e..9e447dc9738 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -127,7 +127,6 @@ static char *filename = NULL;
static SimpleStringList database_exclude_patterns = {NULL, NULL};
static SimpleStringList database_exclude_names = {NULL, NULL};
-static char *restrict_key;
static Archive *fout = NULL;
static pg_compress_specification compression_spec = {0};
static int dumpIdVal = 0;
@@ -397,7 +396,7 @@ main(int argc, char *argv[])
break;
case 9:
- restrict_key = pg_strdup(optarg);
+ dopt.restrict_key = pg_strdup(optarg);
appendPQExpBufferStr(pgdumpopts, " --restrict-key ");
appendShellString(pgdumpopts, optarg);
break;
@@ -555,15 +554,20 @@ main(int argc, char *argv[])
else
OPF = stdout;
- /*
- * If you don't provide a restrict key, one will be appointed for you.
- */
- if (!restrict_key)
- restrict_key = generate_restrict_key();
- if (!restrict_key)
- pg_fatal("could not generate restrict key");
- if (!valid_restrict_key(restrict_key))
- pg_fatal("invalid restrict key");
+ if (archDumpFormat == archNull)
+ {
+ /*
+ * If you don't provide a restrict key, one will be appointed for you.
+ */
+ if (!dopt.restrict_key)
+ dopt.restrict_key = generate_restrict_key();
+ if (!dopt.restrict_key)
+ pg_fatal("could not generate restrict key");
+ if (!valid_restrict_key(dopt.restrict_key))
+ pg_fatal("invalid restrict key");
+ }
+ else if (dopt.restrict_key)
+ pg_fatal("option --restrict-key can only be used with --format=plain");
/*
* If there was a database specified on the command line, use that,
@@ -670,15 +674,6 @@ main(int argc, char *argv[])
createOneArchiveEntry("--\n-- PostgreSQL database cluster dump\n--\n\n", "COMMENT");
- /* create entry for restrict */
- {
- PQExpBuffer qry = createPQExpBuffer();
-
- appendPQExpBuffer(qry, "\\restrict %s\n\n", restrict_key);
- createOneArchiveEntry(qry->data, "RESTRICT");
- destroyPQExpBuffer(qry);
- }
-
/* default_transaction_read_only = off */
{
PQExpBuffer qry = createPQExpBuffer();
@@ -727,7 +722,7 @@ main(int argc, char *argv[])
* meta-commands so that the client machine that runs psql with the dump
* output remains unaffected.
*/
- fprintf(OPF, "\\restrict %s\n\n", restrict_key);
+ fprintf(OPF, "\\restrict %s\n\n", dopt.restrict_key);
/*
* We used to emit \connect postgres here, but that served no purpose
@@ -793,19 +788,10 @@ main(int argc, char *argv[])
if (archDumpFormat == archNull)
{
/*
- * Exit restricted mode just before dumping the databases. pg_dump will
- * handle entering restricted mode again as appropriate.
+ * Exit restricted mode just before dumping the databases. pg_dump
+ * will handle entering restricted mode again as appropriate.
*/
- fprintf(OPF, "\\unrestrict %s\n\n", restrict_key);
- }
- else
- {
- /* create entry for unrestrict */
- PQExpBuffer qry = createPQExpBuffer();
-
- appendPQExpBuffer(qry, "\\unrestrict %s\n\n", restrict_key);
- createOneArchiveEntry(qry->data, "UNRESTRICT");
- destroyPQExpBuffer(qry);
+ fprintf(OPF, "\\unrestrict %s\n\n", dopt.restrict_key);
}
if (!globals_only && !roles_only && !tablespaces_only)
--
2.43.0
view thread (100+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Non-text mode for pg_dumpall
In-Reply-To: <CA+vB=AE9ypeQV-hhXscJ6T75BrAFfZh56GX_eM=eVG2n1o_Beg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox