Horton

Thread

Commits

  1. Move handling of database properties from pg_dumpall into pg_dump.

  2. Allow pg_dumpall to dump roles w/o user passwords

  3. Fix pg_dumpall to work for databases flagged as read-only.

  1. Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-01T01:59:46Z

    Subject changed for better context of the patch.
(was - Re: Question about grant create on database and pg_dump/pg_dumpall)

On Fri, Sep 30, 2016 at 12:29 AM, Tom Lane
<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>1. pg_dump without --create continues to do what it does today, ie it just
>dumps objects within the database, assuming that database-level properties
>will already be set correctly for the target database.
>
>2. pg_dump with --create creates the target database and also sets all
>database-level properties (ownership, ACLs, ALTER DATABASE SET, etc etc).
>
>3. pg_dumpall loses all code relating to individual-database creation
>and property setting and instead relies on pg_dump --create to do that.
>This would leave only the code relating to "pg_dumpall -g" (ie, dump roles
>and tablespaces) within pg_dumpall itself.

I removed all the database related code from pg_dumpall and moved the
necessary part of the code into pg_dump and called pg_dump with --create
option from pg_dumpall to ensure that all the database create commands
are getting dumped.

Except postgres, template1 databases for rest of the databases the
CREATE DATABASE command is issued. And all other properties
dump is same for every database.

>One thing that would still be messy is that presumably "pg_dumpall -g"
>would issue ALTER ROLE SET commands, but it's unclear what to do with
>ALTER ROLE IN DATABASE SET commands.  Should those become part of
>"pg_dump --create"'s charter?  It seems like not, but I'm not certain.

Yes, I moved the ALTER ROLE IN DATABASE SET commands also as part
of pg_dump --create command, this way it will be easier to dump all the
database objects using (pg_dumpall -g and pg_dump -C <database>).

>Another thing that requires some thought is that pg_dumpall is currently
>willing to dump ACLs and other properties for template1/template0, though
>it does not invoke pg_dump on them.  If we wanted to preserve that
>behavior while still moving the code that does those things to pg_dump,
>pg_dump would have to grow an option that would let it do that.  But
>I'm not sure how much of that behavior is actually sensible.

Currently the ACLs and other changes related to template database are
getting
dumped with --create option in pg_dump. do we still need another option?

>This would probably take a pg_dump archive version bump, since I think
>we don't currently record enough information for --create to do this
>(and we can't just cram the extra commands into the DATABASE entry,
>since we don't know whether --create will be specified to pg_restore).
>But we've done those before.

There is no specific code is required related to the archive version check.
Still do we need to bump the archive version? As it just adds some new
commands as part of --create with pg_dump.

Patch attached. Still some more docs needs to be added.

comments?

[1] - https://www.postgresql.org/message-id/21573.1475162949@sss.pgh.pa.us

Regards,
Hari Babu
Fujitsu Australia

  2. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-08T03:25:01Z

    On Wed, Mar 1, 2017 at 12:59 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
> Patch attached. Still some more docs needs to be added.
>

Updated patch attached to resolve the conflicts with following commit.

commit 9a83d56b38c870ce47b7651385ff2add583bf136
Author: Simon Riggs <simon@2ndQuadrant.com>
Date:   Tue Mar 7 22:00:54 2017 +0800

    Allow pg_dumpall to dump roles w/o user passwords

    Add new option --no-role-passwords which dumps roles without passwords.
    Since we don’t need passwords, we choose to use pg_roles in preference
    to pg_authid since access may be restricted for security reasons in
    some configrations.

    Robins Tharakan and Simon Riggs



Regards,
Hari Babu
Fujitsu Australia

  3. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-21T07:02:42Z

    Because of this refactor handing of database objects between
pg_dump and pg_dumpall, the latest pg_dump tap tests are
failing in the following scenarios.

1. CREATE DATABASE postgres

Before this patch, the pg_dump uses to dump the CREATE
DATABASE command of postgres but not by pg_dumpall.
During this refactor handling, the approach that I took in
pg_dump for the --create option to use the similar appraoch
of pg_dumpall to not to print the CREATE DATABASE commands
for "postgres" and "template1" databases.

It just prints the ALTER DATABASE commands to SET the
TABLESPACE for those two databases.

Solution -1) Just ignore dumping these CREATE DATABASE
commands and provide the user information in the documentation
to create "postgres" and "template1" database in the target in case
if they don't exist. If this kind of cases are very rare.

Solution-2) Add a new command line option/some other settings
to indicate the pg_dump execution is from pg_dumpall and follow
the current refactored behavior, otherwise follow the earlier pg_dump
behavior in handling CREATE DATABASE commands for "postgres"
and "template1" databases.


2.  In dumpDatabases function before calling the runPgDump command,
Before refactoring, it used to connect to the database and dump
"SET default_transaction_read_only = off;" to prevent some accidental
overwrite of the target.

I fixed it in the attached patch by removing the connection and dumping
the set command.

Does it needs the similar approach of solution-2) in previous problem and
handle dumping the "SET default_transaction_read_only = off;" whenever
the CREATE DATABASE and \connect command is issued?

Documentation is yet to update to reflect the above changes.

Regards,
Hari Babu
Fujitsu Australia

  4. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Andreas Karlsson <andreas@proxel.se> — 2017-03-23T10:45:12Z

    On 03/21/2017 08:02 AM, Haribabu Kommi wrote:
> Solution -1) Just ignore dumping these CREATE DATABASE
> commands and provide the user information in the documentation
> to create "postgres" and "template1" database in the target in case
> if they don't exist. If this kind of cases are very rare.
>
> Solution-2) Add a new command line option/some other settings
> to indicate the pg_dump execution is from pg_dumpall and follow
> the current refactored behavior, otherwise follow the earlier pg_dump
> behavior in handling CREATE DATABASE commands for "postgres"
> and "template1" databases.

I am leaning towards (2) since I feel having pg_dump act differently 
depending on the name of the database is a quite surprising behavior. It 
makes more sense to let a tool like pg_dumpall handle logic like that.

> 2.  In dumpDatabases function before calling the runPgDump command,
> Before refactoring, it used to connect to the database and dump
> "SET default_transaction_read_only = off;" to prevent some accidental
> overwrite of the target.
>
> I fixed it in the attached patch by removing the connection and dumping
> the set command.
>
> Does it needs the similar approach of solution-2) in previous problem and
> handle dumping the "SET default_transaction_read_only = off;" whenever
> the CREATE DATABASE and \connect command is issued?

Hm, that is a bit annoying. I do not think we want to change any 
behavior here, either of pg_dump or pg_dumpall, but I also do not like 
having to add two new flags to pg_dump (one for including the ALTER 
DATABASE commands but not CREATE DATABASE, and another flag for 
default_transaction_read_only) or a special flag similar to 
--binary-upgrade.

None of these options seem optimal to me, and I do not have any strong 
preference other than that we should avoid breaking pg_dump or changing 
behavior not related to the database attributes.

Andreas

  5. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Andreas Karlsson <andreas@proxel.se> — 2017-03-27T13:50:29Z

    Hi,

Here is my review. I agree with the goal of the refactoring, as we want 
to make it easier to dump all the properties for the database object. 
But I think we need to solve the issues with the special casing of 
postgres and template1 which I personally would find very surprising if 
pg_dump -C did. On the other hand I think that we cannot get away from 
having pg_dumpall give them a special treatment.

The nitpicking section is for minor code style errors.

= Functional review

I have not done an in depth functional review due to the discussion 
about how postgres and template1 should be handled.

- The patch does not apply cleanly anymore

- I do not like the change in behavior which causes "pg_dump -C 
postgres" to no longer include CREATE DATABASE. Special treatment of 
specific databases based on name makes sense in pg_dumpall, but not in 
pg_dump.

- There are test failures in the pg_dump tests. It seems like some could 
be related to that you do not include CREATE DATABASE postgres in the 
dumps but I also get errors like 'ERROR:  syntax error at or near 
"fault_tablespace"'.

not ok 691 - createdb: dumps CREATE DATABASE postgres
not ok 3003 - pg_dumpall_dbprivs: dumps CREATE DATABASE dump_test
not ok 11 - restore full dump using environment variables for connection 
parameters
not ok 12 - no dump errors
not ok 13 - restore full dump with command-line options for connection 
parameters
not ok 14 - no dump errors

= Code review

- As a response to "TBD -- is it necessary to get the default encoding": 
I think so, but either way changing this seems unrelated to this patch.

- I know it is taken from the old pg_dumpall code, but the way the 
database owner is handled seems I wrong.think we should set it like the 
owner for other objects. And more importantly it should respect --no-owner.

- The logic for switching database when setting the default table space 
is broken. You generate "\ connect" rather than "\connect".

- I saw the comment "Note that we do not support initial privileges 
(pg_init_privs) on databases." and wondered: why not? I definitly think 
that we should support this.

= Nitpicking

- You should probably use SGML style </> over </command> and 
</application> for inline tags.

- In "database-level properties such as Ownership, ACLs, [...]" I do not 
think that "Ownerships" shuld be capitalized.

- There are two extra spaces on the lines below, and a space is missing 
after the closing tag.

<command> ALTER ROLE IN DATABASE ...  SET </command>commands.

with --create option to dump <command> ALTER ROLE IN DATABASE ...  SET 
</command>

- On the following comment ".." should be "...", since that is the 
correct way to write an ellipsis.

* Frame the ALTER .. SET .. commands and fill it in buf.

- Rename arrayitem to configitem in makeAlterConfigCommand().

- In makeAlterConfigCommand() you should do "*pos++ = '\0';" rather than 
"*pos = 0;" and then remove the later + 1 so our code matches with the 
code in dumpFunc(). Either is correct, but it would be nice if both 
pieces of code looked more similar.

- You removed an empty line in pg_backup_utils.h between globals 
variables and function declartions which I think should be left there. 
It should be directly after g_verbose.

- There is something wrong with the indentation of the query for 
collecting info about databases in dumpDatabase() for PG >= 9.6.

- Missing space before "'' as rdatacl" in dumpDatabase(), and a missing 
space at the end of the string.

- Double space in 'FROM pg_database  "' in dumpDatabase().

Andreas

  6. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-29T03:43:58Z

    On Tue, Mar 28, 2017 at 12:50 AM, Andreas Karlsson <andreas@proxel.se>
wrote:

> Hi,
>
> Here is my review. I agree with the goal of the refactoring, as we want to
> make it easier to dump all the properties for the database object. But I
> think we need to solve the issues with the special casing of postgres and
> template1 which I personally would find very surprising if pg_dump -C did.
> On the other hand I think that we cannot get away from having pg_dumpall
> give them a special treatment.
>

Thanks for the review.

I added a new option --enable-pgdumpall-behaviour to get the pg_dumpall
behaviour for the database objects
while dumping them through pg_dump. I am open to change the option name if
we come up with any other
better name.


> The nitpicking section is for minor code style errors.
>
> = Functional review
>
> I have not done an in depth functional review due to the discussion about
> how postgres and template1 should be handled.
>
> - The patch does not apply cleanly anymore
>
> - I do not like the change in behavior which causes "pg_dump -C postgres"
> to no longer include CREATE DATABASE. Special treatment of specific
> databases based on name makes sense in pg_dumpall, but not in pg_dump.
>

With the new additional option, CREATE DATABASE commands for postgres and
special treatment of
"SET default_transaction_read_only = off" still held.

- There are test failures in the pg_dump tests. It seems like some could be
> related to that you do not include CREATE DATABASE postgres in the dumps
> but I also get errors like 'ERROR:  syntax error at or near
> "fault_tablespace"'.
>
> not ok 691 - createdb: dumps CREATE DATABASE postgres
> not ok 3003 - pg_dumpall_dbprivs: dumps CREATE DATABASE dump_test
> not ok 11 - restore full dump using environment variables for connection
> parameters
> not ok 12 - no dump errors
> not ok 13 - restore full dump with command-line options for connection
> parameters
> not ok 14 - no dump errors
>

Fixed. Now all tests pass.

= Code review
>
> - As a response to "TBD -- is it necessary to get the default encoding": I
> think so, but either way changing this seems unrelated to this patch.
>

Removed.


> - I know it is taken from the old pg_dumpall code, but the way the
> database owner is handled seems I wrong.think we should set it like the
> owner for other objects. And more importantly it should respect --no-owner.
>

Removed the code for owner, as it is handled in another place with ALTER
DATABASE
command.


> - The logic for switching database when setting the default table space is
> broken. You generate "\ connect" rather than "\connect".
>

Fixed.



> - I saw the comment "Note that we do not support initial privileges
> (pg_init_privs) on databases." and wondered: why not? I definitly think
> that we should support this.
>

This is the existing code that moved from pg_dumpall.

= Nitpicking
>
> - You should probably use SGML style </> over </command> and
> </application> for inline tags.
>

Corrected.


> - In "database-level properties such as Ownership, ACLs, [...]" I do not
> think that "Ownerships" shuld be capitalized.
>

Fixed.

- There are two extra spaces on the lines below, and a space is missing
> after the closing tag.
>
> <command> ALTER ROLE IN DATABASE ...  SET </command>commands.
>
> with --create option to dump <command> ALTER ROLE IN DATABASE ...  SET
> </command>
>

Fixed.


> - On the following comment ".." should be "...", since that is the correct
> way to write an ellipsis.
>
> * Frame the ALTER .. SET .. commands and fill it in buf.
>

Fixed.


> - Rename arrayitem to configitem in makeAlterConfigCommand().
>

Corrected.


> - In makeAlterConfigCommand() you should do "*pos++ = '\0';" rather than
> "*pos = 0;" and then remove the later + 1 so our code matches with the code
> in dumpFunc(). Either is correct, but it would be nice if both pieces of
> code looked more similar.
>

Corrected.


> - You removed an empty line in pg_backup_utils.h between globals variables
> and function declartions which I think should be left there. It should be
> directly after g_verbose.


Fixed.

- There is something wrong with the indentation of the query for collecting
> info about databases in dumpDatabase() for PG >= 9.6.
>

Fixed.

- Missing space before "'' as rdatacl" in dumpDatabase(), and a missing
> space at the end of the string.
>

Fixed.


> - Double space in 'FROM pg_database  "' in dumpDatabase().


Fixed.

Updated patch attached.


Regards,
Hari Babu
Fujitsu Australia

  7. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Andreas Karlsson <andreas@proxel.se> — 2017-03-29T12:04:38Z

    On 03/29/2017 05:43 AM, Haribabu Kommi wrote:
 > Updated patch attached.

I get a test failure in the pg_upgrade tests, but I do not have time 
right now to investigate.

The failing test is "Restoring database schemas in the new cluster".

I get the following in the log:

command: 
"/home/andreas/dev/postgresql/src/bin/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_dump" 
--host /home/andreas/dev/postgresql/src/bin/pg_upgrade --port 50848 
--username andreas --schema-only --quote-all-identifiers 
--binary-upgrade --format=custom  --file="pg_upgrade_dump_16385.custom" 
'dbname='"'"'./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ'"'"'' >> 
"pg_upgrade_dump_16385.log" 2>&1


command: 
"/home/andreas/dev/postgresql/src/bin/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_restore" 
--host /home/andreas/dev/postgresql/src/bin/pg_upgrade --port 50848 
--username andreas --exit-on-error --verbose --dbname 
'dbname='"'"'./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ'"'"'' 
"pg_upgrade_dump_16385.custom" >> "pg_upgrade_dump_16385.log" 2>&1
pg_restore: connecting to database for restore
pg_restore: [archiver (db)] connection to database 
"./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ" failed: FATAL:  database 
"./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ" does not exist

Andreas

  8. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-30T01:00:02Z

    On Wed, Mar 29, 2017 at 11:04 PM, Andreas Karlsson <andreas@proxel.se>
wrote:

> On 03/29/2017 05:43 AM, Haribabu Kommi wrote:
> > Updated patch attached.
>
> I get a test failure in the pg_upgrade tests, but I do not have time right
> now to investigate.
>
> The failing test is "Restoring database schemas in the new cluster".
>

Thanks for test.

I found the reason for failure.

Before this refactor patch, in case of --binary-upgrade, the pg_dumpall
dumps all the global objects and also the database objects. These objects
will be restored first during the preparation of the new cluster and later
each individual database is restored.

Because of the refactoring of the database objects, currently as part of
globals dump with --binary-upgrade, no database objects gets dumped.
During restore no databases are created. so while restoring individual
database, it leads to failure as it not able to connect to the target
database.

Currently I marked the patch in the commitfest as "returned with feedback"
as in the current situation, this needs some analysis in handling database
objects in --binary-upgrade mode.

Regards,
Hari Babu
Fujitsu Australia

  9. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-04-04T05:06:08Z

    On Thu, Mar 30, 2017 at 12:00 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
>
> On Wed, Mar 29, 2017 at 11:04 PM, Andreas Karlsson <andreas@proxel.se>
> wrote:
>
>> On 03/29/2017 05:43 AM, Haribabu Kommi wrote:
>> > Updated patch attached.
>>
>> I get a test failure in the pg_upgrade tests, but I do not have time
>> right now to investigate.
>>
>> The failing test is "Restoring database schemas in the new cluster".
>>
>
> Thanks for test.
>
> I found the reason for failure.
>
> Before this refactor patch, in case of --binary-upgrade, the pg_dumpall
> dumps all the global objects and also the database objects. These objects
> will be restored first during the preparation of the new cluster and later
> each individual database is restored.
>
> Because of the refactoring of the database objects, currently as part of
> globals dump with --binary-upgrade, no database objects gets dumped.
> During restore no databases are created. so while restoring individual
> database, it leads to failure as it not able to connect to the target
> database.
>

I modified the pg_upgrade code to use template1 database as a connecting
database while restoring the dump along with --create option to pg_restore
to create the database objects instead of connecting to the each individual
database.

And also while dumping the database objects, passed the new option of
--enable-pgdumpall-behaviour to pg_dump to dump the database objects
as it expected dump during pg_dumpall --binary-upgrade.

Both pg_dump and pg_upgrade tests are passed. Updated patch attached
I will add this patch to the next commitfest.

Regards,
Hari Babu
Fujitsu Australia

  10. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-14T21:29:19Z

    On 4/4/17 01:06, Haribabu Kommi wrote:
> Both pg_dump and pg_upgrade tests are passed. Updated patch attached
> I will add this patch to the next commitfest.

This patch needs to be rebased for the upcoming commit fest.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-08-21T04:35:13Z

    On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> On 4/4/17 01:06, Haribabu Kommi wrote:
> > Both pg_dump and pg_upgrade tests are passed. Updated patch attached
> > I will add this patch to the next commitfest.
>
> This patch needs to be rebased for the upcoming commit fest.
>

Thanks for checking. Rebased patch is attached.

Regards,
Hari Babu
Fujitsu Australia

  12. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-08T00:24:17Z

    On Mon, Aug 21, 2017 at 4:35 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 4/4/17 01:06, Haribabu Kommi wrote:
>> > Both pg_dump and pg_upgrade tests are passed. Updated patch attached
>> > I will add this patch to the next commitfest.
>>
>> This patch needs to be rebased for the upcoming commit fest.
>
> Thanks for checking. Rebased patch is attached.

Hi Haribabu,

This patch breaks the documentation build, possibly because of these empty tags:

+        <option>--create</option> option to dump <>ALTER ROLE IN
DATABASE ... SET</>

-- 
Thomas Munro
http://www.enterprisedb.com

  13. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-09-09T03:29:45Z

    On Fri, Sep 8, 2017 at 10:24 AM, Thomas Munro <thomas.munro@enterprisedb.com
> wrote:

> On Mon, Aug 21, 2017 at 4:35 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> wrote:
> >> On 4/4/17 01:06, Haribabu Kommi wrote:
> >> > Both pg_dump and pg_upgrade tests are passed. Updated patch attached
> >> > I will add this patch to the next commitfest.
> >>
> >> This patch needs to be rebased for the upcoming commit fest.
> >
> > Thanks for checking. Rebased patch is attached.
>
> Hi Haribabu,
>
> This patch breaks the documentation build, possibly because of these empty
> tags:
>
> +        <option>--create</option> option to dump <>ALTER ROLE IN
> DATABASE ... SET</>
>

Thanks for checking the patch.
Fixed patch is attached.


Regards,
Hari Babu
Fujitsu Australia

  14. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-09-29T04:44:33Z

    Hi,


On Sat, Sep 9, 2017 at 1:29 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
> Fixed patch is attached.
>


Patch applies and has lot of noise due to indent with spaces.
I did ran regression tests located in - src/test/regress,
src/test/modules/test_pg_dump, src/bin/pg_dump, src/bin/pg_upgrade folders
and no issues observed.



+      <term><option>--enable-pgdumpall-behaviour</option></term>
+      <listitem>
+       <para>
+        This option is for the use of <command>pg_dumpall</command> or
+        <command>pg_upgrade</command> utility to dump the database objects
+        by <application>pg_dump</application> for a complete dump.
+        This option can only be used with <option>-C/--create</option>.
+        Its use for other ourposes is not recommended or supported.
+        The behavior of the option may change in future releases without
notice.
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>

s/ourposes/purposes/


Option name "--enable-pgdumpall-behaviour"  is very generic and it is
better to rename it to something that reflects its functionality like
--skip-default-db-create/--no-default-db-create

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  15. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2017-09-29T17:31:16Z

    On Fri, Sep 29, 2017 at 12:44 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:
> Option name "--enable-pgdumpall-behaviour"  is very generic

Yeah, that's a terrible name, at least in my opinion.

> and it is better
> to rename it to something that reflects its functionality like
> --skip-default-db-create/--no-default-db-create

But I wonder why this patch needs a new option at all?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-10-04T07:40:21Z

    On Sat, Sep 30, 2017 at 3:31 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Fri, Sep 29, 2017 at 12:44 AM, Vaishnavi Prabakaran
> <vaishnaviprabakaran@gmail.com> wrote:
> > Option name "--enable-pgdumpall-behaviour"  is very generic
>
> Yeah, that's a terrible name, at least in my opinion.
>

OK. I will use a new name based on the discussion.


> and it is better
> > to rename it to something that reflects its functionality like
> > --skip-default-db-create/--no-default-db-create
>
> But I wonder why this patch needs a new option at all?
>

There are some differences in handling database objects
between pg_dump and pg_dumpall, To retain both pg_dump
and pg_dumpall behavior even after refactoring, this option
is added. Currently this option is used mainly for the three
purposes.

1. Don't print unnecessary CREATE DATABASE options like
ENCODING, LC_COLLATE and LC_CTYPE options if the
default encoding is same with the above values.

The above behavior is as per the pg_dumpall, but it can be
changed to print irrespective of the default encoding.

2. Do not dump postgres and template0 databases.

3. Set default_transaction_read_only = off.

As per the following comment in pg_dumpall, based on that flag
the GUC is set, to retain the same behavior even after this
refactoring.

/*
* Restore will need to write to the target cluster.  This connection
* setting is emitted for pg_dumpall rather than in the code also used
* by pg_dump, so that a cluster with databases or users which have
* this flag turned on can still be replicated through pg_dumpall
* without editing the file or stream.  With pg_dump there are many
* other ways to allow the file to be used, and leaving it out allows
         * users to protect databases from being accidental restore targets.
*/
fprintf(OPF, "SET default_transaction_read_only = off;\n\n");

we can remove the usage -1 and retain the usage-2 with modified option
name as --no-default-database or similar.

Any opinions about the usage-3, In case if we need to retain that change,
any best solution to the option name?

Regards,
Hari Babu
Fujitsu Australia

  17. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2017-10-05T13:29:27Z

    On Wed, Oct 4, 2017 at 3:40 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> There are some differences in handling database objects
> between pg_dump and pg_dumpall, To retain both pg_dump
> and pg_dumpall behavior even after refactoring, this option
> is added. Currently this option is used mainly for the three
> purposes.
>
> 1. Don't print unnecessary CREATE DATABASE options like
> ENCODING, LC_COLLATE and LC_CTYPE options if the
> default encoding is same with the above values.
>
> The above behavior is as per the pg_dumpall, but it can be
> changed to print irrespective of the default encoding.
>
> 2. Do not dump postgres and template0 databases.
>
> 3. Set default_transaction_read_only = off.

To me it seems that a refactoring which requires pg_dump to behave
differently in small ways like this based on whether it is being
called by pg_dumpall or not is probably not a good refactoring.  And I
don't see why the proposal from Tom that started this thread would
require such a thing to be true.

From your list, I would say that (1) and (3) seem like behaviors that
we either want or do not want.  Whether pg_dump or pg_dumpall is
involved seems irrelevant.  (2) seems like it might need some special
handling, but that could be handled in pg_dumpall by just not calling
pg_dump at all for those database.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-10-21T05:30:38Z

    On Fri, Oct 6, 2017 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Wed, Oct 4, 2017 at 3:40 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
> > There are some differences in handling database objects
> > between pg_dump and pg_dumpall, To retain both pg_dump
> > and pg_dumpall behavior even after refactoring, this option
> > is added. Currently this option is used mainly for the three
> > purposes.
> >
> > 1. Don't print unnecessary CREATE DATABASE options like
> > ENCODING, LC_COLLATE and LC_CTYPE options if the
> > default encoding is same with the above values.
> >
> > The above behavior is as per the pg_dumpall, but it can be
> > changed to print irrespective of the default encoding.
> >
> > 2. Do not dump postgres and template0 databases.
> >
> > 3. Set default_transaction_read_only = off.
>
> To me it seems that a refactoring which requires pg_dump to behave
> differently in small ways like this based on whether it is being
> called by pg_dumpall or not is probably not a good refactoring.  And I
> don't see why the proposal from Tom that started this thread would
> require such a thing to be true.
>

Before refactoring, pg_dumpall doesn't print "create database" commands
for both tempalte1 and postgres database, but on the other hand pg_dump
dump the create database commands with --create option.

To keep the behavior of all the database attributes in the dump of both
pg_dump and pg_dumpall, the code is unified and moved into pg_dump.
But to retain the pg_dumpall behavior of not dumping the "create database"
commands, a new option is added to pg_dump to skip dumping the
create database commands.

The new option name is now "--skip-create-default-db", this can be used
normal user also when try to dump the postgres database to not let create
the database commands in the dump.

From your list, I would say that (1) and (3) seem like behaviors that
> we either want or do not want.  Whether pg_dump or pg_dumpall is
> involved seems irrelevant.  (2) seems like it might need some special
> handling, but that could be handled in pg_dumpall by just not calling
> pg_dump at all for those database.



I didn't any better way other than creating a new option to not let the
default db create database commands to dump, so I renamed the
older option to better one and change the behavior to use by the
normal users also.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

  19. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2017-10-21T16:08:33Z

    On Sat, Oct 21, 2017 at 1:30 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> Before refactoring, pg_dumpall doesn't print "create database" commands
> for both tempalte1 and postgres database, but on the other hand pg_dump
> dump the create database commands with --create option.
>
> To keep the behavior of all the database attributes in the dump of both
> pg_dump and pg_dumpall, the code is unified and moved into pg_dump.
> But to retain the pg_dumpall behavior of not dumping the "create database"
> commands, a new option is added to pg_dump to skip dumping the
> create database commands.
>
> The new option name is now "--skip-create-default-db", this can be used
> normal user also when try to dump the postgres database to not let create
> the database commands in the dump.

I don't get this at all.  If you don't want to create the database,
just don't pass the -C argument.  It doesn't make sense to have a -C
argument which makes it create the database and then a
--skip-create-default-db argument which makes it sometimes not create
the database after all.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  20. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-10-23T05:36:34Z

    On Sun, Oct 22, 2017 at 3:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Sat, Oct 21, 2017 at 1:30 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > Before refactoring, pg_dumpall doesn't print "create database" commands
> > for both tempalte1 and postgres database, but on the other hand pg_dump
> > dump the create database commands with --create option.
> >
> > To keep the behavior of all the database attributes in the dump of both
> > pg_dump and pg_dumpall, the code is unified and moved into pg_dump.
> > But to retain the pg_dumpall behavior of not dumping the "create
> database"
> > commands, a new option is added to pg_dump to skip dumping the
> > create database commands.
> >
> > The new option name is now "--skip-create-default-db", this can be used
> > normal user also when try to dump the postgres database to not let create
> > the database commands in the dump.
>
> I don't get this at all.  If you don't want to create the database,
> just don't pass the -C argument.  It doesn't make sense to have a -C
> argument which makes it create the database and then a
> --skip-create-default-db argument which makes it sometimes not create
> the database after all.


Apologies for not providing much details.

pg_dumpall is used to produce the following statements for database,

"Create database" (other than default database) or
"Alter database set tablespace" for default database (if required)

ACL queries related to database
Alter database config
Alter database role config

whereas, pg_dump used to produce only "create database statement".

With the refactoring, all the pg_dumpall database statements are moved
into pg_dump. -C/--create option of pg_dump produces all the statements
of pg_dumpall. The --skip-default-create-db option is to make sure that
it doesn't produce "Create database" statement and instead may produce
"Alter database set tablespace" for default databases of (postgres and
template1).

-C/--create option is to control the entire database statements.
--skip-create-default-db is option to control the "create" or "Alter"
database statement
for default database.

During restore the dump, the -C/--create restores all the Database
statements.

comments? or any better approach?

Regards,
Hari Babu
Fujitsu Australia

  21. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2017-10-26T11:01:32Z

    On Mon, Oct 23, 2017 at 7:36 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> Apologies for not providing much details.
>
> pg_dumpall is used to produce the following statements for database,
>
> "Create database" (other than default database) or
> "Alter database set tablespace" for default database (if required)
>
> ACL queries related to database
> Alter database config
> Alter database role config
>
> whereas, pg_dump used to produce only "create database statement".

How about adding a new flag --set-db-properties that doesn't produce
CREATE DATABASE but does dump the other stuff?  -C would dump both
CREATE DATABASE *and* the other stuff.  Then you could dump built-in
databases with --set-db-properties and others with -C.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  22. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-11-07T09:35:40Z

    On Thu, Oct 26, 2017 at 10:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Oct 23, 2017 at 7:36 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > Apologies for not providing much details.
> >
> > pg_dumpall is used to produce the following statements for database,
> >
> > "Create database" (other than default database) or
> > "Alter database set tablespace" for default database (if required)
> >
> > ACL queries related to database
> > Alter database config
> > Alter database role config
> >
> > whereas, pg_dump used to produce only "create database statement".
>
> How about adding a new flag --set-db-properties that doesn't produce
> CREATE DATABASE but does dump the other stuff?  -C would dump both
> CREATE DATABASE *and* the other stuff.  Then you could dump built-in
> databases with --set-db-properties and others with -C.


Thanks for the idea, Here I attached the patch that implements the same.

The newly added option is not recommended to be used in normal cases and
it is used only for upgrade utilities.

In case if user issues pg_dump with --set-db-properties option along with
--create
or --clean options, an error is raised. Currently there is no way to throw
an error
in case if the dump is generated with --set-db-properties and try to
restore with
--clean option. To avoid this change, we may need to add additional details
in the
archive handler, but is it really needed?

Regards,
Hari Babu
Fujitsu Australia

  23. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2017-11-07T21:48:16Z

    On Tue, Nov 7, 2017 at 4:35 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> The newly added option is not recommended to be used in normal cases and
> it is used only for upgrade utilities.

I don't know why it couldn't be used in normal cases.  That seems like
a totally legitimate thing for somebody to want.  Maybe nobody does,
but I see no reason to worry if they do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  24. Re: Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-11-07T23:50:57Z

    On Wed, Nov 8, 2017 at 8:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Tue, Nov 7, 2017 at 4:35 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
> > The newly added option is not recommended to be used in normal cases and
> > it is used only for upgrade utilities.
>
> I don't know why it couldn't be used in normal cases.  That seems like
> a totally legitimate thing for somebody to want.  Maybe nobody does,
> but I see no reason to worry if they do.


Ok. Removed the documentation changes that it cannot be used for normal
scenarios, and also added a Note section explaining the problem of using
the dump with pg_restore command with --clean and --create options.

Regards,
Hari Babu
Fujitsu Australia

  25. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-29T05:25:17Z

    On Wed, Nov 8, 2017 at 8:50 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> Ok. Removed the documentation changes that it cannot be used for normal
> scenarios, and also added a Note section explaining the problem of using
> the dump with pg_restore command with --clean and --create options.

Hari, the documentation portion of the patch does not apply. Could you
rebase? For now I am moving it to next CF as this did not get any
reviews, and the status is switched to "waiting on author".
-- 
Michael

  26. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-12-13T02:50:12Z

    On Wed, Nov 29, 2017 at 4:25 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Wed, Nov 8, 2017 at 8:50 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
> > Ok. Removed the documentation changes that it cannot be used for normal
> > scenarios, and also added a Note section explaining the problem of using
> > the dump with pg_restore command with --clean and --create options.
>
> Hari, the documentation portion of the patch does not apply. Could you
> rebase? For now I am moving it to next CF as this did not get any
> reviews, and the status is switched to "waiting on author".
>

Rebased patch attached that fixes the documentation build problem.

Regards,
Hari Babu
Fujitsu Australia

  27. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2018-01-05T03:54:17Z

    On Wed, Dec 13, 2017 at 1:50 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

> Rebased patch attached that fixes the documentation build problem.
>
> Latest patch applies without noise. And here are some minor comments

+     <varlistentry>
+      <term><option>--set-db-properties</option></term>
+      <listitem>
+       <para>
+        This option is to skip create database command without ignoring
the rest
+        of the DATABASE specific commands and it is used when the database
already
+        exists in the restore location. This option cannot be used with
either of
+        <option>-C/--create</option> or <option>-c/--clean</option>
options.
+       </para>

I see the description is framed based on previous flag "
--skip-default-create-db" , and I think this needs to be re-phrased to
describe what it does , e.g: This option is to set database properties for
already existing database without issuing create database command ....".


+ printf(_("  --set-db-properties          dump db properties, for use by
upgrade utilities only\n"));
After the discussion about its usage up-thread, this needs to be corrected.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  28. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-01-05T05:32:05Z

    On Fri, Jan 5, 2018 at 2:54 PM, Vaishnavi Prabakaran <
vaishnaviprabakaran@gmail.com> wrote:

>
> On Wed, Dec 13, 2017 at 1:50 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>
>> Rebased patch attached that fixes the documentation build problem.
>>
>> Latest patch applies without noise. And here are some minor comments
>

Thanks for the review.


> +     <varlistentry>
> +      <term><option>--set-db-properties</option></term>
> +      <listitem>
> +       <para>
> +        This option is to skip create database command without ignoring
> the rest
> +        of the DATABASE specific commands and it is used when the
> database already
> +        exists in the restore location. This option cannot be used with
> either of
> +        <option>-C/--create</option> or <option>-c/--clean</option>
> options.
> +       </para>
>
> I see the description is framed based on previous flag "
> --skip-default-create-db" , and I think this needs to be re-phrased to
> describe what it does , e.g: This option is to set database properties for
> already existing database without issuing create database command ....".
>

Updated with more details.


+ printf(_("  --set-db-properties          dump db properties, for use by
> upgrade utilities only\n"));
> After the discussion about its usage up-thread, this needs to be corrected.
>

Corrected.
Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

  29. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2018-01-05T06:00:23Z

    On Fri, Jan 5, 2018 at 4:32 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:
>
>
> Corrected.
> Updated patch attached.
>
> Moved this CF item to "Ready for committer"

Regards,
Vaishnavi,
Fujitsu Australia.

  30. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-17T21:25:15Z

    Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> [ pg_dump-and-pg_dumpall-database-handling-refactoring_v12.patch ]

I started to look through this, and almost immediately found that the
diff in t/002_pg_dump.pl doesn't seem necessary --- the test passes
for me without applying that hunk.  Is that a leftover from a previous
patch iteration, or is there some platform dependency in the test?
If it's not necessary, I'd be inclined to leave it as it was.

			regards, tom lane

  31. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-01-17T23:32:17Z

    On Thu, Jan 18, 2018 at 8:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> > [ pg_dump-and-pg_dumpall-database-handling-refactoring_v12.patch ]
>
> I started to look through this, and almost immediately found that the
> diff in t/002_pg_dump.pl doesn't seem necessary --- the test passes
> for me without applying that hunk.  Is that a leftover from a previous
> patch iteration, or is there some platform dependency in the test?
> If it's not necessary, I'd be inclined to leave it as it was.
>

Thanks for the review.

Yes, it is a left over from previous patch iteration. The test used to fail
with this
patch earlier. Now there is no problem in test even after removing the hunk
from
my side also. There is no platform dependency.

Attached is an updated patch after removing the test changes.

Regards,
Hari Babu
Fujitsu Australia

  32. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-17T23:50:32Z

    Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> [ pg_dump-and-pg_dumpall-database-handling-refactoring_v12.patch ]

I've gone through this in a once-over-lightly fashion.  Since there was
quite a bit of debate upthread about how things should work, I'd like
to start by summarizing the decisions this patch has made, in case
anyone still wants to object to them:

* pg_dump will now be responsible for dumping all per-database properties,
including "ALTER ROLE IN DATABASE SET" settings.  To get that behavior,
pg_dumpall will invoke pg_dump using -C, or using the new switch
--set-db-properties when working on "template1" or "postgres" databases,
since those should already exist in the target installation.

* pg_dumpall still won't dump template0 (or any other not-datallowconn
database).  This means that it will no longer propagate any non-default
per-database properties for such databases.  I think this is fine for
template0: if you've messed with that at all, you are fooling with non
user-serviceable parts.  It's a bit less good for other DBs maybe, but on
the whole it seems more consistent to simply treat nonconnectable DBs
as not existing.  (That could stand to be documented though.)

* "pg_dumpall -g" will now produce only role- and tablespace-related output.

* There isn't any direct way to ask pg_dump for "just the DB properties
and nothing else" (although I suppose you can mostly fake it by adding
"--schema nosuchschema" or some such).  This makes it inconvenient to
exactly duplicate the old behavior of "pg_dumpall -g", if someone wanted
to do that to fit into their existing backup procedures.  I'm not sure
how important that is.  Personally I'm content to lose it, but if there's
enough pushback maybe we could invent a "--db-properties-only" switch?

There are some other points that haven't been debated:

* As the patch stands, --set-db-properties is implicit when you specify
-C, and in fact the patch goes to the trouble of throwing an error if you
try to specify both switches.  I'm inclined to think this might be a bad
idea.  What about saying that -C enables emitting CREATE DATABASE and
reconnecting to that DB, and independently of that, --set-db-properties
enables emitting the additional per-database properties?  This seems
simpler to understand, more flexible, and less of a change from the
previous behavior of -C.  On the other hand you could argue that people
would always want --set-db-properties with -C and so we're merely
promoting carpal tunnel (and errors of omission) if we do it like that.
If so, maybe we could say "-C implies --set-db-properties by default, but
if you really don't want that, you can say -C --no-set-db-properties".
Perhaps the only application of this is to reproduce pg_dump's historical
behavior, but that's probably of some value.

* The patch fails to make any provision for deciding at pg_restore time
whether to emit DB properties.  Considering that you can use -C at restore
time, I think it's pretty awful that you can't use --set-db-properties
then.  Moreover, not only has the patch not added a separate "DATABASE
PROPERTIES" TOC entry type as was originally proposed, what it's actually
doing is emitting two identically labeled "DATABASE" TOC entries, one
with the CREATE and the other with the rest.  That's simply horrid.
I think we need to do this properly and emit two distinguishable TOC
entries, and control which of them get printed on the restore side of the
logic, not the TOC entry construction side.  (I'm not sure at this point
if we need an archive version bump to do that, but perhaps not --- in the
past we've added new TOC types without a version bump.)

* Some years ago, commit 4bd371f6f caused pg_dumpall to emit "SET
default_transaction_read_only = off;" in its scripts, so that it could
successfully handle DBs that have "SET default_transaction_read_only = on"
as a database property.  This was intentionally NOT done to pg_dump,
arguing that doing so would greatly weaken such a setting as a defense
against stupid errors (i.e. restoring into your production database).
The patch currently ignores that reasoning and moves the setting into
pg_dump anyway.  Haribabu mentioned this point upthread and got basically
no response, but I'm concerned about it.

I think however that we might be able to dodge that issue as a byproduct
of fixing the previous problem.  If the CREATE DATABASE is done as one
TOC entry, and we reconnect to the target DB after processing that entry,
and then we process the DATABASE PROPERTIES entry, then any DB property
settings we've installed will not apply in our existing session.  So I
think we can just leave out the "SET default_transaction_read_only = on"
altogether.  Obviously this puts a premium on not reconnecting, but
reconnecting would break other more important features like
--single-transaction, so I am not worried about that.

* An issue in connection with that is that because you can't issue
ALTER DATABASE SET TABLESPACE against the current DB, pg_dumpall
currently takes the trouble to explicitly reconnect to a different DB
and then back to the target DB when issuing that command.  That doesn't
play nice with my suggestion above, nor with --single-transaction.
I'm still thinking about how to fix that, but probably the best fix
involves handling non-default tablespaces by including them in the
initial CREATE DATABASE command, plus emitting an ALTER DATABASE SET
TABLESPACE in a separate new "DATABASE TABLESPACE" TOC object, which
we would have to teach the pg_restore logic to handle correctly.
At minimum it'd need to be aware about the interaction with
--single-transaction; since we have to have that anyway, we could also
make it do the extra-reconnections dance instead of hard-wiring \connect
commands into the text of the entry, and/or skip the whole thing if it
knows it emitted the CREATE DATABASE command.

Comments?  If there's not objections I plan to push forward on this.

			regards, tom lane

  33. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2018-01-18T21:34:31Z

    On Wed, Jan 17, 2018 at 6:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * As the patch stands, --set-db-properties is implicit when you specify
> -C, and in fact the patch goes to the trouble of throwing an error if you
> try to specify both switches.  I'm inclined to think this might be a bad
> idea.  What about saying that -C enables emitting CREATE DATABASE and
> reconnecting to that DB, and independently of that, --set-db-properties
> enables emitting the additional per-database properties?  This seems
> simpler to understand, more flexible, and less of a change from the
> previous behavior of -C.  On the other hand you could argue that people
> would always want --set-db-properties with -C and so we're merely
> promoting carpal tunnel (and errors of omission) if we do it like that.

I would vigorously agree with that latter argument.  I think the
chances of errors of omission would be very high even if the carpal
tunnel dangers were ameliorated by giving --set-db-properties a short
option name.

> If so, maybe we could say "-C implies --set-db-properties by default, but
> if you really don't want that, you can say -C --no-set-db-properties".

That seems like a better idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  34. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-18T22:09:19Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jan 17, 2018 at 6:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * As the patch stands, --set-db-properties is implicit when you specify
>> -C, and in fact the patch goes to the trouble of throwing an error if you
>> try to specify both switches.  I'm inclined to think this might be a bad
>> idea.  What about saying that -C enables emitting CREATE DATABASE and
>> reconnecting to that DB, and independently of that, --set-db-properties
>> enables emitting the additional per-database properties?  This seems
>> simpler to understand, more flexible, and less of a change from the
>> previous behavior of -C.  On the other hand you could argue that people
>> would always want --set-db-properties with -C and so we're merely
>> promoting carpal tunnel (and errors of omission) if we do it like that.

> I would vigorously agree with that latter argument.  I think the
> chances of errors of omission would be very high even if the carpal
> tunnel dangers were ameliorated by giving --set-db-properties a short
> option name.

Fair enough.  We'll keep the behavioral change then.

>> If so, maybe we could say "-C implies --set-db-properties by default, but
>> if you really don't want that, you can say -C --no-set-db-properties".

> That seems like a better idea.

What I think I'll do for the moment is make them independent options so
far as the implementation is concerned, but have the command line switch
processing do

	/* --create implies --set-db-properties, for now anyway */
	if (dopt.outputCreateDB)
		dopt.set_db_properties = 1;

If somebody actually asks for --no-set-db-properties, we can add that
later.

			regards, tom lane

  35. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-18T23:15:09Z

    ... okay, so the reason why --set-db-properties isn't as general-purpose
a switch as you might think finally penetrated my thick skull :-(

The problem is that, as the patch is currently constituted, that will
result in emitting a lot of "ALTER DATABASE foo" type commands.  If
somebody tries to load the pg_dump output into a database not named foo,
mayhem ensues.  This is exactly the same problem people have already
noted with respect to COMMENT ON DATABASE, but now we're propagating it to
other operations that have (much) higher downsides for getting it wrong.

What we need, therefore, is ALTER DATABASE CURRENT_DATABASE, which
I see is part of the pending patch for fixing the COMMENT ON DATABASE
problem.  That one is stuck in Waiting on Author, but I'm inclined to
go see if I can push it across the finish line, and then come back
to this one.

Even with that, there's a small problem: the backend cannot reasonably
support ALTER DATABASE CURRENT_DATABASE SET TABLESPACE, except perhaps
in the no-op case where the specified tablespace is already the right
one.  So this puts a serious hole in my thoughts about allowing the
tablespace to be adjusted during --set-db-properties without --create.

What I think we should do for the time being is to have pg_dump treat
database tablespace as a property it can't adjust after creation, just
as it can't adjust locale or encoding.  That's a loss of functionality
for pg_dumpall/pg_upgrade compared to where we are today, in that if
you've set up the template1 or postgres DBs with nondefault tablespace
then that won't propagate to the new cluster.  But the same can already
be said about their locale and encoding, and I find it hard to believe
that many people are trying to give those two DBs tablespace settings
different from the cluster default, anyway.

The only way around that that I can see is to give pg_dump some
additional switch along the lines of --i-promise-i-wont-restore-
this-into-a-database-with-a-different-name, and then it could emit
the same sort of
	\connect template1
	alter database postgres set tablespace ...;
	\connect postgres
dance that pg_dumpall is using today.  But that sort of ugliness
is exactly what we said we didn't want in this patch.

			regards, tom lane

  36. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-18T23:35:41Z

    I wrote:
> What I think we should do for the time being is to have pg_dump treat
> database tablespace as a property it can't adjust after creation, just
> as it can't adjust locale or encoding.  That's a loss of functionality
> for pg_dumpall/pg_upgrade compared to where we are today, in that if
> you've set up the template1 or postgres DBs with nondefault tablespace
> then that won't propagate to the new cluster.  But the same can already
> be said about their locale and encoding, and I find it hard to believe
> that many people are trying to give those two DBs tablespace settings
> different from the cluster default, anyway.

Hm ... actually, there is more than one way to skin this cat.

Let me offer a modest proposal: pg_dumpall/pg_upgrade should simply DROP
postgres and template1 in the target cluster, and then re-create them
(from template0 of course).  With that, we'd not only cope with preserving
their tablespace settings, but we'd gain the ability to preserve their
locale and encoding, even if the target cluster had been initialized with
some other default.

If we did it like that, the rationale for an actual --set-db-properties
switch would vanish, at least so far as pg_dumpall is concerned -- we
could just make all that behavior an integral part of --create.  And
this wouldn't need to be conditional on getting ALTER DATABASE
CURRENT_DATABASE done.

Comments?

			regards, tom lane

  37. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-01-19T00:09:46Z

    On Fri, Jan 19, 2018 at 10:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > What I think we should do for the time being is to have pg_dump treat
> > database tablespace as a property it can't adjust after creation, just
> > as it can't adjust locale or encoding.  That's a loss of functionality
> > for pg_dumpall/pg_upgrade compared to where we are today, in that if
> > you've set up the template1 or postgres DBs with nondefault tablespace
> > then that won't propagate to the new cluster.  But the same can already
> > be said about their locale and encoding, and I find it hard to believe
> > that many people are trying to give those two DBs tablespace settings
> > different from the cluster default, anyway.
>
> Hm ... actually, there is more than one way to skin this cat.
>
> Let me offer a modest proposal: pg_dumpall/pg_upgrade should simply DROP
> postgres and template1 in the target cluster, and then re-create them
> (from template0 of course).  With that, we'd not only cope with preserving
> their tablespace settings, but we'd gain the ability to preserve their
> locale and encoding, even if the target cluster had been initialized with
> some other default.
>

Yes, I agree that this may be simple change to handle this problem.
Already pg_upgrade doesn't work if there is any encoding difference
between source and target databases. Most probably User will create
with same encoding.

Regards,
Hari Babu
Fujitsu Australia

  38. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2018-01-19T14:28:20Z

    On Thu, Jan 18, 2018 at 6:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> If we did it like that, the rationale for an actual --set-db-properties
> switch would vanish, at least so far as pg_dumpall is concerned -- we
> could just make all that behavior an integral part of --create.  And
> this wouldn't need to be conditional on getting ALTER DATABASE
> CURRENT_DATABASE done.

Unfortunately, I have a feeling that --set-db-properties might not be
the only thing that would vanish.  I think users are accustomed by now
to the idea that if you restore into an existing database, the
existing contents are preserved and the new stuff from the dump is
added (possibly with some errors and messiness).  With this design,
the existing database contents will instead vanish, and that is
probably going to make somebody unhappy.

I agree with you that making ALTER DATABASE accept CURRENT_DATABASE is
a good idea.  I don't have a great idea what to do about the SET
TABLESPACE problem.  It's always seemed to me to be sort of weird that
you have to have a database in order to create, drop, etc. another
database, or even get a list of databases that exist.  As a new user,
I remember being quite frustrated that connecting to a database that
didn't exist gave no hint of how to find out what databases did exist.
Eventually I discovered psql -l and all was well, but I had no idea
how it worked under the hood.  Even though I now understand the
architectural considerations that have gotten us to where we are, I
still think it would be more intuitive to users if there were a
command-shell unrelated to any database that would let you perform
operations on databases.  I realize, however, that this patch isn't
going to create such a thing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  39. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-19T14:45:41Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jan 18, 2018 at 6:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If we did it like that, the rationale for an actual --set-db-properties
>> switch would vanish, at least so far as pg_dumpall is concerned -- we
>> could just make all that behavior an integral part of --create.  And
>> this wouldn't need to be conditional on getting ALTER DATABASE
>> CURRENT_DATABASE done.

> Unfortunately, I have a feeling that --set-db-properties might not be
> the only thing that would vanish.  I think users are accustomed by now
> to the idea that if you restore into an existing database, the
> existing contents are preserved and the new stuff from the dump is
> added (possibly with some errors and messiness).  With this design,
> the existing database contents will instead vanish, and that is
> probably going to make somebody unhappy.

Well, we could say that the properties of template1 and postgres
are only restored if you use --clean.

			regards, tom lane

  40. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2018-01-19T14:52:00Z

    On Fri, Jan 19, 2018 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Jan 18, 2018 at 6:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> If we did it like that, the rationale for an actual --set-db-properties
>>> switch would vanish, at least so far as pg_dumpall is concerned -- we
>>> could just make all that behavior an integral part of --create.  And
>>> this wouldn't need to be conditional on getting ALTER DATABASE
>>> CURRENT_DATABASE done.
>
>> Unfortunately, I have a feeling that --set-db-properties might not be
>> the only thing that would vanish.  I think users are accustomed by now
>> to the idea that if you restore into an existing database, the
>> existing contents are preserved and the new stuff from the dump is
>> added (possibly with some errors and messiness).  With this design,
>> the existing database contents will instead vanish, and that is
>> probably going to make somebody unhappy.
>
> Well, we could say that the properties of template1 and postgres
> are only restored if you use --clean.

True.  Would that be a POLA violation, do you think?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  41. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-19T15:00:00Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jan 19, 2018 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, we could say that the properties of template1 and postgres
>> are only restored if you use --clean.

> True.  Would that be a POLA violation, do you think?

It seems a bit non-orthogonal.  Also, while your point that people
expect "merge" behavior from pg_dump is certainly true, I'm not
convinced that anybody would be relying on that for pg_dumpall.

			regards, tom lane

  42. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2018-01-19T15:13:48Z

    On Fri, Jan 19, 2018 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Jan 19, 2018 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Well, we could say that the properties of template1 and postgres
>>> are only restored if you use --clean.
>
>> True.  Would that be a POLA violation, do you think?
>
> It seems a bit non-orthogonal.  Also, while your point that people
> expect "merge" behavior from pg_dump is certainly true, I'm not
> convinced that anybody would be relying on that for pg_dumpall.

What I expect typically happens is that someone restores into an
existing cluster and then realizes their mistake and goes and removes
all of the extra stuff that got created.  This is a bit like when you
run tar -zxf expecting it to create a subdirectory, but it turns out
to extract in the current directory instead.  You then curse your fate
and remove all of the files it created.  It's annoying, but not that
bad.  If it nuked the contents of the currently directory first,
though, you'd be much sadder.  If somebody changed tar to have that
behavior, people would probably *eventually* become aware of the risk
and adjust their behavior to avoid catastrophe, but it would probably
take 1-2 disasters per individual before they got used to the new way,
and that's a lot of disasters considering how many people use tar.
Or, maybe people wouldn't get used to it and they'd just go after
whoever made the change with pitchforks.  Anyway, that kind of thing
seems like a danger here.

The other possibility that comes to mind is that somebody might be
doing this is working around a failure of something like CREATE
LANGUAGE.  Say the name of the .so has changed, so CREATE LANGUAGE
will fail or do the wrong thing.  Instead of editing the dump, they
pre-install the language with the correct definition and then restore
over it, ignoring errors.  I guess that would only work with pg_dump,
not pg_dumpall, unless the database that had the problem was one of
the pre-created ones.  Still, it's worth thinking over these kinds of
scenarios, I think.  People do a lot of ugly things in the real world
that we as developers would never do, mostly to work around the
problems we fail to foresee.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  43. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-19T16:28:16Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jan 19, 2018 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Fri, Jan 19, 2018 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Well, we could say that the properties of template1 and postgres
>>>> are only restored if you use --clean.

>>> True.  Would that be a POLA violation, do you think?

>> It seems a bit non-orthogonal.  Also, while your point that people
>> expect "merge" behavior from pg_dump is certainly true, I'm not
>> convinced that anybody would be relying on that for pg_dumpall.

> [ assorted examples ]
> Still, it's worth thinking over these kinds of
> scenarios, I think.  People do a lot of ugly things in the real world
> that we as developers would never do, mostly to work around the
> problems we fail to foresee.

Unless someone has a better idea, I'll go with the semantics stated
above: DB-level properties of the two standard databases are only
transferred to pg_dumpall's target cluster if you authorize dropping
their old contents by saying --clean.  (pg_upgrade, of course, will
do exactly that.)

			regards, tom lane

  44. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-19T19:54:25Z

    Hmm ... so there's a small problem with this idea of dropping and
recreating template1:

pg_restore: connecting to database for restore
pg_restore: dropping DATABASE template1
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 3024; 1262 1 DATABASE template1
 postgres
pg_restore: [archiver (db)] could not execute query: ERROR:  cannot drop a templ
ate database
    Command was: DROP DATABASE "template1";

Now in principle we could hack around that by issuing "ALTER DATABASE
... IS_TEMPLATE false" first, but it turns out to be harder than you
might think to wedge that into the pg_dump infrastructure.  (The
natural way to do it, trying to add this into the dropCmd for the
database TOC entry, fails because (a) DROP DATABASE then ends up as
one part of an implicit transaction block, and (b) it confuses the heck
out of pg_restore's --if-exists kluge.)

You can actually exhibit this in current releases if you try "pg_dump
--clean --create" on a user-created template database, so it's not
solely the fault of this patch.

What do people think of just removing this DROP DATABASE restriction?
Arguably, superusers should know better than to drop template1 anyway.
Maybe we should replace it with a hard-wired check against dropping
template0 (matched by name) just to stave off the worst-case scenario.

			regards, tom lane

  45. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Robert Haas <robertmhaas@gmail.com> — 2018-01-19T20:43:39Z

    On Fri, Jan 19, 2018 at 2:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hmm ... so there's a small problem with this idea of dropping and
> recreating template1:
>
> pg_restore: connecting to database for restore
> pg_restore: dropping DATABASE template1
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 3024; 1262 1 DATABASE template1
>  postgres
> pg_restore: [archiver (db)] could not execute query: ERROR:  cannot drop a templ
> ate database
>     Command was: DROP DATABASE "template1";
>
> Now in principle we could hack around that by issuing "ALTER DATABASE
> ... IS_TEMPLATE false" first, but it turns out to be harder than you
> might think to wedge that into the pg_dump infrastructure.  (The
> natural way to do it, trying to add this into the dropCmd for the
> database TOC entry, fails because (a) DROP DATABASE then ends up as
> one part of an implicit transaction block, and (b) it confuses the heck
> out of pg_restore's --if-exists kluge.)
>
> You can actually exhibit this in current releases if you try "pg_dump
> --clean --create" on a user-created template database, so it's not
> solely the fault of this patch.
>
> What do people think of just removing this DROP DATABASE restriction?
> Arguably, superusers should know better than to drop template1 anyway.
> Maybe we should replace it with a hard-wired check against dropping
> template0 (matched by name) just to stave off the worst-case scenario.

I think it's a little scary.  The tail might be wagging the dog at this point.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  46. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-19T21:07:24Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jan 19, 2018 at 2:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What do people think of just removing this DROP DATABASE restriction?
>> Arguably, superusers should know better than to drop template1 anyway.
>> Maybe we should replace it with a hard-wired check against dropping
>> template0 (matched by name) just to stave off the worst-case scenario.

> I think it's a little scary.  The tail might be wagging the dog at this point.

True, and having to make assumptions about which server version we're
restoring to is not very nice either.

After further reflection I've found a way to do this on the pg_dump
end that's not as ugly as I feared -- no uglier than most of the other
hacks there, anyway.  So nevermind ...

			regards, tom lane

  47. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Bruce Momjian <bruce@momjian.us> — 2018-01-21T15:17:48Z

    On Fri, Jan 19, 2018 at 02:54:25PM -0500, Tom Lane wrote:
> Hmm ... so there's a small problem with this idea of dropping and
> recreating template1:
> 
> pg_restore: connecting to database for restore
> pg_restore: dropping DATABASE template1
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 3024; 1262 1 DATABASE template1
>  postgres
> pg_restore: [archiver (db)] could not execute query: ERROR:  cannot drop a templ
> ate database
>     Command was: DROP DATABASE "template1";

Uh, the oid of the template1 database is 1, and I assume we would want
to preserve that too.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  48. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-21T16:02:29Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Jan 19, 2018 at 02:54:25PM -0500, Tom Lane wrote:
>> Command was: DROP DATABASE "template1";

> Uh, the oid of the template1 database is 1, and I assume we would want
> to preserve that too.

I don't feel any huge attachment to that.  In the first place, under
this proposal recreating template1 is something you would only need to do
if you weren't satisfied with its default properties as set by initdb.
Which ought to be a minority of users.  In the second place, if you are
changing those properties from the way initdb set it up, it's not really
virgin template1 anymore, so why shouldn't it have a new OID?

			regards, tom lane

  49. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-21T17:42:11Z

    I've been hacking away at this patch, and attached is what I've got
so far.  I think this is committable, but if anyone wants to do
further review and testing, that'd be fine.

Per discussion, I got rid of the separate --set-db-properties switch:
additional database properties are now applied if and only if --create is
specified.  But the DATABASE TOC entry itself still contains only CREATE
DATABASE; the additional commands are carried in a "DATABASE PROPERTIES"
TOC entry so that they will be issued after reconnecting to the new DB.
This dodges the "ALTER DATABASE SET default_transaction_read_only"
problem.  (Furthermore, it turns out that we have to do it like that
because pg_restore issues any one TOC entry's contents as a single PQexec.
If you try to cram anything else in with the CREATE DATABASE, then the
server spits up because CREATE DATABASE becomes part of an implicit
transaction block.)

I also fixed it so that a database's comment, security label, and ACL are
restored only when saying --create.  This is different from the previous
behavior for DB comments and security labels, but it seems a great deal
more useful and consistent.  In no other case would pg_dump/pg_restore
attempt to restore a comment, security label, or ACL for an object it
hadn't created, so why should it act differently for databases?

Worth noting here is that if we accept that behavior, the problem for
which "COMMENT ON CURRENT_DATABASE" was proposed goes away, because
there's no longer a risk of trying to apply COMMENT ON DATABASE to the
wrong database.  We might still want that patch as a standalone feature,
but pg_dump no longer needs it.

Another point worth commenting on (this change was also in the v13 patch)
is that pg_dumpall formerly took some pains to omit the encoding and
locale specifications in its CREATE DATABASE commands, for databases whose
settings matched the cluster default.  This new implementation won't do
that.  We could change it to do so, but then a standalone "pg_dump
--create" would also act that way, which is not really easy to defend.
IIRC, the argument for pg_dumpall doing it like that was to make it
less painful to migrate a cluster to a new machine that might not have the
identical set of locales, or if one wanted to migrate everything to a new
encoding.  But those are not mainstream use-cases, and if you really need
to do that you can still get there by dumping databases individually
without using --create.  On the other hand, there are obvious gotchas
involved in letting a dump/reload silently change to another locale or
encoding.  So I think this is an improvement overall, but it bears
noting as a behavioral change.

Another point to note is that I dithered about whether to bump the
pg_dump archive version number, which would have the effect of preventing
pre-v11 versions of pg_restore from processing dumps made by this version.
The argument for breaking archive compatibility is that older pg_restore
versions will not know that it'd be a good idea to skip DATABASE
PROPERTIES TOC entries and database ACL entries if not using --create.
However, in default cases there won't be a DATABASE PROPERTIES entry to
skip.  Moreover, applying these entries unconditionally isn't that much
different conceptually from applying database comments or sec labels
unconditionally, as such older pg_restore versions would do anyway.
It also seems like if your back were against the wall, being able to
read a newer archive file with an older pg_restore is better than being
locked out of it completely.  So I'm leaning to no version bump, but
it could still be discussed.

One thing we could possibly use here is more regression test cases.
The only existing regression test that's affected by this patch is
002_pg_dump.pl's expectations about which cases will print a database
comment, so it seems like we're missing some behavioral coverage.
Not sure what that should look like though.

This patch has to be applied over the patches proposed in
https://www.postgresql.org/message-id/21714.1516553459%40sss.pgh.pa.us
Aside from touching some of the same code, this is dependent on
the changes made there to make comment, seclabel, and ACL entries
reliably identifiable.

As far as notable code changes go, I got rid of the previous patch's
move of executeQuery() into dumputils.c.  That had some undesirable
knock-on effects in terms of creating even more coupling between
different modules (through the g_verbose global), and it was basically
misguided anyway.  pg_dump executes queries via ExecuteSqlQuery; we
do not need a few of its functions to be using low-level routines with
behavior different from that.

If anyone wants to do further review on this, let me know; otherwise
I'll push it in a day or so.

			regards, tom lane

  50. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Bruce Momjian <bruce@momjian.us> — 2018-01-23T15:45:37Z

    On Sun, Jan 21, 2018 at 11:02:29AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Fri, Jan 19, 2018 at 02:54:25PM -0500, Tom Lane wrote:
> >> Command was: DROP DATABASE "template1";
> 
> > Uh, the oid of the template1 database is 1, and I assume we would want
> > to preserve that too.
> 
> I don't feel any huge attachment to that.  In the first place, under
> this proposal recreating template1 is something you would only need to do
> if you weren't satisfied with its default properties as set by initdb.
> Which ought to be a minority of users.  In the second place, if you are
> changing those properties from the way initdb set it up, it's not really
> virgin template1 anymore, so why shouldn't it have a new OID?

Oh, I see what you mean.  I was just worried that some code might expect
template1 to always have an oid of 1, but we can just call that code
broken.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  51. Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-23T16:46:48Z

    Bruce Momjian <bruce@momjian.us> writes:
> Oh, I see what you mean.  I was just worried that some code might expect
> template1 to always have an oid of 1, but we can just call that code
> broken.

Ever since we invented template0, it's been possible to drop and recreate
template1, so I'd say any expectation that it must have OID 1 has been
wrong for a long time.  This change will just make the situation more
common.

			regards, tom lane