BUG #6706: pg_upgrade fails when plpgsql dropped/re-created

On Mon, Jun 25, 2012 at 10:57:56PM +0000, m.sakrejda@gmail.com wrote:

The following bug has been logged on the website:

Bug reference: 6706
Logged by: Maciek Sakrejda
Email address: m.sakrejda@gmail.com
PostgreSQL version: Unsupported/Unknown
Operating system: Ubuntu 12.04 LTS (3.2.0-25-generic x86_64)
Description:

Using the 9.2beta2 of pg_upgrade and for the upgrade-to cluster.

Ran into an issue upgrading a 9.1 cluster via pg_upgrade. I can reproduce it
consistently:

1. Create a 9.1 cluster
2. Run "drop extension plpgsql" as superuser
3. Run "create extension plpgsql" as non-superuser
4. Perform normal upgrade via pg_upgrade

The last step fails and I get the following error in
pg_upgrade_restore.log:

SELECT binary_upgrade.create_empty_extension('plpgsql', 'pg_catalog', false,
'1.0', NULL, NULL, ARRAY[]::pg_catalog.text[]);
psql:pg_upgrade_dump_db.sql:40: ERROR: duplicate key value violates unique
constraint "pg_extension_name_index"
DETAIL: Key (extname)=(plpgsql) already exists.

I can easily recreate this failure, even doing the drop/create as
super-user. Fortunately the cause is clearly outlined in the C comments
of pg_dump.c:

if (!binary_upgrade)
{
/*
* In a regular dump, we use IF NOT EXISTS so that there isn't a
* problem if the extension already exists in the target database;
* this is essential for installed-by-default extensions such as
* plpgsql.
*
* In binary-upgrade mode, that doesn't work well, so instead we skip
* built-in extensions based on their OIDs; see
* selectDumpableExtension.
*/
appendPQExpBuffer(q, "CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s;\n",
qextname, fmtId(extinfo->namespace));
}
else
{
int i;
int n;

appendPQExpBuffer(q, "-- For binary upgrade, create an empty extension and insert objects into it\n");
appendPQExpBuffer(q,
"SELECT binary_upgrade.create_empty_extension(");

For non-binary-upgrade dumps, IF NOT EXISTS easily allows drop/create of
plpgsql to work. In binary-upgrade mode, selectDumpableExtension()
dumps all extensions that have an oid greater than FirstNormalObjectId.
This is the only use of FirstNormalObjectId in the pg_dump code, and
obviously something that needs attention. Other objects are skipped if
they exist in pg_catalog, but extensions are always in pg_catalog, so
that filter will not work.

I can't think of how to fix this. Perhaps we need to query the
pg_extension table as of the SELECT function all. A workaround is to
renumber the oid of the plpgsql pg_extension row to be less than
FirstNormalObjectId, but that is hardly user-friendly.

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

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

Bruce Momjian <bruce@momjian.us> writes:

I can't think of how to fix this. Perhaps we need to query the
pg_extension table as of the SELECT function all.

I think you're misjudging the core of the issue. The same thing
would happen if somebody dropped and recreated the public schema.
Or anything else that we create at initdb time but allow to be
dropped.

regards, tom lane

On Fri, Jun 29, 2012 at 11:35:15PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I can't think of how to fix this. Perhaps we need to query the
pg_extension table as of the SELECT function all.

I think you're misjudging the core of the issue. The same thing
would happen if somebody dropped and recreated the public schema.
Or anything else that we create at initdb time but allow to be
dropped.

I just tested dropping and recreating the 'public' schema and pg_upgrade
worked fine.

I think the fix we need for extensions is to change:

SELECT binary_upgrade.create_empty_extension('plpgsql',
'pg_catalog', false, '1.0', NULL, NULL,
ARRAY[]::pg_catalog.text[]);

to

SELECT binary_upgrade.create_empty_extension('plpgsql',
'pg_catalog', false, '1.0', NULL, NULL, ARRAY[]::pg_catalog.text[])
WHERE (SELECT COUNT(*) FROM pg_extension WHERE extname = 'plpgsql') = 0;

This basically conditionally calls
binary_upgrade.create_empty_extension() based on whether the extension
already exists in the new cluster.

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

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

On Sat, Jun 30, 2012 at 01:00:07AM -0400, Bruce Momjian wrote:

On Fri, Jun 29, 2012 at 11:35:15PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I can't think of how to fix this. Perhaps we need to query the
pg_extension table as of the SELECT function all.

I think you're misjudging the core of the issue. The same thing
would happen if somebody dropped and recreated the public schema.
Or anything else that we create at initdb time but allow to be
dropped.

I just tested dropping and recreating the 'public' schema and pg_upgrade
worked fine.

I think the fix we need for extensions is to change:

SELECT binary_upgrade.create_empty_extension('plpgsql',
'pg_catalog', false, '1.0', NULL, NULL,
ARRAY[]::pg_catalog.text[]);

to

SELECT binary_upgrade.create_empty_extension('plpgsql',
'pg_catalog', false, '1.0', NULL, NULL, ARRAY[]::pg_catalog.text[])
WHERE (SELECT COUNT(*) FROM pg_extension WHERE extname = 'plpgsql') = 0;

This basically conditionally calls
binary_upgrade.create_empty_extension() based on whether the extension
already exists in the new cluster.

FYI, I forgot to mention that there is a unique index on extname, so
testing just for the name should work fine.

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

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

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Jun 29, 2012 at 11:35:15PM -0400, Tom Lane wrote:

I think you're misjudging the core of the issue. The same thing
would happen if somebody dropped and recreated the public schema.
Or anything else that we create at initdb time but allow to be
dropped.

I just tested dropping and recreating the 'public' schema and pg_upgrade
worked fine.

Did it restore the nonstandard ownership of the schema? Your proposed
fix for plpgsql won't preserve the previous state of the extension.
(Maybe we don't care, but it needs consideration.)

regards, tom lane

On Sat, Jun 30, 2012 at 11:12:56AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Jun 29, 2012 at 11:35:15PM -0400, Tom Lane wrote:

I think you're misjudging the core of the issue. The same thing
would happen if somebody dropped and recreated the public schema.
Or anything else that we create at initdb time but allow to be
dropped.

I just tested dropping and recreating the 'public' schema and pg_upgrade
worked fine.

Did it restore the nonstandard ownership of the schema? Your proposed
fix for plpgsql won't preserve the previous state of the extension.
(Maybe we don't care, but it needs consideration.)

My point was that drop/create of the public schema does not generate a
pg_upgrade error like plpgsql does.

Let me address the schema question here and the plpgsql issue in the
next email.

Did it restore the nonstandard ownership of the schema?

No --- drop/create of the public schema produces:

test=> \dn+
List of schemas
Name | Owner | Access privileges | Description
--------+----------+-------------------+-------------
public | postgres | |
(1 row)

while the original shipped public and the post-upgrade of a drop/created
schema are:

test=> \dn+
List of schemas
Name | Owner | Access privileges | Description
--------+----------+----------------------+------------------------
public | postgres | postgres=UC/postgres+| standard public schema
| | =UC/postgres |
(1 row)

However, surprisingly, a simple pg_dump/restore also does not preserve
the public schema permissions either. :-(

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

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

Bruce Momjian <bruce@momjian.us> writes:

On Sat, Jun 30, 2012 at 11:12:56AM -0400, Tom Lane wrote:

Did it restore the nonstandard ownership of the schema?

No --- drop/create of the public schema produces:
...
However, surprisingly, a simple pg_dump/restore also does not preserve
the public schema permissions either. :-(

Right. My point is that there is a whole lot of stuff that initdb
creates but does not mark "pinned" in pg_depend, with the intention that
users could drop it, and perhaps recreate similarly-named objects with
different properties. We have never had a very sane story for what
would happen to such modified objects during dump/reload, and pg_upgrade
is no better (or worse). I don't think there's too much point in
thinking about plpgsql alone without also worrying about

* system views (including the information schema)
* collations
* conversions
* text search dictionaries

Now for a lot of this stuff, it's perhaps reasonable that a major
version upgrade would restore the objects to standard state. I'm
thinking though that it's rather bad that we treat either the public
schema or the plpgsql language that way. In particular, an admin
might have wished to remove or restrict those two objects for security
reasons, in which case he'd not be very happy if pg_upgrade resurrected
them or restored their default permissions.

BTW, I think your proposed fix doesn't work even without considering
this angle --- it would prevent creation of the duplicate pg_extension
row, but the binary-upgrade dump script is still going to try to create
the extension's member objects.

regards, tom lane

On Sat, Jun 30, 2012 at 02:37:47PM -0400, Tom Lane wrote:

However, surprisingly, a simple pg_dump/restore also does not preserve
the public schema permissions either. :-(

Right. My point is that there is a whole lot of stuff that initdb
creates but does not mark "pinned" in pg_depend, with the intention that
users could drop it, and perhaps recreate similarly-named objects with
different properties. We have never had a very sane story for what
would happen to such modified objects during dump/reload, and pg_upgrade
is no better (or worse). I don't think there's too much point in
thinking about plpgsql alone without also worrying about

* system views (including the information schema)
* collations
* conversions
* text search dictionaries

Now for a lot of this stuff, it's perhaps reasonable that a major
version upgrade would restore the objects to standard state. I'm
thinking though that it's rather bad that we treat either the public
schema or the plpgsql language that way. In particular, an admin
might have wished to remove or restrict those two objects for security
reasons, in which case he'd not be very happy if pg_upgrade resurrected
them or restored their default permissions.

Agreed What surprised me is that pg_dumpall/restore brings them back to
their default state too, and I haven't seen any complaints. (I would
complain.)

BTW, I think your proposed fix doesn't work even without considering
this angle --- it would prevent creation of the duplicate pg_extension
row, but the binary-upgrade dump script is still going to try to create
the extension's member objects.

Agreed. The conditionally function call worked just fine, but all those
dependent helper functions made a simple solution impossible.

What I decided to do was just conditionally drop the extension, just
like we conditionally create the plpgsql extension in non-binary-upgrade
mode. We could have just said drop/recreate of plpgsql was unsupported,
but it bothered me that we had different error cases for binary and
non-binary upgrades, which seemed odd.

Do we want to keep the FirstNormalObjectId on the condtional
drop/recreate?

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

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

Bruce Momjian <bruce@momjian.us> writes:

+ 
+ 		/*
+ 		 *	We unconditionally create the extension, so we must drop it if it
+ 		 *	exists.  This could happen if the user deleted 'plpgsql' and then
+ 		 *	readded it, causing its oid to be greater than FirstNormalObjectId.
+ 		 */
+ 		appendPQExpBuffer(q, "DROP EXTENSION IF EXISTS %s;\n", qextname);

This doesn't seem like anything but a wart :-(. It's unlike the
behavior for every other kind of object, it introduces the inconsistent
behavior even in non-binary-upgrade cases, and it does nothing at all to
address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

regards, tom lane

On Mon, Jul 02, 2012 at 01:01:51PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

+ 
+ 		/*
+ 		 *	We unconditionally create the extension, so we must drop it if it
+ 		 *	exists.  This could happen if the user deleted 'plpgsql' and then
+ 		 *	readded it, causing its oid to be greater than FirstNormalObjectId.
+ 		 */
+ 		appendPQExpBuffer(q, "DROP EXTENSION IF EXISTS %s;\n", qextname);

This doesn't seem like anything but a wart :-(. It's unlike the
behavior for every other kind of object, it introduces the inconsistent

Well, our use of FirstNormalObjectId by pg_dump for extensions is unique
too.

behavior even in non-binary-upgrade cases, and it does nothing at all to

This code is in the binary-upgrade block --- not sure how it could
affect non-binary upgrades.

This is the trimmed-down code block:

if (!binary_upgrade)
{
appendPQExpBuffer(q, "CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s;\n",
qextname, fmtId(extinfo->namespace));
}
else
{
--> appendPQExpBuffer(q, "DROP EXTENSION IF EXISTS %s;\n", qextname);
appendPQExpBuffer(q,
"SELECT binary_upgrade.create_empty_extension(");

The idea is that the IF NOT EXISTS and IF EXISTS are symmetric, which is
my goal.

address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

Well, it still does the create extension in binary mode like before ---
not sure what the problem is.

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

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

On Mon, Jul 02, 2012 at 01:28:36PM -0400, Bruce Momjian wrote:

This is the trimmed-down code block:

if (!binary_upgrade)
{
appendPQExpBuffer(q, "CREATE EXTENSION IF NOT EXISTS %s WITH SCHEMA %s;\n",
qextname, fmtId(extinfo->namespace));
}
else
{
--> appendPQExpBuffer(q, "DROP EXTENSION IF EXISTS %s;\n", qextname);
appendPQExpBuffer(q,
"SELECT binary_upgrade.create_empty_extension(");

The idea is that the IF NOT EXISTS and IF EXISTS are symmetric, which is
my goal.

address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

Well, it still does the create extension in binary mode like before ---
not sure what the problem is.

Applied and back-patched to 9.2.

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

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

Bruce Momjian <bruce@momjian.us> writes:

address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

Well, it still does the create extension in binary mode like before ---
not sure what the problem is.

Applied and back-patched to 9.2.

I do not believe that this patch fixes the problem, and I also believe
that it creates new problems. Please revert.

regards, tom lane

On Thu, Jul 05, 2012 at 12:21:58AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

Well, it still does the create extension in binary mode like before ---
not sure what the problem is.

Applied and back-patched to 9.2.

I do not believe that this patch fixes the problem, and I also believe
that it creates new problems. Please revert.

I asked for an explaination of the problem, but received no reply:

http://archives.postgresql.org/pgsql-bugs/2012-07/msg00005.php

so I assumed you were fine with it. Please explain. Does anyone else
understand the problem Tom is seeing?

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

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

On Thu, Jul 5, 2012 at 9:32 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Jul 05, 2012 at 12:21:58AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

Well, it still does the create extension in binary mode like before ---
not sure what the problem is.

Applied and back-patched to 9.2.

I do not believe that this patch fixes the problem, and I also believe
that it creates new problems. Please revert.

I asked for an explaination of the problem, but received no reply:

http://archives.postgresql.org/pgsql-bugs/2012-07/msg00005.php

so I assumed you were fine with it. Please explain. Does anyone else
understand the problem Tom is seeing?

Well, the part I understood was that your fix apparently does not
guarantee to restore plpgsql to the state it was in, just to restore
it to existence. But previous complaints about similar issues have
fallen on deaf ears (see bug #5184). Perhaps Tom has had a change of
heart, but if so we have a few things to fix, not just this one.

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

On Fri, Jul 06, 2012 at 07:32:10PM -0400, Robert Haas wrote:

On Thu, Jul 5, 2012 at 9:32 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Jul 05, 2012 at 12:21:58AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

address the points I made about reproducing the previous state in cases
where the admin removed the language or changed its permissions.

Well, it still does the create extension in binary mode like before ---
not sure what the problem is.

Applied and back-patched to 9.2.

I do not believe that this patch fixes the problem, and I also believe
that it creates new problems. Please revert.

I asked for an explaination of the problem, but received no reply:

http://archives.postgresql.org/pgsql-bugs/2012-07/msg00005.php

so I assumed you were fine with it. Please explain. Does anyone else
understand the problem Tom is seeing?

Well, the part I understood was that your fix apparently does not
guarantee to restore plpgsql to the state it was in, just to restore
it to existence. But previous complaints about similar issues have
fallen on deaf ears (see bug #5184). Perhaps Tom has had a change of
heart, but if so we have a few things to fix, not just this one.

Yes, I think my fix gives binary-upgrade the same behavior as
pg_dump/restore --- for all its good and bad. I couldn't see why they
should be different, or at least why binary-upgrade should be worse
(throw an error).

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

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

Well, the part I understood was that your fix apparently does not
guarantee to restore plpgsql to the state it was in, just to restore
it to existence. But previous complaints about similar issues have
fallen on deaf ears (see bug #5184). Perhaps Tom has had a change of
heart, but if so we have a few things to fix, not just this one.

Yes, I think my fix gives binary-upgrade the same behavior as
pg_dump/restore --- for all its good and bad. I couldn't see why they
should be different, or at least why binary-upgrade should be worse
(throw an error).

I agree that they shouldn't be different, but if this can't be made to
work, perhaps both should fail in this situation? Changing ownership
of objects on a dump/restore seems like a decidedly un-Postgres-like
foot-gun. Granted, this is only applicable in only a small set of
situations, but it's still a foot-gun--a metadata integrity issue if
you will. For what it's worth, I completely agree with Robert's
comments in the thread regarding #5184 [1]http://archives.postgresql.org/pgsql-bugs/2009-11/msg00113.php. Does the comparison to
template0/1 suggested in that thread merit further consideration?

[1]: http://archives.postgresql.org/pgsql-bugs/2009-11/msg00113.php

On Fri, Jul 06, 2012 at 08:38:01PM -0700, Maciek Sakrejda wrote:

Well, the part I understood was that your fix apparently does not
guarantee to restore plpgsql to the state it was in, just to restore
it to existence. But previous complaints about similar issues have
fallen on deaf ears (see bug #5184). Perhaps Tom has had a change of
heart, but if so we have a few things to fix, not just this one.

Yes, I think my fix gives binary-upgrade the same behavior as
pg_dump/restore --- for all its good and bad. I couldn't see why they
should be different, or at least why binary-upgrade should be worse
(throw an error).

I agree that they shouldn't be different, but if this can't be made to
work, perhaps both should fail in this situation? Changing ownership
of objects on a dump/restore seems like a decidedly un-Postgres-like
foot-gun. Granted, this is only applicable in only a small set of
situations, but it's still a foot-gun--a metadata integrity issue if
you will. For what it's worth, I completely agree with Robert's
comments in the thread regarding #5184 [1]. Does the comparison to
template0/1 suggested in that thread merit further consideration?

[1]: http://archives.postgresql.org/pgsql-bugs/2009-11/msg00113.php

Yes, if both binary and non-binary restores should throw errors, I am
fine with that too. Frankly, the filter we use now for extensions in
binary mode, FirstNormalObjectId, isn't sufficient because I think you
can modify the extension without changing its oid.

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

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

So, is there hope of a better fix here for 9.2 (specifically for
preserving extension ownership on pg_upgrade and dump/restore, though
I understand it may not make sense to do that if we can't fix a number
of related issues)? If not, is the below catalog-twiddling sane,
lacking an ALTER EXTENSION foo OWNER TO ...?

postgres=# update pg_extension set extowner = (select oid from
pg_roles where rolname = 'maciek') where extname = 'plpgsql';

On Tue, Jul 10, 2012 at 01:03:18PM -0700, Maciek Sakrejda wrote:

So, is there hope of a better fix here for 9.2 (specifically for
preserving extension ownership on pg_upgrade and dump/restore, though
I understand it may not make sense to do that if we can't fix a number
of related issues)? If not, is the below catalog-twiddling sane,
lacking an ALTER EXTENSION foo OWNER TO ...?

postgres=# update pg_extension set extowner = (select oid from
pg_roles where rolname = 'maciek') where extname = 'plpgsql';

There is no hope that any changes to extensions will be preserved across
upgrades any better than it was in 9.1 --- the change is only that
pg_upgrade will not fail.

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

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