Thread: Re: [PATCH] Add --create-only option to pg_dump/pg_dumpall
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested Hi I have tried the patch and the new option is able to control the contents of pg_dump outputs to include only create db relatedcommands. I also agree that the option name is a little misleading to the user so I would suggest instead of using"create-only", you can say something maybe like "createdb-only" because this option only applies to CREATE DATABASErelated commands, not CREATE TABLE or other objects. In the help menu, you can then elaborate more that this option"dump only the commands related to create database like ALTER, GRANT..etc" Cary Huang ------------- HighGo Software Inc. (Canada) cary.huang@highgo.ca www.highgo.ca
Hi, Am Montag, den 29.03.2021, 17:59 +0000 schrieb Cary Huang: > I have tried the patch and the new option is able to control the > contents of pg_dump outputs to include only create db related > commands. Thanks for testing! > I also agree that the option name is a little misleading to the user > so I would suggest instead of using "create-only", you can say > something maybe like "createdb-only" because this option only applies > to CREATE DATABASE related commands, not CREATE TABLE or other > objects. In the help menu, you can then elaborate more that this > option "dump only the commands related to create database like ALTER, > GRANT..etc" Well I have to say I agree with Peter that the option name I came up with is pretty confusing, not sure createdb-only is much better as it also includes GRANTs etc. I think from a technical POV it's useful as it closes a gap between pg_dumpall -g and pg_dump -Fc $DATABASE in my opinion, without having to potentially schema-dump and filter out a large number of database objects. Anybody else have some opinions on what to call this best? Maybe just a short option and some explanatory text in --help along with it? Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.de credativ GmbH, HRB Mönchengladbach 12080 USt-ID-Nummer: DE204566209 Trompeterallee 108, 41189 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer Unser Umgang mit personenbezogenen Daten unterliegt folgenden Bestimmungen: https://www.credativ.de/datenschutz
On Tue, Mar 30, 2021 at 6:02 PM Michael Banck <michael.banck@credativ.de> wrote: > > Hi, > > Am Montag, den 29.03.2021, 17:59 +0000 schrieb Cary Huang: > > I have tried the patch and the new option is able to control the > > contents of pg_dump outputs to include only create db related > > commands. > > Thanks for testing! > > > I also agree that the option name is a little misleading to the user > > so I would suggest instead of using "create-only", you can say > > something maybe like "createdb-only" because this option only applies > > to CREATE DATABASE related commands, not CREATE TABLE or other > > objects. In the help menu, you can then elaborate more that this > > option "dump only the commands related to create database like ALTER, > > GRANT..etc" > > Well I have to say I agree with Peter that the option name I came up > with is pretty confusing, not sure createdb-only is much better as it > also includes GRANTs etc. > > I think from a technical POV it's useful as it closes a gap between > pg_dumpall -g and pg_dump -Fc $DATABASE in my opinion, without having to > potentially schema-dump and filter out a large number of database > objects. > > Anybody else have some opinions on what to call this best? Maybe just a > short option and some explanatory text in --help along with it? Maybe --database-globals or something like that? Other than the name (which might be influenced by this), shouldn't this functionality be in pg_restore as well? That is, if I make a pg_dump in custom format, I would want to be able to extract that information from the dump as well? -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Hi,
I have reviewed and tested the patch. Following are a few comments.
1.
The main objective of this patch is to get the dump which consists of SQLs related to
CREATEDB only. I have tested the patch and it generates a proper dump file. But my
concern is, it should execute the code which is necessary. But I see that the code
is preparing some data which we may not dump. So I feel we should avoid executing
such flows. Please correct me if I am wrong.
2.
> > I also agree that the option name is a little misleading to the user
> > so I would suggest instead of using "create-only", you can say
> > something maybe like "createdb-only" because this option only applies
> > to CREATE DATABASE related commands, not CREATE TABLE or other
> > objects. In the help menu, you can then elaborate more that this
> > option "dump only the commands related to create database like ALTER,
> > GRANT..etc"
>
> Well I have to say I agree with Peter that the option name I came up
> with is pretty confusing, not sure createdb-only is much better as it
> also includes GRANTs etc.
> > so I would suggest instead of using "create-only", you can say
> > something maybe like "createdb-only" because this option only applies
> > to CREATE DATABASE related commands, not CREATE TABLE or other
> > objects. In the help menu, you can then elaborate more that this
> > option "dump only the commands related to create database like ALTER,
> > GRANT..etc"
>
> Well I have to say I agree with Peter that the option name I came up
> with is pretty confusing, not sure createdb-only is much better as it
> also includes GRANTs etc.
I agree with Cary that we should name this as 'createdb-only' and provide a brief
description in help.
3.
if (!plainText)
dopt.outputCreateDB = 1;
+ if (dopt.outputCreateDBOnly)
+ dopt.outputCreateDB = 1;
+
'dopt.outputCreateDBOnly' if block can be merged with '!plainText' if block.
4.
static int binary_upgrade = 0;
static int column_inserts = 0;
+static int create_only = 0;
static int disable_dollar_quoting = 0;
The variable 'create_only' should be changed to 'createdb_only' to match with
similar variable used in pg_dump.c.
Thanks and Regards,
Nitin Jadhav
On Tue, Mar 30, 2021 at 9:32 PM Michael Banck <michael.banck@credativ.de> wrote:
Hi,
Am Montag, den 29.03.2021, 17:59 +0000 schrieb Cary Huang:
> I have tried the patch and the new option is able to control the
> contents of pg_dump outputs to include only create db related
> commands.
Thanks for testing!
> I also agree that the option name is a little misleading to the user
> so I would suggest instead of using "create-only", you can say
> something maybe like "createdb-only" because this option only applies
> to CREATE DATABASE related commands, not CREATE TABLE or other
> objects. In the help menu, you can then elaborate more that this
> option "dump only the commands related to create database like ALTER,
> GRANT..etc"
Well I have to say I agree with Peter that the option name I came up
with is pretty confusing, not sure createdb-only is much better as it
also includes GRANTs etc.
I think from a technical POV it's useful as it closes a gap between
pg_dumpall -g and pg_dump -Fc $DATABASE in my opinion, without having to
potentially schema-dump and filter out a large number of database
objects.
Anybody else have some opinions on what to call this best? Maybe just a
short option and some explanatory text in --help along with it?
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
> On 9 Apr 2021, at 15:34, Nitin Jadhav <nitinjadhavpostgres@gmail.com> wrote: > I have reviewed and tested the patch. Following are a few comments. This review has gone unanswered since April, has been WoA since early April and the patch no longer applies. I'm marking this Returned with Feedback, a new version can be submitted for a future CF once the issues have been resolved. -- Daniel Gustafsson https://vmware.com/