Thread: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

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

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

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

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

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

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

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

... 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

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

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

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

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

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

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

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

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

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

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

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 +

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

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

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 08cad68..11582dd 100644
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
*************** PostgreSQL documentation
*** 46,54 ****
    </para>

    <para>
!    <application>pg_dump</application> only dumps a single database.  To backup
!    global objects that are common to all databases in a cluster, such as roles
!    and tablespaces, use <xref linkend="app-pg-dumpall"/>.
    </para>

    <para>
--- 46,55 ----
    </para>

    <para>
!    <application>pg_dump</application> only dumps a single database.
!    To back up an entire cluster, or to back up global objects that are
!    common to all databases in a cluster (such as roles and tablespaces),
!    use <xref linkend="app-pg-dumpall"/>.
    </para>

    <para>
*************** PostgreSQL documentation
*** 142,148 ****
          switch is therefore only useful to add large objects to dumps
          where a specific schema or table has been requested.  Note that
          blobs are considered data and therefore will be included when
!         --data-only is used, but not when --schema-only is.
         </para>
        </listitem>
       </varlistentry>
--- 143,150 ----
          switch is therefore only useful to add large objects to dumps
          where a specific schema or table has been requested.  Note that
          blobs are considered data and therefore will be included when
!         <option>--data-only</option> is used, but not
!         when <option>--schema-only</option> is.
         </para>
        </listitem>
       </varlistentry>
*************** PostgreSQL documentation
*** 197,202 ****
--- 199,215 ----
         </para>

         <para>
+         With <option>--create</option>, the output also includes the
+         database's comment if any, and any configuration variable settings
+         that are specific to this database, that is,
+         any <command>ALTER DATABASE ... SET ...</command>
+         and <command>ALTER ROLE ... IN DATABASE ... SET ...</command>
+         commands that mention this database.
+         Access privileges for the database itself are also dumped,
+         unless <option>--no-acl</option> is specified.
+        </para>
+
+        <para>
          This option is only meaningful for the plain-text format.  For
          the archive formats, you can specify the option when you
          call <command>pg_restore</command>.
*************** CREATE DATABASE foo WITH TEMPLATE templa
*** 1231,1240 ****
     <command>ANALYZE</command> after restoring from a dump file
     to ensure optimal performance; see <xref linkend="vacuum-for-statistics"/>
     and <xref linkend="autovacuum"/> for more information.
-    The dump file also does not
-    contain any <command>ALTER DATABASE ... SET</command> commands;
-    these settings are dumped by <xref linkend="app-pg-dumpall"/>,
-    along with database users and other installation-wide settings.
    </para>

    <para>
--- 1244,1249 ----
*************** CREATE DATABASE foo WITH TEMPLATE templa
*** 1326,1331 ****
--- 1335,1349 ----
    </para>

    <para>
+    To reload an archive file into the same database it was dumped from,
+    discarding the current contents of that database:
+
+ <screen>
+ <prompt>$</prompt> <userinput>pg_restore -d postgres --clean --create db.dump</userinput>
+ </screen>
+   </para>
+
+   <para>
     To dump a single table named <literal>mytab</literal>:

  <screen>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 5196a21..4a639f2 100644
*** a/doc/src/sgml/ref/pg_dumpall.sgml
--- b/doc/src/sgml/ref/pg_dumpall.sgml
*************** PostgreSQL documentation
*** 36,48 ****
     of a cluster into one script file.  The script file contains
     <acronym>SQL</acronym> commands that can be used as input to <xref
     linkend="app-psql"/> to restore the databases.  It does this by
!    calling <xref linkend="app-pgdump"/> for each database in a cluster.
     <application>pg_dumpall</application> also dumps global objects
!    that are common to all databases.
     (<application>pg_dump</application> does not save these objects.)
-    This currently includes information about database users and
-    groups, tablespaces, and properties such as access permissions
-    that apply to databases as a whole.
    </para>

    <para>
--- 36,45 ----
     of a cluster into one script file.  The script file contains
     <acronym>SQL</acronym> commands that can be used as input to <xref
     linkend="app-psql"/> to restore the databases.  It does this by
!    calling <xref linkend="app-pgdump"/> for each database in the cluster.
     <application>pg_dumpall</application> also dumps global objects
!    that are common to all databases, that is, database roles and tablespaces.
     (<application>pg_dump</application> does not save these objects.)
    </para>

    <para>
*************** PostgreSQL documentation
*** 50,56 ****
     databases you will most likely have to connect as a database
     superuser in order to produce a complete dump.  Also you will need
     superuser privileges to execute the saved script in order to be
!    allowed to add users and groups, and to create databases.
    </para>

    <para>
--- 47,53 ----
     databases you will most likely have to connect as a database
     superuser in order to produce a complete dump.  Also you will need
     superuser privileges to execute the saved script in order to be
!    allowed to add roles and create databases.
    </para>

    <para>
*************** PostgreSQL documentation
*** 308,314 ****
        <listitem>
         <para>
          Use conditional commands (i.e. add an <literal>IF EXISTS</literal>
!         clause) to clean databases and other objects.  This option is not valid
          unless <option>--clean</option> is also specified.
         </para>
        </listitem>
--- 305,311 ----
        <listitem>
         <para>
          Use conditional commands (i.e. add an <literal>IF EXISTS</literal>
!         clause) to drop databases and other objects.  This option is not valid
          unless <option>--clean</option> is also specified.
         </para>
        </listitem>
*************** PostgreSQL documentation
*** 500,509 ****
         <para>
          The option is called <literal>--dbname</literal> for consistency with other
          client applications, but because <application>pg_dumpall</application>
!         needs to connect to many databases, database name in the connection
!         string will be ignored. Use <literal>-l</literal> option to specify
!         the name of the database used to dump global objects and to discover
!         what other databases should be dumped.
         </para>
        </listitem>
       </varlistentry>
--- 497,507 ----
         <para>
          The option is called <literal>--dbname</literal> for consistency with other
          client applications, but because <application>pg_dumpall</application>
!         needs to connect to many databases, the database name in the
!         connection string will be ignored.  Use the <literal>-l</literal>
!         option to specify the name of the database used for the initial
!         connection, which will dump global objects and discover what other
!         databases should be dumped.
         </para>
        </listitem>
       </varlistentry>
*************** PostgreSQL documentation
*** 658,663 ****
--- 656,672 ----
    </para>

    <para>
+    The <option>--clean</option> option can be useful even when your
+    intention is to restore the dump script into a fresh cluster.  Use of
+    <option>--clean</option> authorizes the script to drop and re-create the
+    built-in <literal>postgres</literal> and <literal>template1</literal>
+    databases, ensuring that those databases will retain the same properties
+    (for instance, locale and encoding) that they had in the source cluster.
+    Without the option, those databases will retain their existing
+    database-level properties, as well as any pre-existing contents.
+   </para>
+
+   <para>
     Once restored, it is wise to run <command>ANALYZE</command> on each
     database so the optimizer has useful statistics. You
     can also run <command>vacuumdb -a -z</command> to analyze all
*************** PostgreSQL documentation
*** 665,670 ****
--- 674,691 ----
    </para>

    <para>
+    The dump script should not be expected to run completely without errors.
+    In particular, because the script will issue <command>CREATE ROLE</command>
+    for every role existing in the source cluster, it is certain to get a
+    <quote>role already exists</quote> error for the bootstrap superuser,
+    unless the destination cluster was initialized with a different bootstrap
+    superuser name.  This error is harmless and should be ignored.  Use of
+    the <option>--clean</option> option is likely to produce additional
+    harmless error messages about non-existent objects, although you can
+    minimize those by adding <option>--if-exists</option>.
+   </para>
+
+   <para>
     <application>pg_dumpall</application> requires all needed
     tablespace directories to exist before the restore;  otherwise,
     database creation will fail for databases in non-default
*************** PostgreSQL documentation
*** 688,697 ****
  <screen>
  <prompt>$</prompt> <userinput>psql -f db.out postgres</userinput>
  </screen>
!    (It is not important to which database you connect here since the
     script file created by <application>pg_dumpall</application> will
     contain the appropriate commands to create and connect to the saved
!    databases.)
    </para>
   </refsect1>

--- 709,721 ----
  <screen>
  <prompt>$</prompt> <userinput>psql -f db.out postgres</userinput>
  </screen>
!    It is not important to which database you connect here since the
     script file created by <application>pg_dumpall</application> will
     contain the appropriate commands to create and connect to the saved
!    databases.  An exception is that if you specified <option>--clean</option>,
!    you must connect to the <literal>postgres</literal> database initially;
!    the script will attempt to drop other databases immediately, and that
!    will fail for the database you are connected to.
    </para>
   </refsect1>

diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 9946b94..a2ebf75 100644
*** a/doc/src/sgml/ref/pg_restore.sgml
--- b/doc/src/sgml/ref/pg_restore.sgml
***************
*** 127,132 ****
--- 127,143 ----
         </para>

         <para>
+         With <option>--create</option>, <application>pg_restore</application>
+         also restores the database's comment if any, and any configuration
+         variable settings that are specific to this database, that is,
+         any <command>ALTER DATABASE ... SET ...</command>
+         and <command>ALTER ROLE ... IN DATABASE ... SET ...</command>
+         commands that mention this database.
+         Access privileges for the database itself are also restored,
+         unless <option>--no-acl</option> is specified.
+        </para>
+
+        <para>
          When this option is used, the database named with <option>-d</option>
          is used only to issue the initial <command>DROP DATABASE</command> and
          <command>CREATE DATABASE</command> commands.  All data is restored into the
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 32ad600..7afddc3 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
*************** buildACLQueries(PQExpBuffer acl_subquery
*** 807,809 ****
--- 807,860 ----
          printfPQExpBuffer(init_racl_subquery, "NULL");
      }
  }
+
+ /*
+  * Helper function for dumping "ALTER DATABASE/ROLE SET ..." commands.
+  *
+  * Parse the contents of configitem (a "name=value" string), wrap it in
+  * a complete ALTER command, and append it to buf.
+  *
+  * type is DATABASE or ROLE, and name is the name of the database or role.
+  * If we need an "IN" clause, type2 and name2 similarly define what to put
+  * there; otherwise they should be NULL.
+  * conn is used only to determine string-literal quoting conventions.
+  */
+ void
+ makeAlterConfigCommand(PGconn *conn, const char *configitem,
+                        const char *type, const char *name,
+                        const char *type2, const char *name2,
+                        PQExpBuffer buf)
+ {
+     char       *mine;
+     char       *pos;
+
+     /* Parse the configitem.  If we can't find an "=", silently do nothing. */
+     mine = pg_strdup(configitem);
+     pos = strchr(mine, '=');
+     if (pos == NULL)
+     {
+         pg_free(mine);
+         return;
+     }
+     *pos++ = '\0';
+
+     /* Build the command, with suitable quoting for everything. */
+     appendPQExpBuffer(buf, "ALTER %s %s ", type, fmtId(name));
+     if (type2 != NULL && name2 != NULL)
+         appendPQExpBuffer(buf, "IN %s %s ", type2, fmtId(name2));
+     appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
+
+     /*
+      * Some GUC variable names are 'LIST' type and hence must not be quoted.
+      * XXX this list is incomplete ...
+      */
+     if (pg_strcasecmp(mine, "DateStyle") == 0
+         || pg_strcasecmp(mine, "search_path") == 0)
+         appendPQExpBufferStr(buf, pos);
+     else
+         appendStringLiteralConn(buf, pos, conn);
+
+     appendPQExpBufferStr(buf, ";\n");
+
+     pg_free(mine);
+ }
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index d5f150d..23a0645 100644
*** a/src/bin/pg_dump/dumputils.h
--- b/src/bin/pg_dump/dumputils.h
*************** extern void buildACLQueries(PQExpBuffer
*** 56,59 ****
--- 56,64 ----
                  const char *acl_column, const char *acl_owner,
                  const char *obj_kind, bool binary_upgrade);

+ extern void makeAlterConfigCommand(PGconn *conn, const char *configitem,
+                        const char *type, const char *name,
+                        const char *type2, const char *name2,
+                        PQExpBuffer buf);
+
  #endif                            /* DUMPUTILS_H */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 41741ae..9dabbf6 100644
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*************** RestoreArchive(Archive *AHX)
*** 489,504 ****
               * whole.  Issuing drops against anything else would be wrong,
               * because at this point we're connected to the wrong database.
               * Conversely, if we're not in createDB mode, we'd better not
!              * issue a DROP against the database at all.
               */
              if (ropt->createDB)
              {
!                 if (strcmp(te->desc, "DATABASE") != 0)
                      continue;
              }
              else
              {
!                 if (strcmp(te->desc, "DATABASE") == 0)
                      continue;
              }

--- 489,507 ----
               * whole.  Issuing drops against anything else would be wrong,
               * because at this point we're connected to the wrong database.
               * Conversely, if we're not in createDB mode, we'd better not
!              * issue a DROP against the database at all.  (The DATABASE
!              * PROPERTIES entry, if any, works like the DATABASE entry.)
               */
              if (ropt->createDB)
              {
!                 if (strcmp(te->desc, "DATABASE") != 0 &&
!                     strcmp(te->desc, "DATABASE PROPERTIES") != 0)
                      continue;
              }
              else
              {
!                 if (strcmp(te->desc, "DATABASE") == 0 ||
!                     strcmp(te->desc, "DATABASE PROPERTIES") == 0)
                      continue;
              }

*************** RestoreArchive(Archive *AHX)
*** 558,563 ****
--- 561,568 ----
                               * we simply emit the original command for DEFAULT
                               * objects (modulo the adjustment made above).
                               *
+                              * Likewise, don't mess with DATABASE PROPERTIES.
+                              *
                               * If we used CREATE OR REPLACE VIEW as a means of
                               * quasi-dropping an ON SELECT rule, that should
                               * be emitted unchanged as well.
*************** RestoreArchive(Archive *AHX)
*** 570,575 ****
--- 575,581 ----
                               * search for hardcoded "DROP CONSTRAINT" instead.
                               */
                              if (strcmp(te->desc, "DEFAULT") == 0 ||
+                                 strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
                                  strncmp(dropStmt, "CREATE OR REPLACE VIEW", 22) == 0)
                                  appendPQExpBufferStr(ftStmt, dropStmt);
                              else
*************** restore_toc_entry(ArchiveHandle *AH, Toc
*** 750,760 ****
      reqs = te->reqs;

      /*
!      * Ignore DATABASE entry unless we should create it.  We must check this
!      * here, not in _tocEntryRequired, because the createDB option should not
!      * affect emitting a DATABASE entry to an archive file.
       */
!     if (!ropt->createDB && strcmp(te->desc, "DATABASE") == 0)
          reqs = 0;

      /* Dump any relevant dump warnings to stderr */
--- 756,774 ----
      reqs = te->reqs;

      /*
!      * Ignore DATABASE and related entries unless createDB is specified.  We
!      * must check this here, not in _tocEntryRequired, because !createDB
!      * should not prevent emitting these entries to an archive file.
       */
!     if (!ropt->createDB &&
!         (strcmp(te->desc, "DATABASE") == 0 ||
!          strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
!          (strcmp(te->desc, "ACL") == 0 &&
!           strncmp(te->tag, "DATABASE ", 9) == 0) ||
!          (strcmp(te->desc, "COMMENT") == 0 &&
!           strncmp(te->tag, "DATABASE ", 9) == 0) ||
!          (strcmp(te->desc, "SECURITY LABEL") == 0 &&
!           strncmp(te->tag, "DATABASE ", 9) == 0)))
          reqs = 0;

      /* Dump any relevant dump warnings to stderr */
*************** _tocEntryRequired(TocEntry *te, teSectio
*** 2917,2924 ****
           * Special Case: If 'SEQUENCE SET' or anything to do with BLOBs, then
           * it is considered a data entry.  We don't need to check for the
           * BLOBS entry or old-style BLOB COMMENTS, because they will have
!          * hadDumper = true ... but we do need to check new-style BLOB
!          * comments.
           */
          if (strcmp(te->desc, "SEQUENCE SET") == 0 ||
              strcmp(te->desc, "BLOB") == 0 ||
--- 2931,2938 ----
           * Special Case: If 'SEQUENCE SET' or anything to do with BLOBs, then
           * it is considered a data entry.  We don't need to check for the
           * BLOBS entry or old-style BLOB COMMENTS, because they will have
!          * hadDumper = true ... but we do need to check new-style BLOB ACLs,
!          * comments, etc.
           */
          if (strcmp(te->desc, "SEQUENCE SET") == 0 ||
              strcmp(te->desc, "BLOB") == 0 ||
*************** _printTocEntry(ArchiveHandle *AH, TocEnt
*** 3590,3595 ****
--- 3604,3610 ----
          else if (strcmp(te->desc, "CAST") == 0 ||
                   strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
                   strcmp(te->desc, "CONSTRAINT") == 0 ||
+                  strcmp(te->desc, "DATABASE PROPERTIES") == 0 ||
                   strcmp(te->desc, "DEFAULT") == 0 ||
                   strcmp(te->desc, "FK CONSTRAINT") == 0 ||
                   strcmp(te->desc, "INDEX") == 0 ||
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index db65552..6473e58 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** static void dumpPublication(Archive *fou
*** 252,257 ****
--- 252,259 ----
  static void dumpPublicationTable(Archive *fout, PublicationRelInfo *pubrinfo);
  static void dumpSubscription(Archive *fout, SubscriptionInfo *subinfo);
  static void dumpDatabase(Archive *AH);
+ static void dumpDatabaseConfig(Archive *AH, PQExpBuffer outbuf,
+                    const char *dbname, Oid dboid);
  static void dumpEncoding(Archive *AH);
  static void dumpStdStrings(Archive *AH);
  static void binary_upgrade_set_type_oids_by_type_oid(Archive *fout,
*************** main(int argc, char **argv)
*** 838,844 ****
      dumpEncoding(fout);
      dumpStdStrings(fout);

!     /* The database item is always next, unless we don't want it at all */
      if (dopt.include_everything && !dopt.dataOnly)
          dumpDatabase(fout);

--- 840,846 ----
      dumpEncoding(fout);
      dumpStdStrings(fout);

!     /* The database items are always next, unless we don't want them at all */
      if (dopt.include_everything && !dopt.dataOnly)
          dumpDatabase(fout);

*************** dumpDatabase(Archive *fout)
*** 2540,2545 ****
--- 2542,2551 ----
                  i_ctype,
                  i_frozenxid,
                  i_minmxid,
+                 i_datacl,
+                 i_rdatacl,
+                 i_datistemplate,
+                 i_datconnlimit,
                  i_tablespace;
      CatalogId    dbCatId;
      DumpId        dbDumpId;
*************** dumpDatabase(Archive *fout)
*** 2548,2558 ****
--- 2554,2570 ----
                 *encoding,
                 *collate,
                 *ctype,
+                *datacl,
+                *rdatacl,
+                *datistemplate,
+                *datconnlimit,
                 *tablespace;
      uint32        frozenxid,
                  minmxid;
+     char       *qdatname;

      datname = PQdb(conn);
+     qdatname = pg_strdup(fmtId(datname));

      if (g_verbose)
          write_msg(NULL, "saving database definition\n");
*************** dumpDatabase(Archive *fout)
*** 2560,2572 ****
      /* Make sure we are in proper schema */
      selectSourceSchema(fout, "pg_catalog");

!     /* Get the database owner and parameters from pg_database */
!     if (fout->remoteVersion >= 90300)
      {
          appendPQExpBuffer(dbQry, "SELECT tableoid, oid, "
                            "(%s datdba) AS dba, "
                            "pg_encoding_to_char(encoding) AS encoding, "
                            "datcollate, datctype, datfrozenxid, datminmxid, "
                            "(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
                            "shobj_description(oid, 'pg_database') AS description "

--- 2572,2608 ----
      /* Make sure we are in proper schema */
      selectSourceSchema(fout, "pg_catalog");

!     /* Fetch the database-level properties for this database */
!     if (fout->remoteVersion >= 90600)
!     {
!         appendPQExpBuffer(dbQry, "SELECT tableoid, oid, "
!                           "(%s datdba) AS dba, "
!                           "pg_encoding_to_char(encoding) AS encoding, "
!                           "datcollate, datctype, datfrozenxid, datminmxid, "
!                           "(SELECT array_agg(acl ORDER BY acl::text COLLATE \"C\") FROM ( "
!                           "  SELECT unnest(coalesce(datacl,acldefault('d',datdba))) AS acl "
!                           "  EXCEPT SELECT unnest(acldefault('d',datdba))) as datacls)"
!                           " AS datacl, "
!                           "(SELECT array_agg(acl ORDER BY acl::text COLLATE \"C\") FROM ( "
!                           "  SELECT unnest(acldefault('d',datdba)) AS acl "
!                           "  EXCEPT SELECT unnest(coalesce(datacl,acldefault('d',datdba)))) as rdatacls)"
!                           " AS rdatacl, "
!                           "datistemplate, datconnlimit, "
!                           "(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
!                           "shobj_description(oid, 'pg_database') AS description "
!
!                           "FROM pg_database "
!                           "WHERE datname = ",
!                           username_subquery);
!         appendStringLiteralAH(dbQry, datname, fout);
!     }
!     else if (fout->remoteVersion >= 90300)
      {
          appendPQExpBuffer(dbQry, "SELECT tableoid, oid, "
                            "(%s datdba) AS dba, "
                            "pg_encoding_to_char(encoding) AS encoding, "
                            "datcollate, datctype, datfrozenxid, datminmxid, "
+                           "datacl, '' as rdatacl, datistemplate, datconnlimit, "
                            "(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
                            "shobj_description(oid, 'pg_database') AS description "

*************** dumpDatabase(Archive *fout)
*** 2581,2586 ****
--- 2617,2623 ----
                            "(%s datdba) AS dba, "
                            "pg_encoding_to_char(encoding) AS encoding, "
                            "datcollate, datctype, datfrozenxid, 0 AS datminmxid, "
+                           "datacl, '' as rdatacl, datistemplate, datconnlimit, "
                            "(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
                            "shobj_description(oid, 'pg_database') AS description "

*************** dumpDatabase(Archive *fout)
*** 2595,2600 ****
--- 2632,2638 ----
                            "(%s datdba) AS dba, "
                            "pg_encoding_to_char(encoding) AS encoding, "
                            "NULL AS datcollate, NULL AS datctype, datfrozenxid, 0 AS datminmxid, "
+                           "datacl, '' as rdatacl, datistemplate, datconnlimit, "
                            "(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
                            "shobj_description(oid, 'pg_database') AS description "

*************** dumpDatabase(Archive *fout)
*** 2609,2614 ****
--- 2647,2654 ----
                            "(%s datdba) AS dba, "
                            "pg_encoding_to_char(encoding) AS encoding, "
                            "NULL AS datcollate, NULL AS datctype, datfrozenxid, 0 AS datminmxid, "
+                           "datacl, '' as rdatacl, datistemplate, "
+                           "-1 as datconnlimit, "
                            "(SELECT spcname FROM pg_tablespace t WHERE t.oid = dattablespace) AS tablespace "
                            "FROM pg_database "
                            "WHERE datname = ",
*************** dumpDatabase(Archive *fout)
*** 2626,2631 ****
--- 2666,2675 ----
      i_ctype = PQfnumber(res, "datctype");
      i_frozenxid = PQfnumber(res, "datfrozenxid");
      i_minmxid = PQfnumber(res, "datminmxid");
+     i_datacl = PQfnumber(res, "datacl");
+     i_rdatacl = PQfnumber(res, "rdatacl");
+     i_datistemplate = PQfnumber(res, "datistemplate");
+     i_datconnlimit = PQfnumber(res, "datconnlimit");
      i_tablespace = PQfnumber(res, "tablespace");

      dbCatId.tableoid = atooid(PQgetvalue(res, 0, i_tableoid));
*************** dumpDatabase(Archive *fout)
*** 2636,2645 ****
      ctype = PQgetvalue(res, 0, i_ctype);
      frozenxid = atooid(PQgetvalue(res, 0, i_frozenxid));
      minmxid = atooid(PQgetvalue(res, 0, i_minmxid));
      tablespace = PQgetvalue(res, 0, i_tablespace);

      appendPQExpBuffer(creaQry, "CREATE DATABASE %s WITH TEMPLATE = template0",
!                       fmtId(datname));
      if (strlen(encoding) > 0)
      {
          appendPQExpBufferStr(creaQry, " ENCODING = ");
--- 2680,2699 ----
      ctype = PQgetvalue(res, 0, i_ctype);
      frozenxid = atooid(PQgetvalue(res, 0, i_frozenxid));
      minmxid = atooid(PQgetvalue(res, 0, i_minmxid));
+     datacl = PQgetvalue(res, 0, i_datacl);
+     rdatacl = PQgetvalue(res, 0, i_rdatacl);
+     datistemplate = PQgetvalue(res, 0, i_datistemplate);
+     datconnlimit = PQgetvalue(res, 0, i_datconnlimit);
      tablespace = PQgetvalue(res, 0, i_tablespace);

+     /*
+      * Prepare the CREATE DATABASE command.  We must specify encoding, locale,
+      * and tablespace since those can't be altered later.  Other DB properties
+      * are left to the DATABASE PROPERTIES entry, so that they can be applied
+      * after reconnecting to the target DB.
+      */
      appendPQExpBuffer(creaQry, "CREATE DATABASE %s WITH TEMPLATE = template0",
!                       qdatname);
      if (strlen(encoding) > 0)
      {
          appendPQExpBufferStr(creaQry, " ENCODING = ");
*************** dumpDatabase(Archive *fout)
*** 2655,2680 ****
          appendPQExpBufferStr(creaQry, " LC_CTYPE = ");
          appendStringLiteralAH(creaQry, ctype, fout);
      }
      if (strlen(tablespace) > 0 && strcmp(tablespace, "pg_default") != 0 &&
          !dopt->outputNoTablespaces)
          appendPQExpBuffer(creaQry, " TABLESPACE = %s",
                            fmtId(tablespace));
      appendPQExpBufferStr(creaQry, ";\n");

-     if (dopt->binary_upgrade)
-     {
-         appendPQExpBufferStr(creaQry, "\n-- For binary upgrade, set datfrozenxid and datminmxid.\n");
-         appendPQExpBuffer(creaQry, "UPDATE pg_catalog.pg_database\n"
-                           "SET datfrozenxid = '%u', datminmxid = '%u'\n"
-                           "WHERE datname = ",
-                           frozenxid, minmxid);
-         appendStringLiteralAH(creaQry, datname, fout);
-         appendPQExpBufferStr(creaQry, ";\n");
-
-     }
-
      appendPQExpBuffer(delQry, "DROP DATABASE %s;\n",
!                       fmtId(datname));

      dbDumpId = createDumpId();

--- 2709,2731 ----
          appendPQExpBufferStr(creaQry, " LC_CTYPE = ");
          appendStringLiteralAH(creaQry, ctype, fout);
      }
+
+     /*
+      * Note: looking at dopt->outputNoTablespaces here is completely the wrong
+      * thing; the decision whether to specify a tablespace should be left till
+      * pg_restore, so that pg_restore --no-tablespaces applies.  Ideally we'd
+      * label the DATABASE entry with the tablespace and let the normal
+      * tablespace selection logic work ... but CREATE DATABASE doesn't pay
+      * attention to default_tablespace, so that won't work.
+      */
      if (strlen(tablespace) > 0 && strcmp(tablespace, "pg_default") != 0 &&
          !dopt->outputNoTablespaces)
          appendPQExpBuffer(creaQry, " TABLESPACE = %s",
                            fmtId(tablespace));
      appendPQExpBufferStr(creaQry, ";\n");

      appendPQExpBuffer(delQry, "DROP DATABASE %s;\n",
!                       qdatname);

      dbDumpId = createDumpId();

*************** dumpDatabase(Archive *fout)
*** 2697,2703 ****
                   NULL);            /* Dumper Arg */

      /* Compute correct tag for comments etc */
!     appendPQExpBuffer(labelq, "DATABASE %s", fmtId(datname));

      /* Dump DB comment if any */
      if (fout->remoteVersion >= 80200)
--- 2748,2754 ----
                   NULL);            /* Dumper Arg */

      /* Compute correct tag for comments etc */
!     appendPQExpBuffer(labelq, "DATABASE %s", qdatname);

      /* Dump DB comment if any */
      if (fout->remoteVersion >= 80200)
*************** dumpDatabase(Archive *fout)
*** 2717,2723 ****
               * Generates warning when loaded into a differently-named
               * database.
               */
!             appendPQExpBuffer(dbQry, "COMMENT ON DATABASE %s IS ", fmtId(datname));
              appendStringLiteralAH(dbQry, comment, fout);
              appendPQExpBufferStr(dbQry, ";\n");

--- 2768,2774 ----
               * Generates warning when loaded into a differently-named
               * database.
               */
!             appendPQExpBuffer(dbQry, "COMMENT ON DATABASE %s IS ", qdatname);
              appendStringLiteralAH(dbQry, comment, fout);
              appendPQExpBufferStr(dbQry, ";\n");

*************** dumpDatabase(Archive *fout)
*** 2759,2764 ****
--- 2810,2882 ----
      }

      /*
+      * Dump ACL if any.  Note that we do not support initial privileges
+      * (pg_init_privs) on databases.
+      */
+     dumpACL(fout, dbCatId, dbDumpId, "DATABASE",
+             qdatname, NULL, labelq->data, NULL,
+             dba, datacl, rdatacl, "", "");
+
+     /*
+      * Now construct a DATABASE PROPERTIES archive entry to restore any
+      * non-default database-level properties.  We want to do this after
+      * reconnecting so that these properties won't apply during the restore
+      * session.  In this way, restoring works even if there is, say, an ALTER
+      * DATABASE SET that turns on default_transaction_read_only.
+      */
+     resetPQExpBuffer(creaQry);
+     resetPQExpBuffer(delQry);
+
+     if (strlen(datconnlimit) > 0 && strcmp(datconnlimit, "-1") != 0)
+         appendPQExpBuffer(creaQry, "ALTER DATABASE %s CONNECTION LIMIT = %s;\n",
+                           qdatname, datconnlimit);
+
+     if (strcmp(datistemplate, "t") == 0)
+     {
+         appendPQExpBuffer(creaQry, "ALTER DATABASE %s IS_TEMPLATE = true;\n",
+                           qdatname);
+
+         /*
+          * The backend won't accept DROP DATABASE on a template database.  We
+          * can deal with that by removing the template marking before the DROP
+          * gets issued.  We'd prefer to use ALTER DATABASE IF EXISTS here, but
+          * since no such command is currently supported, fake it with a direct
+          * UPDATE on pg_database.
+          */
+         appendPQExpBufferStr(delQry, "UPDATE pg_catalog.pg_database "
+                              "SET datistemplate = false WHERE datname = ");
+         appendStringLiteralAH(delQry, datname, fout);
+         appendPQExpBufferStr(delQry, ";\n");
+     }
+
+     /* Add database-specific SET options */
+     dumpDatabaseConfig(fout, creaQry, datname, dbCatId.oid);
+
+     /*
+      * We stick this binary-upgrade query into the DATABASE PROPERTIES archive
+      * entry, too.  It can't go into the DATABASE entry because that would
+      * result in an implicit transaction block around the CREATE DATABASE.
+      */
+     if (dopt->binary_upgrade)
+     {
+         appendPQExpBufferStr(creaQry, "\n-- For binary upgrade, set datfrozenxid and datminmxid.\n");
+         appendPQExpBuffer(creaQry, "UPDATE pg_catalog.pg_database\n"
+                           "SET datfrozenxid = '%u', datminmxid = '%u'\n"
+                           "WHERE datname = ",
+                           frozenxid, minmxid);
+         appendStringLiteralAH(creaQry, datname, fout);
+         appendPQExpBufferStr(creaQry, ";\n");
+     }
+
+     if (creaQry->len > 0)
+         ArchiveEntry(fout, nilCatalogId, createDumpId(),
+                      datname, NULL, NULL, dba,
+                      false, "DATABASE PROPERTIES", SECTION_PRE_DATA,
+                      creaQry->data, delQry->data, NULL,
+                      NULL, 0,
+                      NULL, NULL);
+
+     /*
       * pg_largeobject and pg_largeobject_metadata come from the old system
       * intact, so set their relfrozenxids and relminmxids.
       */
*************** dumpDatabase(Archive *fout)
*** 2793,2800 ****
          appendPQExpBuffer(loOutQry, "UPDATE pg_catalog.pg_class\n"
                            "SET relfrozenxid = '%u', relminmxid = '%u'\n"
                            "WHERE oid = %u;\n",
!                           atoi(PQgetvalue(lo_res, 0, i_relfrozenxid)),
!                           atoi(PQgetvalue(lo_res, 0, i_relminmxid)),
                            LargeObjectRelationId);
          ArchiveEntry(fout, nilCatalogId, createDumpId(),
                       "pg_largeobject", NULL, NULL, "",
--- 2911,2918 ----
          appendPQExpBuffer(loOutQry, "UPDATE pg_catalog.pg_class\n"
                            "SET relfrozenxid = '%u', relminmxid = '%u'\n"
                            "WHERE oid = %u;\n",
!                           atooid(PQgetvalue(lo_res, 0, i_relfrozenxid)),
!                           atooid(PQgetvalue(lo_res, 0, i_relminmxid)),
                            LargeObjectRelationId);
          ArchiveEntry(fout, nilCatalogId, createDumpId(),
                       "pg_largeobject", NULL, NULL, "",
*************** dumpDatabase(Archive *fout)
*** 2833,2840 ****
              appendPQExpBuffer(loOutQry, "UPDATE pg_catalog.pg_class\n"
                                "SET relfrozenxid = '%u', relminmxid = '%u'\n"
                                "WHERE oid = %u;\n",
!                               atoi(PQgetvalue(lo_res, 0, i_relfrozenxid)),
!                               atoi(PQgetvalue(lo_res, 0, i_relminmxid)),
                                LargeObjectMetadataRelationId);
              ArchiveEntry(fout, nilCatalogId, createDumpId(),
                           "pg_largeobject_metadata", NULL, NULL, "",
--- 2951,2958 ----
              appendPQExpBuffer(loOutQry, "UPDATE pg_catalog.pg_class\n"
                                "SET relfrozenxid = '%u', relminmxid = '%u'\n"
                                "WHERE oid = %u;\n",
!                               atooid(PQgetvalue(lo_res, 0, i_relfrozenxid)),
!                               atooid(PQgetvalue(lo_res, 0, i_relminmxid)),
                                LargeObjectMetadataRelationId);
              ArchiveEntry(fout, nilCatalogId, createDumpId(),
                           "pg_largeobject_metadata", NULL, NULL, "",
*************** dumpDatabase(Archive *fout)
*** 2852,2857 ****
--- 2970,2976 ----

      PQclear(res);

+     free(qdatname);
      destroyPQExpBuffer(dbQry);
      destroyPQExpBuffer(delQry);
      destroyPQExpBuffer(creaQry);
*************** dumpDatabase(Archive *fout)
*** 2859,2864 ****
--- 2978,3055 ----
  }

  /*
+  * Collect any database-specific or role-and-database-specific SET options
+  * for this database, and append them to outbuf.
+  */
+ static void
+ dumpDatabaseConfig(Archive *AH, PQExpBuffer outbuf,
+                    const char *dbname, Oid dboid)
+ {
+     PGconn       *conn = GetConnection(AH);
+     PQExpBuffer buf = createPQExpBuffer();
+     PGresult   *res;
+     int            count = 1;
+
+     /*
+      * First collect database-specific options.  Old server versions lack
+      * unnest(), so we do that the hard way by querying once per subscript.
+      */
+     for (;;)
+     {
+         if (AH->remoteVersion >= 90000)
+             printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting "
+                               "WHERE setrole = 0 AND setdatabase = '%u'::oid",
+                               count, dboid);
+         else
+             printfPQExpBuffer(buf, "SELECT datconfig[%d] FROM pg_database WHERE oid = '%u'::oid", count, dboid);
+
+         res = ExecuteSqlQuery(AH, buf->data, PGRES_TUPLES_OK);
+
+         if (PQntuples(res) == 1 &&
+             !PQgetisnull(res, 0, 0))
+         {
+             makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
+                                    "DATABASE", dbname, NULL, NULL,
+                                    outbuf);
+             PQclear(res);
+             count++;
+         }
+         else
+         {
+             PQclear(res);
+             break;
+         }
+     }
+
+     /* Now look for role-and-database-specific options */
+     if (AH->remoteVersion >= 90000)
+     {
+         /* Here we can assume we have unnest() */
+         printfPQExpBuffer(buf, "SELECT rolname, unnest(setconfig) "
+                           "FROM pg_db_role_setting s, pg_roles r "
+                           "WHERE setrole = r.oid AND setdatabase = '%u'::oid",
+                           dboid);
+
+         res = ExecuteSqlQuery(AH, buf->data, PGRES_TUPLES_OK);
+
+         if (PQntuples(res) > 0)
+         {
+             int            i;
+
+             for (i = 0; i < PQntuples(res); i++)
+                 makeAlterConfigCommand(conn, PQgetvalue(res, i, 1),
+                                        "ROLE", PQgetvalue(res, i, 0),
+                                        "DATABASE", dbname,
+                                        outbuf);
+         }
+
+         PQclear(res);
+     }
+
+     destroyPQExpBuffer(buf);
+ }
+
+ /*
   * dumpEncoding: put the correct encoding into the archive
   */
  static void
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 3dd2c38..2fd5a02 100644
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** static void dumpGroups(PGconn *conn);
*** 38,54 ****
  static void dropTablespaces(PGconn *conn);
  static void dumpTablespaces(PGconn *conn);
  static void dropDBs(PGconn *conn);
- static void dumpCreateDB(PGconn *conn);
- static void dumpDatabaseConfig(PGconn *conn, const char *dbname);
  static void dumpUserConfig(PGconn *conn, const char *username);
- static void dumpDbRoleConfig(PGconn *conn);
- static void makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
-                        const char *type, const char *name, const char *type2,
-                        const char *name2);
  static void dumpDatabases(PGconn *conn);
  static void dumpTimestamp(const char *msg);
!
! static int    runPgDump(const char *dbname);
  static void buildShSecLabels(PGconn *conn, const char *catalog_name,
                   uint32 objectId, PQExpBuffer buffer,
                   const char *target, const char *objname);
--- 38,47 ----
  static void dropTablespaces(PGconn *conn);
  static void dumpTablespaces(PGconn *conn);
  static void dropDBs(PGconn *conn);
  static void dumpUserConfig(PGconn *conn, const char *username);
  static void dumpDatabases(PGconn *conn);
  static void dumpTimestamp(const char *msg);
! static int    runPgDump(const char *dbname, const char *create_opts);
  static void buildShSecLabels(PGconn *conn, const char *catalog_name,
                   uint32 objectId, PQExpBuffer buffer,
                   const char *target, const char *objname);
*************** static char pg_dump_bin[MAXPGPATH];
*** 62,67 ****
--- 55,61 ----
  static const char *progname;
  static PQExpBuffer pgdumpopts;
  static char *connstr = "";
+ static bool output_clean = false;
  static bool skip_acls = false;
  static bool verbose = false;
  static bool dosync = true;
*************** main(int argc, char *argv[])
*** 152,158 ****
      trivalue    prompt_password = TRI_DEFAULT;
      bool        data_only = false;
      bool        globals_only = false;
-     bool        output_clean = false;
      bool        roles_only = false;
      bool        tablespaces_only = false;
      PGconn       *conn;
--- 146,151 ----
*************** main(int argc, char *argv[])
*** 558,574 ****
          /* Dump tablespaces */
          if (!roles_only && !no_tablespaces)
              dumpTablespaces(conn);
-
-         /* Dump CREATE DATABASE commands */
-         if (binary_upgrade || (!globals_only && !roles_only && !tablespaces_only))
-             dumpCreateDB(conn);
-
-         /* Dump role/database settings */
-         if (!tablespaces_only && !roles_only)
-         {
-             if (server_version >= 90000)
-                 dumpDbRoleConfig(conn);
-         }
      }

      if (!globals_only && !roles_only && !tablespaces_only)
--- 551,556 ----
*************** dumpTablespaces(PGconn *conn)
*** 1262,1269 ****

  /*
   * Dump commands to drop each database.
-  *
-  * This should match the set of databases targeted by dumpCreateDB().
   */
  static void
  dropDBs(PGconn *conn)
--- 1244,1249 ----
*************** dropDBs(PGconn *conn)
*** 1271,1294 ****
      PGresult   *res;
      int            i;

      res = executeQuery(conn,
                         "SELECT datname "
                         "FROM pg_database d "
!                        "WHERE datallowconn ORDER BY 1");

      if (PQntuples(res) > 0)
!         fprintf(OPF, "--\n-- Drop databases\n--\n\n");

      for (i = 0; i < PQntuples(res); i++)
      {
          char       *dbname = PQgetvalue(res, i, 0);

          /*
!          * Skip "template1" and "postgres"; the restore script is almost
!          * certainly going to be run in one or the other, and we don't know
!          * which.  This must agree with dumpCreateDB's choices!
           */
          if (strcmp(dbname, "template1") != 0 &&
              strcmp(dbname, "postgres") != 0)
          {
              fprintf(OPF, "DROP DATABASE %s%s;\n",
--- 1251,1280 ----
      PGresult   *res;
      int            i;

+     /*
+      * Skip databases marked not datallowconn, since we'd be unable to connect
+      * to them anyway.  This must agree with dumpDatabases().
+      */
      res = executeQuery(conn,
                         "SELECT datname "
                         "FROM pg_database d "
!                        "WHERE datallowconn "
!                        "ORDER BY datname");

      if (PQntuples(res) > 0)
!         fprintf(OPF, "--\n-- Drop databases (except postgres and template1)\n--\n\n");

      for (i = 0; i < PQntuples(res); i++)
      {
          char       *dbname = PQgetvalue(res, i, 0);

          /*
!          * Skip "postgres" and "template1"; dumpDatabases() will deal with
!          * them specially.  Also, be sure to skip "template0", even if for
!          * some reason it's not marked !datallowconn.
           */
          if (strcmp(dbname, "template1") != 0 &&
+             strcmp(dbname, "template0") != 0 &&
              strcmp(dbname, "postgres") != 0)
          {
              fprintf(OPF, "DROP DATABASE %s%s;\n",
*************** dropDBs(PGconn *conn)
*** 1302,1624 ****
      fprintf(OPF, "\n\n");
  }

- /*
-  * Dump commands to create each database.
-  *
-  * To minimize the number of reconnections (and possibly ensuing
-  * password prompts) required by the output script, we emit all CREATE
-  * DATABASE commands during the initial phase of the script, and then
-  * run pg_dump for each database to dump the contents of that
-  * database.  We skip databases marked not datallowconn, since we'd be
-  * unable to connect to them anyway (and besides, we don't want to
-  * dump template0).
-  */
- static void
- dumpCreateDB(PGconn *conn)
- {
-     PQExpBuffer buf = createPQExpBuffer();
-     char       *default_encoding = NULL;
-     char       *default_collate = NULL;
-     char       *default_ctype = NULL;
-     PGresult   *res;
-     int            i;
-
-     fprintf(OPF, "--\n-- Database creation\n--\n\n");
-
-     /*
-      * First, get the installation's default encoding and locale information.
-      * We will dump encoding and locale specifications in the CREATE DATABASE
-      * commands for just those databases with values different from defaults.
-      *
-      * We consider template0's encoding and locale to define the installation
-      * default.  Pre-8.4 installations do not have per-database locale
-      * settings; for them, every database must necessarily be using the
-      * installation default, so there's no need to do anything.
-      */
-     if (server_version >= 80400)
-         res = executeQuery(conn,
-                            "SELECT pg_encoding_to_char(encoding), "
-                            "datcollate, datctype "
-                            "FROM pg_database "
-                            "WHERE datname = 'template0'");
-     else
-         res = executeQuery(conn,
-                            "SELECT pg_encoding_to_char(encoding), "
-                            "null::text AS datcollate, null::text AS datctype "
-                            "FROM pg_database "
-                            "WHERE datname = 'template0'");
-
-     /* If for some reason the template DB isn't there, treat as unknown */
-     if (PQntuples(res) > 0)
-     {
-         if (!PQgetisnull(res, 0, 0))
-             default_encoding = pg_strdup(PQgetvalue(res, 0, 0));
-         if (!PQgetisnull(res, 0, 1))
-             default_collate = pg_strdup(PQgetvalue(res, 0, 1));
-         if (!PQgetisnull(res, 0, 2))
-             default_ctype = pg_strdup(PQgetvalue(res, 0, 2));
-     }
-
-     PQclear(res);
-
-
-     /*
-      * Now collect all the information about databases to dump.
-      *
-      * For the database ACLs, as of 9.6, we extract both the positive (as
-      * datacl) and negative (as rdatacl) ACLs, relative to the default ACL for
-      * databases, which are then passed to buildACLCommands() below.
-      *
-      * See buildACLQueries() and buildACLCommands().
-      *
-      * Note that we do not support initial privileges (pg_init_privs) on
-      * databases.
-      */
-     if (server_version >= 90600)
-         printfPQExpBuffer(buf,
-                           "SELECT datname, "
-                           "coalesce(rolname, (select rolname from %s where oid=(select datdba from pg_database where
datname='template0')))," 
-                           "pg_encoding_to_char(d.encoding), "
-                           "datcollate, datctype, datfrozenxid, datminmxid, "
-                           "datistemplate, "
-                           "(SELECT pg_catalog.array_agg(acl ORDER BY acl::text COLLATE \"C\") FROM ( "
-                           "  SELECT pg_catalog.unnest(coalesce(datacl,pg_catalog.acldefault('d',datdba))) AS acl "
-                           "  EXCEPT SELECT pg_catalog.unnest(pg_catalog.acldefault('d',datdba))) as datacls)"
-                           "AS datacl, "
-                           "(SELECT pg_catalog.array_agg(acl ORDER BY acl::text COLLATE \"C\") FROM ( "
-                           "  SELECT pg_catalog.unnest(pg_catalog.acldefault('d',datdba)) AS acl "
-                           "  EXCEPT SELECT pg_catalog.unnest(coalesce(datacl,pg_catalog.acldefault('d',datdba)))) as
rdatacls)"
-                           "AS rdatacl, "
-                           "datconnlimit, "
-                           "(SELECT spcname FROM pg_tablespace t WHERE t.oid = d.dattablespace) AS dattablespace "
-                           "FROM pg_database d LEFT JOIN %s u ON (datdba = u.oid) "
-                           "WHERE datallowconn ORDER BY 1", role_catalog, role_catalog);
-     else if (server_version >= 90300)
-         printfPQExpBuffer(buf,
-                           "SELECT datname, "
-                           "coalesce(rolname, (select rolname from %s where oid=(select datdba from pg_database where
datname='template0')))," 
-                           "pg_encoding_to_char(d.encoding), "
-                           "datcollate, datctype, datfrozenxid, datminmxid, "
-                           "datistemplate, datacl, '' as rdatacl, "
-                           "datconnlimit, "
-                           "(SELECT spcname FROM pg_tablespace t WHERE t.oid = d.dattablespace) AS dattablespace "
-                           "FROM pg_database d LEFT JOIN %s u ON (datdba = u.oid) "
-                           "WHERE datallowconn ORDER BY 1", role_catalog, role_catalog);
-     else if (server_version >= 80400)
-         printfPQExpBuffer(buf,
-                           "SELECT datname, "
-                           "coalesce(rolname, (select rolname from %s where oid=(select datdba from pg_database where
datname='template0')))," 
-                           "pg_encoding_to_char(d.encoding), "
-                           "datcollate, datctype, datfrozenxid, 0 AS datminmxid, "
-                           "datistemplate, datacl, '' as rdatacl, "
-                           "datconnlimit, "
-                           "(SELECT spcname FROM pg_tablespace t WHERE t.oid = d.dattablespace) AS dattablespace "
-                           "FROM pg_database d LEFT JOIN %s u ON (datdba = u.oid) "
-                           "WHERE datallowconn ORDER BY 1", role_catalog, role_catalog);
-     else if (server_version >= 80100)
-         printfPQExpBuffer(buf,
-                           "SELECT datname, "
-                           "coalesce(rolname, (select rolname from %s where oid=(select datdba from pg_database where
datname='template0')))," 
-                           "pg_encoding_to_char(d.encoding), "
-                           "null::text AS datcollate, null::text AS datctype, datfrozenxid, 0 AS datminmxid, "
-                           "datistemplate, datacl, '' as rdatacl, "
-                           "datconnlimit, "
-                           "(SELECT spcname FROM pg_tablespace t WHERE t.oid = d.dattablespace) AS dattablespace "
-                           "FROM pg_database d LEFT JOIN %s u ON (datdba = u.oid) "
-                           "WHERE datallowconn ORDER BY 1", role_catalog, role_catalog);
-     else
-         printfPQExpBuffer(buf,
-                           "SELECT datname, "
-                           "coalesce(usename, (select usename from pg_shadow where usesysid=(select datdba from
pg_databasewhere datname='template0'))), " 
-                           "pg_encoding_to_char(d.encoding), "
-                           "null::text AS datcollate, null::text AS datctype, datfrozenxid, 0 AS datminmxid, "
-                           "datistemplate, datacl, '' as rdatacl, "
-                           "-1 as datconnlimit, "
-                           "(SELECT spcname FROM pg_tablespace t WHERE t.oid = d.dattablespace) AS dattablespace "
-                           "FROM pg_database d LEFT JOIN pg_shadow u ON (datdba = usesysid) "
-                           "WHERE datallowconn ORDER BY 1");
-
-     res = executeQuery(conn, buf->data);
-
-     for (i = 0; i < PQntuples(res); i++)
-     {
-         char       *dbname = PQgetvalue(res, i, 0);
-         char       *dbowner = PQgetvalue(res, i, 1);
-         char       *dbencoding = PQgetvalue(res, i, 2);
-         char       *dbcollate = PQgetvalue(res, i, 3);
-         char       *dbctype = PQgetvalue(res, i, 4);
-         uint32        dbfrozenxid = atooid(PQgetvalue(res, i, 5));
-         uint32        dbminmxid = atooid(PQgetvalue(res, i, 6));
-         char       *dbistemplate = PQgetvalue(res, i, 7);
-         char       *dbacl = PQgetvalue(res, i, 8);
-         char       *rdbacl = PQgetvalue(res, i, 9);
-         char       *dbconnlimit = PQgetvalue(res, i, 10);
-         char       *dbtablespace = PQgetvalue(res, i, 11);
-         char       *fdbname;
-
-         fdbname = pg_strdup(fmtId(dbname));
-
-         resetPQExpBuffer(buf);
-
-         /*
-          * Skip the CREATE DATABASE commands for "template1" and "postgres",
-          * since they are presumably already there in the destination cluster.
-          * We do want to emit their ACLs and config options if any, however.
-          */
-         if (strcmp(dbname, "template1") != 0 &&
-             strcmp(dbname, "postgres") != 0)
-         {
-             appendPQExpBuffer(buf, "CREATE DATABASE %s", fdbname);
-
-             appendPQExpBufferStr(buf, " WITH TEMPLATE = template0");
-
-             if (strlen(dbowner) != 0)
-                 appendPQExpBuffer(buf, " OWNER = %s", fmtId(dbowner));
-
-             if (default_encoding && strcmp(dbencoding, default_encoding) != 0)
-             {
-                 appendPQExpBufferStr(buf, " ENCODING = ");
-                 appendStringLiteralConn(buf, dbencoding, conn);
-             }
-
-             if (default_collate && strcmp(dbcollate, default_collate) != 0)
-             {
-                 appendPQExpBufferStr(buf, " LC_COLLATE = ");
-                 appendStringLiteralConn(buf, dbcollate, conn);
-             }
-
-             if (default_ctype && strcmp(dbctype, default_ctype) != 0)
-             {
-                 appendPQExpBufferStr(buf, " LC_CTYPE = ");
-                 appendStringLiteralConn(buf, dbctype, conn);
-             }
-
-             /*
-              * Output tablespace if it isn't the default.  For default, it
-              * uses the default from the template database.  If tablespace is
-              * specified and tablespace creation failed earlier, (e.g. no such
-              * directory), the database creation will fail too.  One solution
-              * would be to use 'SET default_tablespace' like we do in pg_dump
-              * for setting non-default database locations.
-              */
-             if (strcmp(dbtablespace, "pg_default") != 0 && !no_tablespaces)
-                 appendPQExpBuffer(buf, " TABLESPACE = %s",
-                                   fmtId(dbtablespace));
-
-             if (strcmp(dbistemplate, "t") == 0)
-                 appendPQExpBuffer(buf, " IS_TEMPLATE = true");
-
-             if (strcmp(dbconnlimit, "-1") != 0)
-                 appendPQExpBuffer(buf, " CONNECTION LIMIT = %s",
-                                   dbconnlimit);
-
-             appendPQExpBufferStr(buf, ";\n");
-         }
-         else if (strcmp(dbtablespace, "pg_default") != 0 && !no_tablespaces)
-         {
-             /*
-              * Cannot change tablespace of the database we're connected to, so
-              * to move "postgres" to another tablespace, we connect to
-              * "template1", and vice versa.
-              */
-             if (strcmp(dbname, "postgres") == 0)
-                 appendPQExpBuffer(buf, "\\connect template1\n");
-             else
-                 appendPQExpBuffer(buf, "\\connect postgres\n");
-
-             appendPQExpBuffer(buf, "ALTER DATABASE %s SET TABLESPACE %s;\n",
-                               fdbname, fmtId(dbtablespace));
-
-             /* connect to original database */
-             appendPsqlMetaConnect(buf, dbname);
-         }
-
-         if (binary_upgrade)
-         {
-             appendPQExpBufferStr(buf, "-- For binary upgrade, set datfrozenxid and datminmxid.\n");
-             appendPQExpBuffer(buf, "UPDATE pg_catalog.pg_database "
-                               "SET datfrozenxid = '%u', datminmxid = '%u' "
-                               "WHERE datname = ",
-                               dbfrozenxid, dbminmxid);
-             appendStringLiteralConn(buf, dbname, conn);
-             appendPQExpBufferStr(buf, ";\n");
-         }
-
-         if (!skip_acls &&
-             !buildACLCommands(fdbname, NULL, "DATABASE",
-                               dbacl, rdbacl, dbowner,
-                               "", server_version, buf))
-         {
-             fprintf(stderr, _("%s: could not parse ACL list (%s) for database \"%s\"\n"),
-                     progname, dbacl, fdbname);
-             PQfinish(conn);
-             exit_nicely(1);
-         }
-
-         fprintf(OPF, "%s", buf->data);
-
-         dumpDatabaseConfig(conn, dbname);
-
-         free(fdbname);
-     }
-
-     if (default_encoding)
-         free(default_encoding);
-     if (default_collate)
-         free(default_collate);
-     if (default_ctype)
-         free(default_ctype);
-
-     PQclear(res);
-     destroyPQExpBuffer(buf);
-
-     fprintf(OPF, "\n\n");
- }
-
-
- /*
-  * Dump database-specific configuration
-  */
- static void
- dumpDatabaseConfig(PGconn *conn, const char *dbname)
- {
-     PQExpBuffer buf = createPQExpBuffer();
-     int            count = 1;
-
-     for (;;)
-     {
-         PGresult   *res;
-
-         if (server_version >= 90000)
-             printfPQExpBuffer(buf, "SELECT setconfig[%d] FROM pg_db_role_setting WHERE "
-                               "setrole = 0 AND setdatabase = (SELECT oid FROM pg_database WHERE datname = ", count);
-         else
-             printfPQExpBuffer(buf, "SELECT datconfig[%d] FROM pg_database WHERE datname = ", count);
-         appendStringLiteralConn(buf, dbname, conn);
-
-         if (server_version >= 90000)
-             appendPQExpBufferChar(buf, ')');
-
-         res = executeQuery(conn, buf->data);
-         if (PQntuples(res) == 1 &&
-             !PQgetisnull(res, 0, 0))
-         {
-             makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
-                                    "DATABASE", dbname, NULL, NULL);
-             PQclear(res);
-             count++;
-         }
-         else
-         {
-             PQclear(res);
-             break;
-         }
-     }
-
-     destroyPQExpBuffer(buf);
- }
-
-

  /*
   * Dump user-specific configuration
--- 1288,1293 ----
*************** dumpUserConfig(PGconn *conn, const char
*** 1649,1656 ****
          if (PQntuples(res) == 1 &&
              !PQgetisnull(res, 0, 0))
          {
              makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
!                                    "ROLE", username, NULL, NULL);
              PQclear(res);
              count++;
          }
--- 1318,1328 ----
          if (PQntuples(res) == 1 &&
              !PQgetisnull(res, 0, 0))
          {
+             resetPQExpBuffer(buf);
              makeAlterConfigCommand(conn, PQgetvalue(res, 0, 0),
!                                    "ROLE", username, NULL, NULL,
!                                    buf);
!             fprintf(OPF, "%s", buf->data);
              PQclear(res);
              count++;
          }
*************** dumpUserConfig(PGconn *conn, const char
*** 1666,1750 ****


  /*
-  * Dump user-and-database-specific configuration
-  */
- static void
- dumpDbRoleConfig(PGconn *conn)
- {
-     PQExpBuffer buf = createPQExpBuffer();
-     PGresult   *res;
-     int            i;
-
-     printfPQExpBuffer(buf, "SELECT rolname, datname, unnest(setconfig) "
-                       "FROM pg_db_role_setting, %s u, pg_database "
-                       "WHERE setrole = u.oid AND setdatabase = pg_database.oid", role_catalog);
-     res = executeQuery(conn, buf->data);
-
-     if (PQntuples(res) > 0)
-     {
-         fprintf(OPF, "--\n-- Per-Database Role Settings \n--\n\n");
-
-         for (i = 0; i < PQntuples(res); i++)
-         {
-             makeAlterConfigCommand(conn, PQgetvalue(res, i, 2),
-                                    "ROLE", PQgetvalue(res, i, 0),
-                                    "DATABASE", PQgetvalue(res, i, 1));
-         }
-
-         fprintf(OPF, "\n\n");
-     }
-
-     PQclear(res);
-     destroyPQExpBuffer(buf);
- }
-
-
- /*
-  * Helper function for dumpXXXConfig().
-  */
- static void
- makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
-                        const char *type, const char *name,
-                        const char *type2, const char *name2)
- {
-     char       *pos;
-     char       *mine;
-     PQExpBuffer buf;
-
-     mine = pg_strdup(arrayitem);
-     pos = strchr(mine, '=');
-     if (pos == NULL)
-     {
-         free(mine);
-         return;
-     }
-
-     buf = createPQExpBuffer();
-
-     *pos = 0;
-     appendPQExpBuffer(buf, "ALTER %s %s ", type, fmtId(name));
-     if (type2 != NULL && name2 != NULL)
-         appendPQExpBuffer(buf, "IN %s %s ", type2, fmtId(name2));
-     appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
-
-     /*
-      * Some GUC variable names are 'LIST' type and hence must not be quoted.
-      */
-     if (pg_strcasecmp(mine, "DateStyle") == 0
-         || pg_strcasecmp(mine, "search_path") == 0)
-         appendPQExpBufferStr(buf, pos + 1);
-     else
-         appendStringLiteralConn(buf, pos + 1, conn);
-     appendPQExpBufferStr(buf, ";\n");
-
-     fprintf(OPF, "%s", buf->data);
-     destroyPQExpBuffer(buf);
-     free(mine);
- }
-
-
-
- /*
   * Dump contents of databases.
   */
  static void
--- 1338,1343 ----
*************** dumpDatabases(PGconn *conn)
*** 1753,1790 ****
      PGresult   *res;
      int            i;

!     res = executeQuery(conn, "SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1");

      for (i = 0; i < PQntuples(res); i++)
      {
          int            ret;

!         char       *dbname = PQgetvalue(res, i, 0);
!         PQExpBufferData connectbuf;

          if (verbose)
              fprintf(stderr, _("%s: dumping database \"%s\"...\n"), progname, dbname);

-         initPQExpBuffer(&connectbuf);
-         appendPsqlMetaConnect(&connectbuf, dbname);
-         fprintf(OPF, "%s\n", connectbuf.data);
-         termPQExpBuffer(&connectbuf);
-
          /*
!          * 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");

          if (filename)
              fclose(OPF);

!         ret = runPgDump(dbname);
          if (ret != 0)
          {
              fprintf(stderr, _("%s: pg_dump failed on database \"%s\", exiting\n"), progname, dbname);
--- 1346,1407 ----
      PGresult   *res;
      int            i;

!     /*
!      * Skip databases marked not datallowconn, since we'd be unable to connect
!      * to them anyway.  This must agree with dropDBs().
!      *
!      * We arrange for template1 to be processed first, then we process other
!      * DBs in alphabetical order.  If we just did them all alphabetically, we
!      * might find ourselves trying to drop the "postgres" database while still
!      * connected to it.  This makes trying to run the restore script while
!      * connected to "template1" a bad idea, but there's no fixed order that
!      * doesn't have some failure mode with --clean.
!      */
!     res = executeQuery(conn,
!                        "SELECT datname "
!                        "FROM pg_database d "
!                        "WHERE datallowconn "
!                        "ORDER BY (datname <> 'template1'), datname");

      for (i = 0; i < PQntuples(res); i++)
      {
+         char       *dbname = PQgetvalue(res, i, 0);
+         const char *create_opts;
          int            ret;

!         /* Skip template0, even if it's not marked !datallowconn. */
!         if (strcmp(dbname, "template0") == 0)
!             continue;

          if (verbose)
              fprintf(stderr, _("%s: dumping database \"%s\"...\n"), progname, dbname);

          /*
!          * We assume that "template1" and "postgres" already exist in the
!          * target installation.  dropDBs() won't have removed them, for fear
!          * of removing the DB the restore script is initially connected to. If
!          * --clean was specified, tell pg_dump to drop and recreate them;
!          * otherwise we'll merely restore their contents.  Other databases
!          * should simply be created.
           */
!         if (strcmp(dbname, "template1") == 0 || strcmp(dbname, "postgres") == 0)
!         {
!             if (output_clean)
!                 create_opts = "--clean --create";
!             else
!             {
!                 create_opts = "";
!                 /* Since pg_dump won't emit a \connect command, we must */
!                 fprintf(OPF, "\\connect %s\n\n", dbname);
!             }
!         }
!         else
!             create_opts = "--create";

          if (filename)
              fclose(OPF);

!         ret = runPgDump(dbname, create_opts);
          if (ret != 0)
          {
              fprintf(stderr, _("%s: pg_dump failed on database \"%s\", exiting\n"), progname, dbname);
*************** dumpDatabases(PGconn *conn)
*** 1810,1826 ****


  /*
!  * Run pg_dump on dbname.
   */
  static int
! runPgDump(const char *dbname)
  {
      PQExpBuffer connstrbuf = createPQExpBuffer();
      PQExpBuffer cmd = createPQExpBuffer();
      int            ret;

!     appendPQExpBuffer(cmd, "\"%s\" %s", pg_dump_bin,
!                       pgdumpopts->data);

      /*
       * If we have a filename, use the undocumented plain-append pg_dump
--- 1427,1443 ----


  /*
!  * Run pg_dump on dbname, with specified options.
   */
  static int
! runPgDump(const char *dbname, const char *create_opts)
  {
      PQExpBuffer connstrbuf = createPQExpBuffer();
      PQExpBuffer cmd = createPQExpBuffer();
      int            ret;

!     appendPQExpBuffer(cmd, "\"%s\" %s %s", pg_dump_bin,
!                       pgdumpopts->data, create_opts);

      /*
       * If we have a filename, use the undocumented plain-append pg_dump
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index fce1465..74730bf 100644
*** a/src/bin/pg_dump/t/002_pg_dump.pl
--- b/src/bin/pg_dump/t/002_pg_dump.pl
*************** qr/^ALTER (?!EVENT TRIGGER|LARGE OBJECT|
*** 1516,1526 ****
          all_runs  => 1,
          catch_all => 'COMMENT commands',
          regexp    => qr/^COMMENT ON DATABASE postgres IS .*;/m,
!         like      => {
              binary_upgrade           => 1,
              clean                    => 1,
              clean_if_exists          => 1,
!             createdb                 => 1,
              defaults                 => 1,
              exclude_dump_test_schema => 1,
              exclude_test_table       => 1,
--- 1516,1529 ----
          all_runs  => 1,
          catch_all => 'COMMENT commands',
          regexp    => qr/^COMMENT ON DATABASE postgres IS .*;/m,
!         # Should appear in the same tests as "CREATE DATABASE postgres"
!         like   => { createdb => 1, },
!         unlike => {
              binary_upgrade           => 1,
              clean                    => 1,
              clean_if_exists          => 1,
!             column_inserts           => 1,
!             data_only                => 1,
              defaults                 => 1,
              exclude_dump_test_schema => 1,
              exclude_test_table       => 1,
*************** qr/^ALTER (?!EVENT TRIGGER|LARGE OBJECT|
*** 1528,1545 ****
              no_blobs                 => 1,
              no_privs                 => 1,
              no_owner                 => 1,
              pg_dumpall_dbprivs       => 1,
              schema_only              => 1,
              section_pre_data         => 1,
!             with_oids                => 1, },
!         unlike => {
!             column_inserts         => 1,
!             data_only              => 1,
!             only_dump_test_schema  => 1,
!             only_dump_test_table   => 1,
!             role                   => 1,
!             section_post_data      => 1,
!             test_schema_plus_blobs => 1, }, },

      'COMMENT ON EXTENSION plpgsql' => {
          all_runs  => 1,
--- 1531,1548 ----
              no_blobs                 => 1,
              no_privs                 => 1,
              no_owner                 => 1,
+             only_dump_test_schema    => 1,
+             only_dump_test_table     => 1,
              pg_dumpall_dbprivs       => 1,
+             pg_dumpall_globals       => 1,
+             pg_dumpall_globals_clean => 1,
+             role                     => 1,
              schema_only              => 1,
              section_pre_data         => 1,
!             section_data             => 1,
!             section_post_data        => 1,
!             test_schema_plus_blobs   => 1,
!             with_oids                => 1, }, },

      'COMMENT ON EXTENSION plpgsql' => {
          all_runs  => 1,
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 8726214..a67e484 100644
*** a/src/bin/pg_upgrade/pg_upgrade.c
--- b/src/bin/pg_upgrade/pg_upgrade.c
***************
*** 46,52 ****
  #endif

  static void prepare_new_cluster(void);
! static void prepare_new_databases(void);
  static void create_new_objects(void);
  static void copy_xact_xlog_xid(void);
  static void set_frozenxids(bool minmxid_only);
--- 46,52 ----
  #endif

  static void prepare_new_cluster(void);
! static void prepare_new_globals(void);
  static void create_new_objects(void);
  static void copy_xact_xlog_xid(void);
  static void set_frozenxids(bool minmxid_only);
*************** main(int argc, char **argv)
*** 124,130 ****
      /* -- NEW -- */
      start_postmaster(&new_cluster, true);

!     prepare_new_databases();

      create_new_objects();

--- 124,130 ----
      /* -- NEW -- */
      start_postmaster(&new_cluster, true);

!     prepare_new_globals();

      create_new_objects();

*************** prepare_new_cluster(void)
*** 271,277 ****


  static void
! prepare_new_databases(void)
  {
      /*
       * We set autovacuum_freeze_max_age to its maximum value so autovacuum
--- 271,277 ----


  static void
! prepare_new_globals(void)
  {
      /*
       * We set autovacuum_freeze_max_age to its maximum value so autovacuum
*************** prepare_new_databases(void)
*** 283,302 ****

      prep_status("Restoring global objects in the new cluster");

-     /*
-      * We have to create the databases first so we can install support
-      * functions in all the other databases.  Ideally we could create the
-      * support functions in template1 but pg_dumpall creates database using
-      * the template0 template.
-      */
      exec_prog(UTILITY_LOG_FILE, NULL, true, true,
                "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
                new_cluster.bindir, cluster_conn_opts(&new_cluster),
                GLOBALS_DUMP_FILE);
      check_ok();
-
-     /* we load this to get a current list of databases */
-     get_db_and_rel_infos(&new_cluster);
  }


--- 283,293 ----
*************** create_new_objects(void)
*** 312,344 ****
          char        sql_file_name[MAXPGPATH],
                      log_file_name[MAXPGPATH];
          DbInfo       *old_db = &old_cluster.dbarr.dbs[dbnum];
!         PQExpBufferData connstr,
!                     escaped_connstr;
!
!         initPQExpBuffer(&connstr);
!         appendPQExpBuffer(&connstr, "dbname=");
!         appendConnStrVal(&connstr, old_db->db_name);
!         initPQExpBuffer(&escaped_connstr);
!         appendShellString(&escaped_connstr, connstr.data);
!         termPQExpBuffer(&connstr);

          pg_log(PG_STATUS, "%s", old_db->db_name);
          snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
          snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);

          /*
!          * pg_dump only produces its output at the end, so there is little
!          * parallelism if using the pipe.
           */
          parallel_exec_prog(log_file_name,
                             NULL,
!                            "\"%s/pg_restore\" %s --exit-on-error --verbose --dbname %s \"%s\"",
                             new_cluster.bindir,
                             cluster_conn_opts(&new_cluster),
!                            escaped_connstr.data,
                             sql_file_name);
-
-         termPQExpBuffer(&escaped_connstr);
      }

      /* reap all children */
--- 303,342 ----
          char        sql_file_name[MAXPGPATH],
                      log_file_name[MAXPGPATH];
          DbInfo       *old_db = &old_cluster.dbarr.dbs[dbnum];
!         const char *create_opts;
!         const char *starting_db;

          pg_log(PG_STATUS, "%s", old_db->db_name);
          snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
          snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);

          /*
!          * template1 and postgres databases will already exist in the target
!          * installation, so tell pg_restore to drop and recreate them;
!          * otherwise we would fail to propagate their database-level
!          * properties.
           */
+         if (strcmp(old_db->db_name, "template1") == 0 ||
+             strcmp(old_db->db_name, "postgres") == 0)
+             create_opts = "--clean --create";
+         else
+             create_opts = "--create";
+
+         /* When processing template1, we can't connect there to start with */
+         if (strcmp(old_db->db_name, "template1") == 0)
+             starting_db = "postgres";
+         else
+             starting_db = "template1";
+
          parallel_exec_prog(log_file_name,
                             NULL,
!                            "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
!                            "--dbname %s \"%s\"",
                             new_cluster.bindir,
                             cluster_conn_opts(&new_cluster),
!                            create_opts,
!                            starting_db,
                             sql_file_name);
      }

      /* reap all children */
*************** create_new_objects(void)
*** 355,361 ****
      if (GET_MAJOR_VERSION(old_cluster.major_version) < 903)
          set_frozenxids(true);

!     /* regenerate now that we have objects in the databases */
      get_db_and_rel_infos(&new_cluster);
  }

--- 353,359 ----
      if (GET_MAJOR_VERSION(old_cluster.major_version) < 903)
          set_frozenxids(true);

!     /* update new_cluster info now that we have objects in the databases */
      get_db_and_rel_infos(&new_cluster);
  }

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 +

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