Re: pg_upgrade if 'postgres' database is dropped

pg_upgrade doesn't work if the 'postgres' database has been dropped in
the old cluster:

~/pgsql.master$ bin/pg_upgrade -b ~/pgsql.91stable/bin -B bin/ -d
~/pgsql.91stable/data -D data-upgraded/
Performing Consistency Checks
-----------------------------
Checking current, bin, and data directories ok
Checking cluster versions ok
Checking database user is a superuser ok
Checking for prepared transactions ok
Checking for reg* system OID user data types ok
Checking for contrib/isn with bigint-passing mismatch ok
Creating catalog dump ok
Checking for prepared transactions ok

New cluster database "postgres" does not exist in the old cluster
Failure, exiting

That's a bit unfortunate. We have some other tools that don't work
without the 'postgres' database, like 'reindexdb -all', but it would
still be nice if pg_upgrade did.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Tue, Oct 4, 2011 at 12:11 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> pg_upgrade doesn't work if the 'postgres' database has been dropped in the
> old cluster:
>
> ~/pgsql.master$ bin/pg_upgrade -b ~/pgsql.91stable/bin -B bin/ -d
> ~/pgsql.91stable/data -D data-upgraded/
> Performing Consistency Checks
> -----------------------------
> Checking current, bin, and data directories                 ok
> Checking cluster versions                                   ok
> Checking database user is a superuser                       ok
> Checking for prepared transactions                          ok
> Checking for reg* system OID user data types                ok
> Checking for contrib/isn with bigint-passing mismatch       ok
> Creating catalog dump                                       ok
> Checking for prepared transactions                          ok
>
> New cluster database "postgres" does not exist in the old cluster
> Failure, exiting
>
>
> That's a bit unfortunate. We have some other tools that don't work without
> the 'postgres' database, like 'reindexdb -all', but it would still be nice
> if pg_upgrade did.

+1. I think our usual policy is to try postgres first and then try
template1, so it would seem logical for pg_upgrade to do the same.

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

Robert Haas wrote:
> On Tue, Oct 4, 2011 at 12:11 PM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> > pg_upgrade doesn't work if the 'postgres' database has been dropped in the
> > old cluster:
> >
> > ~/pgsql.master$ bin/pg_upgrade -b ~/pgsql.91stable/bin -B bin/ -d
> > ~/pgsql.91stable/data -D data-upgraded/
> > Performing Consistency Checks
> > -----------------------------
> > Checking current, bin, and data directories ? ? ? ? ? ? ? ? ok
> > Checking cluster versions ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ok
> > Checking database user is a superuser ? ? ? ? ? ? ? ? ? ? ? ok
> > Checking for prepared transactions ? ? ? ? ? ? ? ? ? ? ? ? ?ok
> > Checking for reg* system OID user data types ? ? ? ? ? ? ? ?ok
> > Checking for contrib/isn with bigint-passing mismatch ? ? ? ok
> > Creating catalog dump ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ok
> > Checking for prepared transactions ? ? ? ? ? ? ? ? ? ? ? ? ?ok
> >
> > New cluster database "postgres" does not exist in the old cluster
> > Failure, exiting
> >
> >
> > That's a bit unfortunate. We have some other tools that don't work without
> > the 'postgres' database, like 'reindexdb -all', but it would still be nice
> > if pg_upgrade did.
>
> +1. I think our usual policy is to try postgres first and then try
> template1, so it would seem logical for pg_upgrade to do the same.

Well, it is a little tricky. The problem is that I am not just
connecting to a database --- I am creating support functions in the
database. Now, this is complex because template1 is the template for
new databases, except for pg_dump which uses template0.

So, it is going to be confusing to support both databases because there
is the cleanup details I have to document if I use template1.

Also, pg_dumpall unconditionally connects to the postgres database to
restore roles:

fprintf(OPF, "\\connect postgres\n\n");

We could connect to template1 for this, but I am not comfortable
changing this. And when pg_dumpall throws an error for a missing
postgres database, pg_upgrade is going to fail.

We started using the postgres database as a database for connections ---
do we want to change that? We certainly can't have the pg_dumpall
output _conditionally_ connecting to template1.

I am feeling this isn't worth pursuing.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

* Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> So, it is going to be confusing to support both databases because there
> is the cleanup details I have to document if I use template1.

Presumably there's some other database in the system besides template1
if postgres doesn't exist.. Would it be possible to just make it
configurable? Then the user could pick a non-template database. Having
it fail if the option isn't used and the default postgres isn't there is
fine, imv.

Thanks,

Stephen

Stephen Frost wrote:
-- Start of PGP signed section.
> * Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> > So, it is going to be confusing to support both databases because there
> > is the cleanup details I have to document if I use template1.
>
> Presumably there's some other database in the system besides template1
> if postgres doesn't exist.. Would it be possible to just make it
> configurable? Then the user could pick a non-template database. Having
> it fail if the option isn't used and the default postgres isn't there is
> fine, imv.

I have not seen enough demand to make this a user-visible configuration.
We can just tell them to create a postgres database. Frankly, they
would have had to _remove_ the postgres database after initdb for it not
to be there, and they are instructed to change nothing about the new
database.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

* Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> I have not seen enough demand to make this a user-visible configuration.
> We can just tell them to create a postgres database. Frankly, they
> would have had to _remove_ the postgres database after initdb for it not
> to be there, and they are instructed to change nothing about the new
> database.

Yes, they would have removed it because they didn't want it. As I
recall, part of the agreement to create an extra database by default was
that it could be removed if users didn't want it. Turning around and
then saying "but things won't work if it's not there" isn't exactly
supporting users who decide to remove it.

Regarding pg_dumpall and pg_restore, I'm pretty sure both of those can
be configured to connect to other databases instead, including for
globals.

Thanks,

Stephen

Stephen Frost wrote:
-- Start of PGP signed section.
> * Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> > I have not seen enough demand to make this a user-visible configuration.
> > We can just tell them to create a postgres database. Frankly, they
> > would have had to _remove_ the postgres database after initdb for it not
> > to be there, and they are instructed to change nothing about the new
> > database.
>
> Yes, they would have removed it because they didn't want it. As I
> recall, part of the agreement to create an extra database by default was
> that it could be removed if users didn't want it. Turning around and
> then saying "but things won't work if it's not there" isn't exactly
> supporting users who decide to remove it.

Well, you would have to remove it _after_ you did the pg_upgrade. Right
now if you do a normal dump/restore upgrade, you also have to re-remove
the postgres database. We don't have any mechanism to drop a database
as part of pg_dumpall's restore if it didn't exist in the old cluster.

> Regarding pg_dumpall and pg_restore, I'm pretty sure both of those can
> be configured to connect to other databases instead, including for
> globals.

Well, please show me the code, because the C code I showed you had the
'\connect postgres' string hardcoded in there.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

* Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> Well, you would have to remove it _after_ you did the pg_upgrade. Right
> now if you do a normal dump/restore upgrade, you also have to re-remove
> the postgres database. We don't have any mechanism to drop a database
> as part of pg_dumpall's restore if it didn't exist in the old cluster.

Perhaps not, but it *could* be removed after the restore and all would
be well, yes?

> > Regarding pg_dumpall and pg_restore, I'm pretty sure both of those can
> > be configured to connect to other databases instead, including for
> > globals.
>
> Well, please show me the code, because the C code I showed you had the
> '\connect postgres' string hardcoded in there.

I guess there's a difference between "can be used and will work
correctly, but might create some extra garbage" and "can't be used at
all". pg_dumpall has a -l option for connecting to whatever *existing*
database you have to pull the global data, and then it'll restore into a
clean initdb'd cluster, after which you could remove postgres.
Admittedly, if you initdb the cluster, drop postgres, and then try a
restore, it would fail. Personally, I'm not a big fan of that (why
don't we use what was passed in to -l for that..?), but, practically,
it's not that big a deal. I don't know many folks who are going to
restore a pg_dumpall dump into an existing configuration where they've
monkied with things (that could cause all kinds of other issues anyway,
role conflicts, etc).

If I understood correctly (perhaps I didn't..), is that pg_upgrade
doesn't have the pg_dumpall equivilant of the '-l' or '--database'
option, and that's what is at issue here.

Thanks,

Stephen

Stephen Frost wrote:
-- Start of PGP signed section.
> * Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> > Well, you would have to remove it _after_ you did the pg_upgrade. Right
> > now if you do a normal dump/restore upgrade, you also have to re-remove
> > the postgres database. We don't have any mechanism to drop a database
> > as part of pg_dumpall's restore if it didn't exist in the old cluster.
>
> Perhaps not, but it *could* be removed after the restore and all would
> be well, yes?

I am not sure how much pg_dump worries about removing system objects
during a restore --- I don't think it does that at all actually. I
thought we did that for plpgsql, but I don't see that in the C code now,
and testing doesn't show it being removed by pg_dump.

> > > Regarding pg_dumpall and pg_restore, I'm pretty sure both of those can
> > > be configured to connect to other databases instead, including for
> > > globals.
> >
> > Well, please show me the code, because the C code I showed you had the
> > '\connect postgres' string hardcoded in there.
>
> I guess there's a difference between "can be used and will work
> correctly, but might create some extra garbage" and "can't be used at
> all". pg_dumpall has a -l option for connecting to whatever *existing*
> database you have to pull the global data, and then it'll restore into a
> clean initdb'd cluster, after which you could remove postgres.

Keep in mind -l might connect to a specified database to do the dump,
but it will still connect to the 'postgres' database to recreate them.

> Admittedly, if you initdb the cluster, drop postgres, and then try a
> restore, it would fail. Personally, I'm not a big fan of that (why

Right, same with pg_upgrade.

> don't we use what was passed in to -l for that..?), but, practically,

No idea.

> it's not that big a deal. I don't know many folks who are going to
> restore a pg_dumpall dump into an existing configuration where they've
> monkied with things (that could cause all kinds of other issues anyway,
> role conflicts, etc).
>
> If I understood correctly (perhaps I didn't..), is that pg_upgrade
> doesn't have the pg_dumpall equivilant of the '-l' or '--database'
> option, and that's what is at issue here.

Well, I can modify pg_upgrade to connect to template1 easily (no need
for a switch) --- the problem is that pg_dumpall requires the 'postgres'
database to restore its dump file.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Stephen Frost wrote:
>> Yes, they would have removed it because they didn't want it. As I
>> recall, part of the agreement to create an extra database by default was
>> that it could be removed if users didn't want it. Turning around and
>> then saying "but things won't work if it's not there" isn't exactly
>> supporting users who decide to remove it.

> Well, you would have to remove it _after_ you did the pg_upgrade.

As far as the *target* cluster is concerned, I have no sympathy for
someone who messes with its contents before running pg_upgrade.
That's an RTFM matter: you're supposed to upgrade into a virgin
just-initdb'd cluster.

However, it would be nice if pg_upgrade supported transferring from a
*source* cluster that didn't have the postgres DB.

What about creating a new, single-purpose database in the source
cluster and then removing it again after we're done?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Stephen Frost wrote:
> >> Yes, they would have removed it because they didn't want it. As I
> >> recall, part of the agreement to create an extra database by default was
> >> that it could be removed if users didn't want it. Turning around and
> >> then saying "but things won't work if it's not there" isn't exactly
> >> supporting users who decide to remove it.
>
> > Well, you would have to remove it _after_ you did the pg_upgrade.
>
> As far as the *target* cluster is concerned, I have no sympathy for
> someone who messes with its contents before running pg_upgrade.
> That's an RTFM matter: you're supposed to upgrade into a virgin
> just-initdb'd cluster.
>
> However, it would be nice if pg_upgrade supported transferring from a
> *source* cluster that didn't have the postgres DB.

I have this C comment in pg_upgrade about this:

* If someone removes the 'postgres' database from the old cluster and
* the new cluster has a 'postgres' database, the number of databases
* will not match. We actually could upgrade such a setup, but it would
* violate the 1-to-1 mapping of database counts, so we throw an error
* instead. We would detect this as a database count mismatch during
* upgrade, but we want to detect it during the check phase and report
* the database name.

Is this worth fixing? Another problem is that pg_dumpall doesn't remove
the postgres database in the new cluster if it was not in the old one,
so they are going to end up with a postgres database in the new cluster
anyway. I could argue that pg_upgrade is better because reports it
cannot recreate the new cluster exactly, while pg_dumpall just keeps a
postgres database that was not in the old cluster.

> What about creating a new, single-purpose database in the source
> cluster and then removing it again after we're done?

That is not a problem --- I can easily use template1.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Oct 27, 2011 at 11:35 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> What about creating a new, single-purpose database in the source
>> cluster and then removing it again after we're done?
>
> That is not a problem --- I can easily use template1.

Huh?

You just said upthread that you didn't want to use template1 because
you didn't want to modify the template database. I think the point is
that if you're doing something to the database that someone might
object to, you oughtn't be doing it to the postgres database either.
You should create a database just for pg_upgrade's use and install its
crap in there.

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

Robert Haas wrote:
> On Thu, Oct 27, 2011 at 11:35 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> What about creating a new, single-purpose database in the source
> >> cluster and then removing it again after we're done?
> >
> > That is not a problem --- I can easily use template1.
>
> Huh?
>
> You just said upthread that you didn't want to use template1 because
> you didn't want to modify the template database. I think the point is

I don't want to use postgres and then fall back to template1 if
necessary --- I would just use template1 always.

> that if you're doing something to the database that someone might
> object to, you oughtn't be doing it to the postgres database either.
> You should create a database just for pg_upgrade's use and install its
> crap in there.

It installs crap in all databases to set oids on system tables, for
example, so we are only creating it early in postgres (or template1) to
set auth_id. Our sticking point now is that pg_dumpall has the
'postgres' database hardcoded for role creation.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Robert Haas wrote:
>> that if you're doing something to the database that someone might
>> object to, you oughtn't be doing it to the postgres database either.
>> You should create a database just for pg_upgrade's use and install its
>> crap in there.

> It installs crap in all databases to set oids on system tables,

It seems like you're both confusing the source and target clusters.
None of that stuff gets installed in the source, does it?

regards, tom lane

On Oct 28, 2011 5:22 AM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Stephen Frost wrote:
> >> Yes, they would have removed it because they didn't want it. As I
> >> recall, part of the agreement to create an extra database by default
was
> >> that it could be removed if users didn't want it. Turning around and
> >> then saying "but things won't work if it's not there" isn't exactly
> >> supporting users who decide to remove it.
>
> > Well, you would have to remove it _after_ you did the pg_upgrade.
>
> As far as the *target* cluster is concerned, I have no sympathy for
> someone who messes with its contents before running pg_upgrade.
> That's an RTFM matter: you're supposed to upgrade into a virgin
> just-initdb'd cluster.
>
> However, it would be nice if pg_upgrade supported transferring from a
> *source* cluster that didn't have the postgres DB.
>
> What about creating a new, single-purpose database in the source
> cluster and then removing it again after we're done?

How about naming this newly created database "postgres"? That would make the
code simple enough - always use the postgres database, just drop it at the
end if it didn't exist in the source cluster.

/Magnus

On Oct 28, 2011 5:19 AM, "Bruce Momjian" <bruce(at)momjian(dot)us> wrote:
>
> Stephen Frost wrote:
>
> > > > Regarding pg_dumpall and pg_restore, I'm pretty sure both of those
can
> > > > be configured to connect to other databases instead, including for
> > > > globals.
> > >
> > > Well, please show me the code, because the C code I showed you had the
> > > '\connect postgres' string hardcoded in there.
> >
> > I guess there's a difference between "can be used and will work
> > correctly, but might create some extra garbage" and "can't be used at
> > all". pg_dumpall has a -l option for connecting to whatever *existing*
> > database you have to pull the global data, and then it'll restore into a
> > clean initdb'd cluster, after which you could remove postgres.
>
> Keep in mind -l might connect to a specified database to do the dump,
> but it will still connect to the 'postgres' database to recreate them.
>
> > Admittedly, if you initdb the cluster, drop postgres, and then try a
> > restore, it would fail. Personally, I'm not a big fan of that (why
>
> Right, same with pg_upgrade.
>
> > don't we use what was passed in to -l for that..?), but, practically,
>
> No idea.

Chicken/egg? If we did that, the pg_dumpall dump could no longer be loaded
into an empty cluster since the db it wanted to talk to didn't exist yet.
And restoring into an empty cluster has to be the main use for pg_dumpall
after all....

/Magnus

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Robert Haas wrote:
> >> that if you're doing something to the database that someone might
> >> object to, you oughtn't be doing it to the postgres database either.
> >> You should create a database just for pg_upgrade's use and install its
> >> crap in there.
>
> > It installs crap in all databases to set oids on system tables,
>
> It seems like you're both confusing the source and target clusters.
> None of that stuff gets installed in the source, does it?

Right, only in the target.

Let me summarize:

The postgres database is required in the source because pg_upgrade likes
to have a 1-1 database mapping of old and new clusters. This can be
changed, but it makes pg_upgrade slightly more complex.

The postgres database is required on the target because pg_upgrade
creates the support functions first in that database. That can be
changed, but pg_dumpall restores roles in the postgres database by
default, not template1. Again, this can be changed.

Because of this 'postgres' new cluster requirement, you can't just
delete the postgres database from the new cluster and run pg_upgrade.

Tom wants pg_upgrade to work if the old cluster doesn't have a postgres
database. I see two solutions --- either remove the 1-1 mapping of
old/new databases, or remove the pg_upgrade and pg_dumpall dependence on
the postgres database and tell users to remove postgres from the new
cluster before the upgrade.

They already get a clear error message about the problem, which I think
is why we haven't seen more problem reports. My guess is they are just
creating the postgres database on the old cluster before the upgrade
after they get the error.

I have applied the attached patch to at least clarify that they need the
postgres database in the old cluster, rather than them trying to remove
the postgres database from the new cluster.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Magnus Hagander wrote:
> On Oct 28, 2011 5:22 AM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > Stephen Frost wrote:
> > >> Yes, they would have removed it because they didn't want it. As I
> > >> recall, part of the agreement to create an extra database by default
> was
> > >> that it could be removed if users didn't want it. Turning around and
> > >> then saying "but things won't work if it's not there" isn't exactly
> > >> supporting users who decide to remove it.
> >
> > > Well, you would have to remove it _after_ you did the pg_upgrade.
> >
> > As far as the *target* cluster is concerned, I have no sympathy for
> > someone who messes with its contents before running pg_upgrade.
> > That's an RTFM matter: you're supposed to upgrade into a virgin
> > just-initdb'd cluster.
> >
> > However, it would be nice if pg_upgrade supported transferring from a
> > *source* cluster that didn't have the postgres DB.
> >
> > What about creating a new, single-purpose database in the source
> > cluster and then removing it again after we're done?
>
> How about naming this newly created database "postgres"? That would make the
> code simple enough - always use the postgres database, just drop it at the
> end if it didn't exist in the source cluster.

Yes, that would work, but see my summarization email on this. Using
template1 is not a problem for pg_upgrade, it is the modifications to
pg_dumpall that are an issue.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Magnus Hagander wrote:
> On Oct 28, 2011 5:19 AM, "Bruce Momjian" <bruce(at)momjian(dot)us> wrote:
> >
> > Stephen Frost wrote:
> >
> > > > > Regarding pg_dumpall and pg_restore, I'm pretty sure both of those
> can
> > > > > be configured to connect to other databases instead, including for
> > > > > globals.
> > > >
> > > > Well, please show me the code, because the C code I showed you had the
> > > > '\connect postgres' string hardcoded in there.
> > >
> > > I guess there's a difference between "can be used and will work
> > > correctly, but might create some extra garbage" and "can't be used at
> > > all". pg_dumpall has a -l option for connecting to whatever *existing*
> > > database you have to pull the global data, and then it'll restore into a
> > > clean initdb'd cluster, after which you could remove postgres.
> >
> > Keep in mind -l might connect to a specified database to do the dump,
> > but it will still connect to the 'postgres' database to recreate them.
> >
> > > Admittedly, if you initdb the cluster, drop postgres, and then try a
> > > restore, it would fail. Personally, I'm not a big fan of that (why
> >
> > Right, same with pg_upgrade.
> >
> > > don't we use what was passed in to -l for that..?), but, practically,
> >
> > No idea.
>
> Chicken/egg? If we did that, the pg_dumpall dump could no longer be loaded
> into an empty cluster since the db it wanted to talk to didn't exist yet.
> And restoring into an empty cluster has to be the main use for pg_dumpall
> after all....

True. My assumption was that they had created some special database
before they did the pg_dumpall restore, but it would be odd because the
database would have been hard-coded into the dump, which isn't good.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Oct 28, 2011 at 8:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Yes, that would work, but see my summarization email on this.  Using
> template1 is not a problem for pg_upgrade, it is the modifications to
> pg_dumpall that are an issue.

I just did a bit of testing on this. It appears that pg_dumpall, if
given a cluster containing no postgres database, will happily try to
connect to template1 instead. If template1 isn't available either,
you can use "-l SOMEDBNAME" to specify the name of another database to
connect to instead. So there is infinite flexibility there.

But regardless of which database it uses to *generate* the dump, the
dump itself will *always* contain this, right at the very beginning:

\connect postgres

That line is in fact hard-coded as a literal string in pg_dumpall.c.
It seems like the easiest fix here might be to just remove that line
from the dump, because AFAICS it's completely pointless. During the
time for which that setting is in effect, we're just restoring
globals, so it shouldn't matter which database we're connected to;
only that we have a valid connection. So trying to switch the
connection from whatever the user is connected to currently to
postgres doesn't accomplish anything useful, but it does make it
possible for dump restoration to unnecessarily fail.

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

On 28 October 2011 14:28, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Oct 28, 2011 at 8:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> Yes, that would work, but see my summarization email on this.  Using
>> template1 is not a problem for pg_upgrade, it is the modifications to
>> pg_dumpall that are an issue.
>
> I just did a bit of testing on this.  It appears that pg_dumpall, if
> given a cluster containing no postgres database, will happily try to
> connect to template1 instead.  If template1 isn't available either,
> you can use "-l SOMEDBNAME" to specify the name of another database to
> connect to instead.  So there is infinite flexibility there.
>
> But regardless of which database it uses to *generate* the dump, the
> dump itself will *always* contain this, right at the very beginning:
>
> \connect postgres
>
> That line is in fact hard-coded as a literal string in pg_dumpall.c.
> It seems like the easiest fix here might be to just remove that line
> from the dump, because AFAICS it's completely pointless.  During the
> time for which that setting is in effect, we're just restoring
> globals, so it shouldn't matter which database we're connected to;
> only that we have a valid connection.  So trying to switch the
> connection from whatever the user is connected to currently to
> postgres doesn't accomplish anything useful, but it does make it
> possible for dump restoration to unnecessarily fail.

This was the kind of qualm I had with createdb, which I submitted a
patch for earlier this year:
http://archives.postgresql.org/pgsql-hackers/2011-03/msg00999.php

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Fri, Oct 28, 2011 at 8:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Yes, that would work, but see my summarization email on this. ?Using
> > template1 is not a problem for pg_upgrade, it is the modifications to
> > pg_dumpall that are an issue.
>
> I just did a bit of testing on this. It appears that pg_dumpall, if
> given a cluster containing no postgres database, will happily try to
> connect to template1 instead. If template1 isn't available either,
> you can use "-l SOMEDBNAME" to specify the name of another database to
> connect to instead. So there is infinite flexibility there.
>
> But regardless of which database it uses to *generate* the dump, the
> dump itself will *always* contain this, right at the very beginning:
>
> \connect postgres
>
> That line is in fact hard-coded as a literal string in pg_dumpall.c.
> It seems like the easiest fix here might be to just remove that line
> from the dump, because AFAICS it's completely pointless. During the
> time for which that setting is in effect, we're just restoring
> globals, so it shouldn't matter which database we're connected to;
> only that we have a valid connection. So trying to switch the
> connection from whatever the user is connected to currently to
> postgres doesn't accomplish anything useful, but it does make it
> possible for dump restoration to unnecessarily fail.

If you remove that line, I can modify pg_upgrade to use template1
instead of postgres, and then the user should just remove the postgres
database from the new cluster before the upgrade --- we can give them a
clear error message on that.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Oct 28, 2011 at 9:55 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Fri, Oct 28, 2011 at 8:12 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > Yes, that would work, but see my summarization email on this. ?Using
>> > template1 is not a problem for pg_upgrade, it is the modifications to
>> > pg_dumpall that are an issue.
>>
>> I just did a bit of testing on this.  It appears that pg_dumpall, if
>> given a cluster containing no postgres database, will happily try to
>> connect to template1 instead.  If template1 isn't available either,
>> you can use "-l SOMEDBNAME" to specify the name of another database to
>> connect to instead.  So there is infinite flexibility there.
>>
>> But regardless of which database it uses to *generate* the dump, the
>> dump itself will *always* contain this, right at the very beginning:
>>
>> \connect postgres
>>
>> That line is in fact hard-coded as a literal string in pg_dumpall.c.
>> It seems like the easiest fix here might be to just remove that line
>> from the dump, because AFAICS it's completely pointless.  During the
>> time for which that setting is in effect, we're just restoring
>> globals, so it shouldn't matter which database we're connected to;
>> only that we have a valid connection.  So trying to switch the
>> connection from whatever the user is connected to currently to
>> postgres doesn't accomplish anything useful, but it does make it
>> possible for dump restoration to unnecessarily fail.
>
> If you remove that line,

I'm happy to do that, unless someone can see a hole in my logic.

> I can modify pg_upgrade to use template1
> instead of postgres, and then the user should just remove the postgres
> database from the new cluster before the upgrade --- we can give them a
> clear error message on that.

What I would prefer is to have the upgrade succeed, and just ignore
the existence of a postgres database in the new cluster. Maybe give
the user a notice and let them decide whether they wish to take any
action. I understand that failing is probably less code, but IMHO one
of the biggest problems with pg_upgrade is that it's too fragile:
there are too many seemingly innocent things that can make it croak
(which isn't good, when you consider that anyone using pg_upgrade is
probably in a hurry to get the upgrade done and the database back
on-line). It seems like this is an opportunity to get rid of one of
those unnecessary failure cases.

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

Robert Haas wrote:
> >> But regardless of which database it uses to *generate* the dump, the
> >> dump itself will *always* contain this, right at the very beginning:
> >>
> >> \connect postgres
> >>
> >> That line is in fact hard-coded as a literal string in pg_dumpall.c.
> >> It seems like the easiest fix here might be to just remove that line
> >> from the dump, because AFAICS it's completely pointless. ?During the
> >> time for which that setting is in effect, we're just restoring
> >> globals, so it shouldn't matter which database we're connected to;
> >> only that we have a valid connection. ?So trying to switch the
> >> connection from whatever the user is connected to currently to
> >> postgres doesn't accomplish anything useful, but it does make it
> >> possible for dump restoration to unnecessarily fail.
> >
> > If you remove that line,
>
> I'm happy to do that, unless someone can see a hole in my logic.
>
> > I can modify pg_upgrade to use template1
> > instead of postgres, and then the user should just remove the postgres
> > database from the new cluster before the upgrade --- we can give them a
> > clear error message on that.
>
> What I would prefer is to have the upgrade succeed, and just ignore
> the existence of a postgres database in the new cluster. Maybe give
> the user a notice and let them decide whether they wish to take any
> action. I understand that failing is probably less code, but IMHO one
> of the biggest problems with pg_upgrade is that it's too fragile:
> there are too many seemingly innocent things that can make it croak
> (which isn't good, when you consider that anyone using pg_upgrade is
> probably in a hurry to get the upgrade done and the database back
> on-line). It seems like this is an opportunity to get rid of one of
> those unnecessary failure cases.

OK, then the simplest fix, once you modify pg_dumpall, would be to
modify pg_upgrade to remove reference to the postgres database in the
new cluster if it doesn't exist in the old one. That would allow
pg_upgrade to maintain a 1-1 matching of databases in the old and new
cluster --- it allows the change to be locallized without affecting much
code.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Robert Haas wrote:
> action. I understand that failing is probably less code, but IMHO one
> of the biggest problems with pg_upgrade is that it's too fragile:
> there are too many seemingly innocent things that can make it croak
> (which isn't good, when you consider that anyone using pg_upgrade is
> probably in a hurry to get the upgrade done and the database back
> on-line). It seems like this is an opportunity to get rid of one of
> those unnecessary failure cases.

FYI, the original design goal of pg_upgrade was to be do reliable
upgrades and fail at the hint of any inconsistency. Seems it is time to
adjust its goals.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Oct 28, 2011 at 10:07 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> OK, then the simplest fix, once you modify pg_dumpall, would be to
> modify pg_upgrade to remove reference to the postgres database in the
> new cluster if it doesn't exist in the old one.  That would allow
> pg_upgrade to maintain a 1-1 matching of databases in the old and new
> cluster --- it allows the change to be locallized without affecting much
> code.

That sounds just fine. +1.

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

On Fri, Oct 28, 2011 at 10:09 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> action.  I understand that failing is probably less code, but IMHO one
>> of the biggest problems with pg_upgrade is that it's too fragile:
>> there are too many seemingly innocent things that can make it croak
>> (which isn't good, when you consider that anyone using pg_upgrade is
>> probably in a hurry to get the upgrade done and the database back
>> on-line).  It seems like this is an opportunity to get rid of one of
>> those unnecessary failure cases.
>
> FYI, the original design goal of pg_upgrade was to be do reliable
> upgrades and fail at the hint of any inconsistency.  Seems it is time to
> adjust its goals.

We definitely don't want it to do anything that could compromise data
integrity. But in this case there seems no risk of that, so it seems
we can have our cake and eat it, too.

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

Robert Haas wrote:
> On Fri, Oct 28, 2011 at 10:07 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > OK, then the simplest fix, once you modify pg_dumpall, would be to
> > modify pg_upgrade to remove reference to the postgres database in the
> > new cluster if it doesn't exist in the old one. ?That would allow
> > pg_upgrade to maintain a 1-1 matching of databases in the old and new
> > cluster --- it allows the change to be locallized without affecting much
> > code.
>
> That sounds just fine. +1.

FYI, I don't want to modify pg_dumpall myself because I didn't want to
have pg_upgrade forcing a pg_dumpall change that applies to
non-binary-upgrade dumps. pg_dumpall is too important. I am fine if
someone else does it, though. :-)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Robert Haas wrote:
> On Fri, Oct 28, 2011 at 10:09 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> action. ?I understand that failing is probably less code, but IMHO one
> >> of the biggest problems with pg_upgrade is that it's too fragile:
> >> there are too many seemingly innocent things that can make it croak
> >> (which isn't good, when you consider that anyone using pg_upgrade is
> >> probably in a hurry to get the upgrade done and the database back
> >> on-line). ?It seems like this is an opportunity to get rid of one of
> >> those unnecessary failure cases.
> >
> > FYI, the original design goal of pg_upgrade was to be do reliable
> > upgrades and fail at the hint of any inconsistency. ?Seems it is time to
> > adjust its goals.
>
> We definitely don't want it to do anything that could compromise data
> integrity. But in this case there seems no risk of that, so it seems
> we can have our cake and eat it, too.

Agreed. I was extra cautious.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> Robert Haas wrote:
> > On Fri, Oct 28, 2011 at 10:07 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > > OK, then the simplest fix, once you modify pg_dumpall, would be to
> > > modify pg_upgrade to remove reference to the postgres database in the
> > > new cluster if it doesn't exist in the old one. ?That would allow
> > > pg_upgrade to maintain a 1-1 matching of databases in the old and new
> > > cluster --- it allows the change to be locallized without affecting much
> > > code.
> >
> > That sounds just fine. +1.
>
> FYI, I don't want to modify pg_dumpall myself because I didn't want to
> have pg_upgrade forcing a pg_dumpall change that applies to
> non-binary-upgrade dumps. pg_dumpall is too important. I am fine if
> someone else does it, though. :-)

If you want crazy, you could suppress the "\connect postgres" line only
in --binary-upgrade dumps, but that seems to error-prone because then
only --binary-upgrade dumps are exercising that behavior.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Oct 28, 2011 at 10:16 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Fri, Oct 28, 2011 at 10:07 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > OK, then the simplest fix, once you modify pg_dumpall, would be to
>> > modify pg_upgrade to remove reference to the postgres database in the
>> > new cluster if it doesn't exist in the old one. ?That would allow
>> > pg_upgrade to maintain a 1-1 matching of databases in the old and new
>> > cluster --- it allows the change to be locallized without affecting much
>> > code.
>>
>> That sounds just fine.  +1.
>
> FYI, I don't want to modify pg_dumpall myself because I didn't want to
> have pg_upgrade forcing a pg_dumpall change that applies to
> non-binary-upgrade dumps.  pg_dumpall is too important.  I am fine if
> someone else does it, though.  :-)

OK, done.

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

On Fri, Oct 28, 2011 at 10:18 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Bruce Momjian wrote:
>> Robert Haas wrote:
>> > On Fri, Oct 28, 2011 at 10:07 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > > OK, then the simplest fix, once you modify pg_dumpall, would be to
>> > > modify pg_upgrade to remove reference to the postgres database in the
>> > > new cluster if it doesn't exist in the old one. ?That would allow
>> > > pg_upgrade to maintain a 1-1 matching of databases in the old and new
>> > > cluster --- it allows the change to be locallized without affecting much
>> > > code.
>> >
>> > That sounds just fine.  +1.
>>
>> FYI, I don't want to modify pg_dumpall myself because I didn't want to
>> have pg_upgrade forcing a pg_dumpall change that applies to
>> non-binary-upgrade dumps.  pg_dumpall is too important.  I am fine if
>> someone else does it, though.  :-)
>
> If you want crazy, you could suppress the "\connect postgres" line only
> in --binary-upgrade dumps, but that seems to error-prone because then
> only --binary-upgrade dumps are exercising that behavior.

Also, then it would still be doing something silly in the non
--binary-upgrade case.

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

Robert Haas wrote:
> On Fri, Oct 28, 2011 at 10:16 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> On Fri, Oct 28, 2011 at 10:07 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> > OK, then the simplest fix, once you modify pg_dumpall, would be to
> >> > modify pg_upgrade to remove reference to the postgres database in the
> >> > new cluster if it doesn't exist in the old one. ?That would allow
> >> > pg_upgrade to maintain a 1-1 matching of databases in the old and new
> >> > cluster --- it allows the change to be locallized without affecting much
> >> > code.
> >>
> >> That sounds just fine. ?+1.
> >
> > FYI, I don't want to modify pg_dumpall myself because I didn't want to
> > have pg_upgrade forcing a pg_dumpall change that applies to
> > non-binary-upgrade dumps. ?pg_dumpall is too important. ?I am fine if
> > someone else does it, though. ?:-)
>
> OK, done.

OK, the attached, applied patch removes the pg_upgrade dependency on the
'postgres' database existing in the new cluster. However, vacuumdb,
used by pg_upgrade, still has this dependency:

conn = connectDatabase("postgres", host, port, username,
prompt_password, progname);

In fact, all the /scripts binaries use the postgres database, except for
createdb/dropdb, which has this heuristic:

/*
* Connect to the 'postgres' database by default, except have the
* 'postgres' user use 'template1' so he can create the 'postgres'
* database.
*/
conn = connectDatabase(strcmp(dbname, "postgres") == 0 ? "template1" : "postgres",
host, port, username, prompt_password, progname);

This makes sense because you might be creating or dropping the postgres
database. Do we want these to have smarter database selection code?

I will now work on code to allow the old cluster to optionally not have
a postgres database.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Oct 28, 2011 at 9:22 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> OK, the attached, applied patch removes the pg_upgrade dependency on the
> 'postgres' database existing in the new cluster.  However, vacuumdb,
> used by pg_upgrade, still has this dependency:
>
>            conn = connectDatabase("postgres", host, port, username,
>                prompt_password, progname);
>
> In fact, all the /scripts binaries use the postgres database, except for
> createdb/dropdb, which has this heuristic:
>
>    /*
>     * Connect to the 'postgres' database by default, except have the
>     * 'postgres' user use 'template1' so he can create the 'postgres'
>     * database.
>     */
>    conn = connectDatabase(strcmp(dbname, "postgres") == 0 ? "template1" : "postgres",
>                           host, port, username, prompt_password, progname);
>
> This makes sense because you might be creating or dropping the postgres
> database.  Do we want these to have smarter database selection code?

Well, I suppose as long as we're cleaning this up, we might as well be
thorough, so, sure, why not? I think the algorithm pg_dumpall uses is
pretty sensible: let the user specify the database to use if they so
desire; if not, try postgres first and then template1. I think we
could stick some logic for that in common.c which could be shared by
clusterdb, createdb, dropdb, dropuser, reindexdb, and vacuumdb.

However, we need to rethink the flag to be used for this: pg_dumpall
uses -l, but many of the other utilities already use that for some
other purpose, and it's not exactly mnemonic anyway. "-d" for
database could work, but that's also in use in some places, and
furthermore somewhat confusing since many if not all of these
utilities have an option to operate on a single database only, and you
might think that -d would specify the database to operate on, rather
than the one to be used to get the list of databases. pgAdmin uses
the term "maintenance database" to refer to a database to be used when
none is explicitly specified, and I think that's fairly clear
terminology. So I propose that we add a --maintenance-db option (with
no short form, since this is a relatively obscure need) to the tools
listed above. The tools will pass the associated value (or NULL if
the option is not specified) to the above-mentioned routine in
common.c, which will do the rest.

If nobody objects, I'll go do that. Hopefully that should be enough
to put this problem to bed more or less permanently.

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

Bruce Momjian wrote:
> > What I would prefer is to have the upgrade succeed, and just ignore
> > the existence of a postgres database in the new cluster. Maybe give
> > the user a notice and let them decide whether they wish to take any
> > action. I understand that failing is probably less code, but IMHO one
> > of the biggest problems with pg_upgrade is that it's too fragile:
> > there are too many seemingly innocent things that can make it croak
> > (which isn't good, when you consider that anyone using pg_upgrade is
> > probably in a hurry to get the upgrade done and the database back
> > on-line). It seems like this is an opportunity to get rid of one of
> > those unnecessary failure cases.
>
> OK, then the simplest fix, once you modify pg_dumpall, would be to
> modify pg_upgrade to remove reference to the postgres database in the
> new cluster if it doesn't exist in the old one. That would allow
> pg_upgrade to maintain a 1-1 matching of databases in the old and new
> cluster --- it allows the change to be locallized without affecting much
> code.

I fixed this a different way. I originally thought I could skip over
the 'postgres' database in the new cluster if it didn't exist in the old
cluster, but we have do things like check it is empty, so that was going
to be awkward.

It turns out there was only one place that expected a 1-1 mapping of old
and new databases (file transfer), so I just modified that code to allow
skipping a database in the new cluster that didn't exist in the old
cluster.

Attached patch applied. This allows an upgrade if the 'postgres'
database is missing from the old cluster.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Nov 1, 2011 at 1:53 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Bruce Momjian wrote:
>> > What I would prefer is to have the upgrade succeed, and just ignore
>> > the existence of a postgres database in the new cluster.  Maybe give
>> > the user a notice and let them decide whether they wish to take any
>> > action.  I understand that failing is probably less code, but IMHO one
>> > of the biggest problems with pg_upgrade is that it's too fragile:
>> > there are too many seemingly innocent things that can make it croak
>> > (which isn't good, when you consider that anyone using pg_upgrade is
>> > probably in a hurry to get the upgrade done and the database back
>> > on-line).  It seems like this is an opportunity to get rid of one of
>> > those unnecessary failure cases.
>>
>> OK, then the simplest fix, once you modify pg_dumpall, would be to
>> modify pg_upgrade to remove reference to the postgres database in the
>> new cluster if it doesn't exist in the old one.  That would allow
>> pg_upgrade to maintain a 1-1 matching of databases in the old and new
>> cluster --- it allows the change to be locallized without affecting much
>> code.
>
> I fixed this a different way.  I originally thought I could skip over
> the 'postgres' database in the new cluster if it didn't exist in the old
> cluster, but we have do things like check it is empty, so that was going
> to be awkward.
>
> It turns out there was only one place that expected a 1-1 mapping of old
> and new databases (file transfer), so I just modified that code to allow
> skipping a database in the new cluster that didn't exist in the old
> cluster.

Urp. But that means that if someone has any data in that database,
pg_upgrade will basically eat it. That does not seem like a step
forward.

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

Robert Haas wrote:
> On Tue, Nov 1, 2011 at 1:53 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Bruce Momjian wrote:
> >> > What I would prefer is to have the upgrade succeed, and just ignore
> >> > the existence of a postgres database in the new cluster. ?Maybe give
> >> > the user a notice and let them decide whether they wish to take any
> >> > action. ?I understand that failing is probably less code, but IMHO one
> >> > of the biggest problems with pg_upgrade is that it's too fragile:
> >> > there are too many seemingly innocent things that can make it croak
> >> > (which isn't good, when you consider that anyone using pg_upgrade is
> >> > probably in a hurry to get the upgrade done and the database back
> >> > on-line). ?It seems like this is an opportunity to get rid of one of
> >> > those unnecessary failure cases.
> >>
> >> OK, then the simplest fix, once you modify pg_dumpall, would be to
> >> modify pg_upgrade to remove reference to the postgres database in the
> >> new cluster if it doesn't exist in the old one. ?That would allow
> >> pg_upgrade to maintain a 1-1 matching of databases in the old and new
> >> cluster --- it allows the change to be locallized without affecting much
> >> code.
> >
> > I fixed this a different way. ?I originally thought I could skip over
> > the 'postgres' database in the new cluster if it didn't exist in the old
> > cluster, but we have do things like check it is empty, so that was going
> > to be awkward.
> >
> > It turns out there was only one place that expected a 1-1 mapping of old
> > and new databases (file transfer), so I just modified that code to allow
> > skipping a database in the new cluster that didn't exist in the old
> > cluster.
>
> Urp. But that means that if someone has any data in that database,
> pg_upgrade will basically eat it. That does not seem like a step
> forward.

Please clarify? We already check that all the new cluster databases are
empty, so we are effectively skipping the transfering of files into
empty new cluster databases. It processes all old cluster databases and
forces a new cluster match --- it is only empty new cluster database
that are being skipped.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Nov 1, 2011 at 2:36 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Tue, Nov 1, 2011 at 1:53 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > Bruce Momjian wrote:
>> >> > What I would prefer is to have the upgrade succeed, and just ignore
>> >> > the existence of a postgres database in the new cluster. ?Maybe give
>> >> > the user a notice and let them decide whether they wish to take any
>> >> > action. ?I understand that failing is probably less code, but IMHO one
>> >> > of the biggest problems with pg_upgrade is that it's too fragile:
>> >> > there are too many seemingly innocent things that can make it croak
>> >> > (which isn't good, when you consider that anyone using pg_upgrade is
>> >> > probably in a hurry to get the upgrade done and the database back
>> >> > on-line). ?It seems like this is an opportunity to get rid of one of
>> >> > those unnecessary failure cases.
>> >>
>> >> OK, then the simplest fix, once you modify pg_dumpall, would be to
>> >> modify pg_upgrade to remove reference to the postgres database in the
>> >> new cluster if it doesn't exist in the old one. ?That would allow
>> >> pg_upgrade to maintain a 1-1 matching of databases in the old and new
>> >> cluster --- it allows the change to be locallized without affecting much
>> >> code.
>> >
>> > I fixed this a different way. ?I originally thought I could skip over
>> > the 'postgres' database in the new cluster if it didn't exist in the old
>> > cluster, but we have do things like check it is empty, so that was going
>> > to be awkward.
>> >
>> > It turns out there was only one place that expected a 1-1 mapping of old
>> > and new databases (file transfer), so I just modified that code to allow
>> > skipping a database in the new cluster that didn't exist in the old
>> > cluster.
>>
>> Urp.  But that means that if someone has any data in that database,
>> pg_upgrade will basically eat it.  That does not seem like a step
>> forward.
>
> Please clarify?  We already check that all the new cluster databases are
> empty, so we are effectively skipping the transfering of files into
> empty new cluster databases.  It processes all old cluster databases and
> forces a new cluster match --- it is only empty new cluster database
> that are being skipped.

Aren't you saying that if a postgres database exists in the old
database (and potentially contains data) but is missing in the new
database, we'll just fail to migrate it?

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

Robert Haas wrote:
> >> > It turns out there was only one place that expected a 1-1 mapping of old
> >> > and new databases (file transfer), so I just modified that code to allow
> >> > skipping a database in the new cluster that didn't exist in the old
> >> > cluster.
> >>
> >> Urp. ?But that means that if someone has any data in that database,
> >> pg_upgrade will basically eat it. ?That does not seem like a step
> >> forward.
> >
> > Please clarify? ?We already check that all the new cluster databases are
> > empty, so we are effectively skipping the transfering of files into
> > empty new cluster databases. ?It processes all old cluster databases and
> > forces a new cluster match --- it is only empty new cluster database
> > that are being skipped.
>
> Aren't you saying that if a postgres database exists in the old
> database (and potentially contains data) but is missing in the new
> database, we'll just fail to migrate it?

No, the reverse. If the 'postgres' database exists in the new cluster,
but not in the old, we allow it to upgrade (we skip over the 'postgres'
database in the new cluster use the loop in the patch).

Unless I am missing something. Did you see something odd in the patch
or in my wording?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Nov 1, 2011 at 2:49 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> >> > It turns out there was only one place that expected a 1-1 mapping of old
>> >> > and new databases (file transfer), so I just modified that code to allow
>> >> > skipping a database in the new cluster that didn't exist in the old
>> >> > cluster.
>> >>
>> >> Urp. ?But that means that if someone has any data in that database,
>> >> pg_upgrade will basically eat it. ?That does not seem like a step
>> >> forward.
>> >
>> > Please clarify? ?We already check that all the new cluster databases are
>> > empty, so we are effectively skipping the transfering of files into
>> > empty new cluster databases. ?It processes all old cluster databases and
>> > forces a new cluster match --- it is only empty new cluster database
>> > that are being skipped.
>>
>> Aren't you saying that if a postgres database exists in the old
>> database (and potentially contains data) but is missing in the new
>> database, we'll just fail to migrate it?
>
> No, the reverse.  If the 'postgres' database exists in the new cluster,
> but not in the old, we allow it to upgrade (we skip over the 'postgres'
> database in the new cluster use the loop in the patch).

Oh, OK. That seems fine - in fact, that seems perfect.

> Unless I am missing something.  Did you see something odd in the patch
> or in my wording?

Your wording confused me, but on further review I think I'm just
easily confused.

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

Robert Haas wrote:
> On Tue, Nov 1, 2011 at 2:49 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> >> > It turns out there was only one place that expected a 1-1 mapping of old
> >> >> > and new databases (file transfer), so I just modified that code to allow
> >> >> > skipping a database in the new cluster that didn't exist in the old
> >> >> > cluster.
> >> >>
> >> >> Urp. ?But that means that if someone has any data in that database,
> >> >> pg_upgrade will basically eat it. ?That does not seem like a step
> >> >> forward.
> >> >
> >> > Please clarify? ?We already check that all the new cluster databases are
> >> > empty, so we are effectively skipping the transfering of files into
> >> > empty new cluster databases. ?It processes all old cluster databases and
> >> > forces a new cluster match --- it is only empty new cluster database
> >> > that are being skipped.
> >>
> >> Aren't you saying that if a postgres database exists in the old
> >> database (and potentially contains data) but is missing in the new
> >> database, we'll just fail to migrate it?
> >
> > No, the reverse. ?If the 'postgres' database exists in the new cluster,
> > but not in the old, we allow it to upgrade (we skip over the 'postgres'
> > database in the new cluster use the loop in the patch).
>
> Oh, OK. That seems fine - in fact, that seems perfect.
>
> > Unless I am missing something. ?Did you see something odd in the patch
> > or in my wording?
>
> Your wording confused me, but on further review I think I'm just
> easily confused.

The concept is pretty confusing and I had to think a while before I came
up with this approach.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Sat, Oct 29, 2011 at 4:07 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Oct 28, 2011 at 9:22 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> OK, the attached, applied patch removes the pg_upgrade dependency on the
>> 'postgres' database existing in the new cluster.  However, vacuumdb,
>> used by pg_upgrade, still has this dependency:
>>
>>            conn = connectDatabase("postgres", host, port, username,
>>                prompt_password, progname);
>>
>> In fact, all the /scripts binaries use the postgres database, except for
>> createdb/dropdb, which has this heuristic:
>>
>>    /*
>>     * Connect to the 'postgres' database by default, except have the
>>     * 'postgres' user use 'template1' so he can create the 'postgres'
>>     * database.
>>     */
>>    conn = connectDatabase(strcmp(dbname, "postgres") == 0 ? "template1" : "postgres",
>>                           host, port, username, prompt_password, progname);
>>
>> This makes sense because you might be creating or dropping the postgres
>> database.  Do we want these to have smarter database selection code?
>
> Well, I suppose as long as we're cleaning this up, we might as well be
> thorough, so, sure, why not?  I think the algorithm pg_dumpall uses is
> pretty sensible: let the user specify the database to use if they so
> desire; if not, try postgres first and then template1.  I think we
> could stick some logic for that in common.c which could be shared by
> clusterdb, createdb, dropdb, dropuser, reindexdb, and vacuumdb.
>
> However, we need to rethink the flag to be used for this: pg_dumpall
> uses -l, but many of the other utilities already use that for some
> other purpose, and it's not exactly mnemonic anyway.  "-d" for
> database could work, but that's also in use in some places, and
> furthermore somewhat confusing since many if not all of these
> utilities have an option to operate on a single database only, and you
> might think that -d would specify the database to operate on, rather
> than the one to be used to get the list of databases.  pgAdmin uses
> the term "maintenance database" to refer to a database to be used when
> none is explicitly specified, and I think that's fairly clear
> terminology.  So I propose that we add a --maintenance-db option (with
> no short form, since this is a relatively obscure need) to the tools
> listed above.  The tools will pass the associated value (or NULL if
> the option is not specified) to the above-mentioned routine in
> common.c, which will do the rest.
>
> If nobody objects, I'll go do that.  Hopefully that should be enough
> to put this problem to bed more or less permanently.

All right, I've worked up a (rather boring and tedious) patch to do
this, which is attached.

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

Robert Haas wrote:
> > However, we need to rethink the flag to be used for this: pg_dumpall
> > uses -l, but many of the other utilities already use that for some
> > other purpose, and it's not exactly mnemonic anyway. ?"-d" for
> > database could work, but that's also in use in some places, and
> > furthermore somewhat confusing since many if not all of these
> > utilities have an option to operate on a single database only, and you
> > might think that -d would specify the database to operate on, rather
> > than the one to be used to get the list of databases. ?pgAdmin uses
> > the term "maintenance database" to refer to a database to be used when
> > none is explicitly specified, and I think that's fairly clear
> > terminology. ?So I propose that we add a --maintenance-db option (with
> > no short form, since this is a relatively obscure need) to the tools
> > listed above. ?The tools will pass the associated value (or NULL if
> > the option is not specified) to the above-mentioned routine in
> > common.c, which will do the rest.
> >
> > If nobody objects, I'll go do that. ?Hopefully that should be enough
> > to put this problem to bed more or less permanently.
>
> All right, I've worked up a (rather boring and tedious) patch to do
> this, which is attached.

I wonder if we should bother using a flag for this. No one has asked
for one, and the new code to conditionally connect to databases should
function fine for most use cases.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Nov 2, 2011 at 2:05 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> > However, we need to rethink the flag to be used for this: pg_dumpall
>> > uses -l, but many of the other utilities already use that for some
>> > other purpose, and it's not exactly mnemonic anyway. ?"-d" for
>> > database could work, but that's also in use in some places, and
>> > furthermore somewhat confusing since many if not all of these
>> > utilities have an option to operate on a single database only, and you
>> > might think that -d would specify the database to operate on, rather
>> > than the one to be used to get the list of databases. ?pgAdmin uses
>> > the term "maintenance database" to refer to a database to be used when
>> > none is explicitly specified, and I think that's fairly clear
>> > terminology. ?So I propose that we add a --maintenance-db option (with
>> > no short form, since this is a relatively obscure need) to the tools
>> > listed above. ?The tools will pass the associated value (or NULL if
>> > the option is not specified) to the above-mentioned routine in
>> > common.c, which will do the rest.
>> >
>> > If nobody objects, I'll go do that. ?Hopefully that should be enough
>> > to put this problem to bed more or less permanently.
>>
>> All right, I've worked up a (rather boring and tedious) patch to do
>> this, which is attached.
>
> I wonder if we should bother using a flag for this.  No one has asked
> for one, and the new code to conditionally connect to databases should
> function fine for most use cases.

True, but OTOH we have such a flag for pg_dumpall, and I've already
done the work.

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

Robert Haas wrote:
> >> > If nobody objects, I'll go do that. ?Hopefully that should be enough
> >> > to put this problem to bed more or less permanently.
> >>
> >> All right, I've worked up a (rather boring and tedious) patch to do
> >> this, which is attached.
> >
> > I wonder if we should bother using a flag for this. ?No one has asked
> > for one, and the new code to conditionally connect to databases should
> > function fine for most use cases.
>
> True, but OTOH we have such a flag for pg_dumpall, and I've already
> done the work.

Well, every user-visible API option has a cost, and I am not sure there
is enough usefulness to overcome the cost of this.

As far as pg_dumpall, you could argue that the -l flag isn't needed;
the docs say:

-l dbname, --database=dbname
Specifies the name of the database to connect to to
dump global objects and discover what other databases
should be dumped. If not specified, the postgres
database will be used, and if that does not exist,
template1 will be used.

What is the value of this flag? The only value I can see would be if
the 'postgres' database does not exist and you are concerned that you
might block create database operations during pg_dumpall's dump of
global objects, or you don't have permissions for template1 for some
reason.

Also, if we are going to add this flag, we should have pg_dumpall use it
too and just depricate the old options.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Nov 2, 2011 at 8:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> >> > If nobody objects, I'll go do that. ?Hopefully that should be enough
>> >> > to put this problem to bed more or less permanently.
>> >>
>> >> All right, I've worked up a (rather boring and tedious) patch to do
>> >> this, which is attached.
>> >
>> > I wonder if we should bother using a flag for this. ?No one has asked
>> > for one, and the new code to conditionally connect to databases should
>> > function fine for most use cases.
>>
>> True, but OTOH we have such a flag for pg_dumpall, and I've already
>> done the work.
>
> Well, every user-visible API option has a cost, and I am not sure there
> is enough usefulness to overcome the cost of this.

I am not sure why you think this is worth the time it takes to argue
about it, but if you want to whack the patch around or just forget the
whole thing, go ahead. The difference between what you're proposing
and what I'm proposing is about 25 lines of code, so it hardly needs
an acre of justification. To me, making the tools consistent with
each other and not dependent on the user's choice of database names is
worth the tiny amount of code it takes to make that happen.

> Also, if we are going to add this flag, we should have pg_dumpall use it
> too and just depricate the old options.

I thought about that, but couldn't think of a compelling reason to
break backward compatibility.

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

Robert Haas wrote:
> On Wed, Nov 2, 2011 at 8:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> >> > If nobody objects, I'll go do that. ?Hopefully that should be enough
> >> >> > to put this problem to bed more or less permanently.
> >> >>
> >> >> All right, I've worked up a (rather boring and tedious) patch to do
> >> >> this, which is attached.
> >> >
> >> > I wonder if we should bother using a flag for this. ?No one has asked
> >> > for one, and the new code to conditionally connect to databases should
> >> > function fine for most use cases.
> >>
> >> True, but OTOH we have such a flag for pg_dumpall, and I've already
> >> done the work.
> >
> > Well, every user-visible API option has a cost, and I am not sure there
> > is enough usefulness to overcome the cost of this.
>
> I am not sure why you think this is worth the time it takes to argue
> about it, but if you want to whack the patch around or just forget the
> whole thing, go ahead. The difference between what you're proposing
> and what I'm proposing is about 25 lines of code, so it hardly needs
> an acre of justification. To me, making the tools consistent with
> each other and not dependent on the user's choice of database names is
> worth the tiny amount of code it takes to make that happen.

Well, it would be good to get other opinions on this. The amount of
code isn't really the issue for me, but rather keeping the user API as
clean as possible.

I don't want someone to say, "Oh, here's a new user option. Wonder why
I should use it? Hmm, no one can tell me."

If an option's use-case is not clear, we have to explain in the docs why
to use it, and right now no one can tell me why we should use it.

> > Also, if we are going to add this flag, we should have pg_dumpall use it
> > too and just deprecate the old options.
>
> I thought about that, but couldn't think of a compelling reason to
> break backward compatibility.

Well, I figure we better have something compelling to do any change,
including a new command-line option.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> I fixed this a different way. I originally thought I could skip over
> the 'postgres' database in the new cluster if it didn't exist in the old
> cluster, but we have do things like check it is empty, so that was going
> to be awkward.
>
> It turns out there was only one place that expected a 1-1 mapping of old
> and new databases (file transfer), so I just modified that code to allow
> skipping a database in the new cluster that didn't exist in the old
> cluster.
>
> Attached patch applied. This allows an upgrade if the 'postgres'
> database is missing from the old cluster.

OK, I thought some more and didn't like the way the code could loop off
the end of the new cluster without matching all the old cluster
database.

The attached, applied patches improves this.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Nov 3, 2011 at 11:20, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Wed, Nov 2, 2011 at 8:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > Robert Haas wrote:
>> >> >> > If nobody objects, I'll go do that. ?Hopefully that should be enough
>> >> >> > to put this problem to bed more or less permanently.
>> >> >>
>> >> >> All right, I've worked up a (rather boring and tedious) patch to do
>> >> >> this, which is attached.
>> >> >
>> >> > I wonder if we should bother using a flag for this. ?No one has asked
>> >> > for one, and the new code to conditionally connect to databases should
>> >> > function fine for most use cases.
>> >>
>> >> True, but OTOH we have such a flag for pg_dumpall, and I've already
>> >> done the work.
>> >
>> > Well, every user-visible API option has a cost, and I am not sure there
>> > is enough usefulness to overcome the cost of this.
>>
>> I am not sure why you think this is worth the time it takes to argue
>> about it, but if you want to whack the patch around or just forget the
>> whole thing, go ahead.  The difference between what you're proposing
>> and what I'm proposing is about 25 lines of code, so it hardly needs
>> an acre of justification.  To me, making the tools consistent with
>> each other and not dependent on the user's choice of database names is
>> worth the tiny amount of code it takes to make that happen.
>
> Well, it would be good to get other opinions on this.  The amount of
> code isn't really the issue for me, but rather keeping the user API as
> clean as possible.

Seems reasonably clean to me. Not sure what would be unclean about it?
Are you saying we need to explain the concept of maintenance db
somewhere in the docs?

>> > Also, if we are going to add this flag, we should have pg_dumpall use it
>> > too and just deprecate the old options.
>>
>> I thought about that, but couldn't think of a compelling reason to
>> break backward compatibility.

Adding it to pg_dumpal lwithout removing the old one doesn't cause
backwards compatibility break. Then mark the old one as deprecated,
and remove it a few releases down the road. We can't keep every switch
around forever ;)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander wrote:
> On Thu, Nov 3, 2011 at 11:20, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> On Wed, Nov 2, 2011 at 8:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> > Robert Haas wrote:
> >> >> >> > If nobody objects, I'll go do that. ?Hopefully that should be enough
> >> >> >> > to put this problem to bed more or less permanently.
> >> >> >>
> >> >> >> All right, I've worked up a (rather boring and tedious) patch to do
> >> >> >> this, which is attached.
> >> >> >
> >> >> > I wonder if we should bother using a flag for this. ?No one has asked
> >> >> > for one, and the new code to conditionally connect to databases should
> >> >> > function fine for most use cases.
> >> >>
> >> >> True, but OTOH we have such a flag for pg_dumpall, and I've already
> >> >> done the work.
> >> >
> >> > Well, every user-visible API option has a cost, and I am not sure there
> >> > is enough usefulness to overcome the cost of this.
> >>
> >> I am not sure why you think this is worth the time it takes to argue
> >> about it, but if you want to whack the patch around or just forget the
> >> whole thing, go ahead. ?The difference between what you're proposing
> >> and what I'm proposing is about 25 lines of code, so it hardly needs
> >> an acre of justification. ?To me, making the tools consistent with
> >> each other and not dependent on the user's choice of database names is
> >> worth the tiny amount of code it takes to make that happen.
> >
> > Well, it would be good to get other opinions on this. ?The amount of
> > code isn't really the issue for me, but rather keeping the user API as
> > clean as possible.
>
> Seems reasonably clean to me. Not sure what would be unclean about it?
> Are you saying we need to explain the concept of maintenance db
> somewhere in the docs?
>
> >> > Also, if we are going to add this flag, we should have pg_dumpall use it
> >> > too and just deprecate the old options.
> >>
> >> I thought about that, but couldn't think of a compelling reason to
> >> break backward compatibility.
>
> Adding it to pg_dumpal lwithout removing the old one doesn't cause
> backwards compatibility break. Then mark the old one as deprecated,
> and remove it a few releases down the road. We can't keep every switch
> around forever ;)

OK, good. I will work on this.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Magnus Hagander wrote:
> On Thu, Nov 3, 2011 at 11:20, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> On Wed, Nov 2, 2011 at 8:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> > Robert Haas wrote:
> >> >> >> > If nobody objects, I'll go do that. ?Hopefully that should be enough
> >> >> >> > to put this problem to bed more or less permanently.
> >> >> >>
> >> >> >> All right, I've worked up a (rather boring and tedious) patch to do
> >> >> >> this, which is attached.
> >> >> >
> >> >> > I wonder if we should bother using a flag for this. ?No one has asked
> >> >> > for one, and the new code to conditionally connect to databases should
> >> >> > function fine for most use cases.
> >> >>
> >> >> True, but OTOH we have such a flag for pg_dumpall, and I've already
> >> >> done the work.
> >> >
> >> > Well, every user-visible API option has a cost, and I am not sure there
> >> > is enough usefulness to overcome the cost of this.
> >>
> >> I am not sure why you think this is worth the time it takes to argue
> >> about it, but if you want to whack the patch around or just forget the
> >> whole thing, go ahead. ?The difference between what you're proposing
> >> and what I'm proposing is about 25 lines of code, so it hardly needs
> >> an acre of justification. ?To me, making the tools consistent with
> >> each other and not dependent on the user's choice of database names is
> >> worth the tiny amount of code it takes to make that happen.
> >
> > Well, it would be good to get other opinions on this. ?The amount of
> > code isn't really the issue for me, but rather keeping the user API as
> > clean as possible.
>
> Seems reasonably clean to me. Not sure what would be unclean about it?
> Are you saying we need to explain the concept of maintenance db
> somewhere in the docs?

My point is that no one can explain why they would need to specify an
alternate database when using 'postgres' and falling back to 'template1'
works for almost everyone. I say just make it automatic, as it was in
Robert's patch, and be done with it.

> >> > Also, if we are going to add this flag, we should have pg_dumpall use it
> >> > too and just deprecate the old options.
> >>
> >> I thought about that, but couldn't think of a compelling reason to
> >> break backward compatibility.
>
> Adding it to pg_dumpall without removing the old one doesn't cause
> backwards compatibility break. Then mark the old one as deprecated,
> and remove it a few releases down the road. We can't keep every switch
> around forever ;)

OK.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Dec 6, 2011 at 7:00 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Seems reasonably clean to me. Not sure what would be unclean about it?

Based on this feedback, I went ahead and committed my previous patch.
This means that if pg_upgrade wants to accept a --maintenance-db
option, it will be able to pass it through to any other commands it
invokes. And if it doesn't do that, vacuumdb et. al. should still
work anyway, as long as either template1 or postgres is accessible.

>>> > Also, if we are going to add this flag, we should have pg_dumpall use it
>>> > too and just deprecate the old options.
>>>
>>> I thought about that, but couldn't think of a compelling reason to
>>> break backward compatibility.
>
> Adding it to pg_dumpal lwithout removing the old one doesn't cause
> backwards compatibility break. Then mark the old one as deprecated,
> and remove it a few releases down the road. We can't keep every switch
> around forever ;)

I'm not really sold on tinkering with pg_dumpall; if it ain't broke,
don't fix it. But we can bikeshed about it if we like. :-)

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