BUG #17144: Upgrade from v13 to v14 with the cube extension failed

PG Bug reporting form <noreply@postgresql.org> writes:

When trying to upgrade PostgreSQL 13 cluster with the earthdistance
extension installed to PostgreSQL 14, I get the following error:
psql:./update_extensions.sql:2: ERROR: type earth is already a member of
extension "earthdistance"

Hm, thanks. This does not seem to be a problem with pg_upgrade per se;
you can reproduce it with

regression=# CREATE EXTENSION cube VERSION '1.4';
CREATE EXTENSION
regression=# CREATE EXTENSION earthdistance;
CREATE EXTENSION
regression=# ALTER EXTENSION "cube" UPDATE;
ERROR: type earth is already a member of extension "earthdistance"

The failure happens during this command in cube--1.4--1.5.sql:

ALTER TYPE cube SET ( RECEIVE = cube_recv, SEND = cube_send );

with stack trace

#0 errfinish (filename=0x9c15e3 "pg_depend.c", lineno=210,
funcname=0x9c1660 <__func__.16045> "recordDependencyOnCurrentExtension")
at elog.c:515
#1 0x000000000049886f in recordDependencyOnCurrentExtension (
object=object@entry=0x7ffe09f2b778, isReplace=isReplace@entry=true)
at pg_depend.c:206
#2 0x00000000005f65ce in GenerateTypeDependencies (
typeTuple=typeTuple@entry=0x2fa8f70,
typeCatalog=typeCatalog@entry=0x7fc882ee5970,
defaultExpr=defaultExpr@entry=0x0, typacl=typacl@entry=0x0,
relationKind=relationKind@entry=0 '\000',
isImplicitArray=isImplicitArray@entry=false, isDependentType=false,
rebuild=true) at pg_type.c:614
#3 0x000000000069519e in AlterTypeRecurse (typeOid=37834,
isImplicitArray=<optimized out>, tup=<optimized out>,
catalog=0x7fc882ee5970, atparams=0x7ffe09f2bba0) at typecmds.c:4411
#4 0x0000000000695265 in AlterTypeRecurse (typeOid=37745,
isImplicitArray=<optimized out>, tup=<optimized out>,
catalog=0x7fc882ee5970, atparams=0x7ffe09f2bba0) at typecmds.c:4488
#5 0x00000000006997f6 in AlterType (stmt=stmt@entry=0x2fb01f0)
at typecmds.c:4313
#6 0x00000000008340be in ProcessUtilitySlow (pstate=0x2ee2900,
pstmt=0x2ee2b88,
queryString=0x2f067b8 "/* contrib/cube/cube--1.4--1.5.sql */\n\n-- complain if script is sourced in psql, rather than via ALTER EXTENSION\n\n\n-- Remove @ and ~\nDROP OPERATOR @ (cube, cube);\nDROP OPERATOR ~ (cube, cube);\n\n-- Add"..., context=PROCESS_UTILITY_QUERY, params=0x0, queryEnv=0x0, qc=0x0,

So apparently the dependency-updating logic is a few bricks shy
of a load. The object being passed to recordDependencyOnCurrentExtension
is

(gdb) p *object
$1 = {classId = 1247, objectId = 37834, objectSubId = 0}

which sure enough is type "earth". So apparently, this code is
trying to absorb EVERYTHING that depends on type cube into the
cube extension.

[ experiments a bit more ] It might just be directly-dependent types.
If I create a table column of type cube, then nothing strange happens
during the upgrade. But if I create a domain over cube, then do the
update, the domain gets absorbed into the extension. That'd be kind
of annoying :-(

Haven't discovered exactly where it's going off the rails, though.

regards, tom lane

I wrote:

Hm, thanks. This does not seem to be a problem with pg_upgrade per se;
you can reproduce it with

regression=# CREATE EXTENSION cube VERSION '1.4';
CREATE EXTENSION
regression=# CREATE EXTENSION earthdistance;
CREATE EXTENSION
regression=# ALTER EXTENSION "cube" UPDATE;
ERROR: type earth is already a member of extension "earthdistance"

[ experiments a bit more ] It might just be directly-dependent types.
If I create a table column of type cube, then nothing strange happens
during the upgrade. But if I create a domain over cube, then do the
update, the domain gets absorbed into the extension. That'd be kind
of annoying :-(

So the problem is that ALTER TYPE SET recurses to dependent domains,
as it must, but it is not careful about what that implies for extension
membership. It looks like we need to extend GenerateTypeDependencies
so that it knows not to mess with extension dependencies when doing
an ALTER.

There's a policy question here, which is when does an operation on
a pre-existing object within an extension script cause the object
to be absorbed into the extension? You might naively say "never",
but that's not our historical behavior, and I think it'd clearly
be the wrong thing in some cases. For example, consider

CREATE TYPE cube; -- make a shell type
-- do something that requires type cube to exist
CREATE EXTENSION cube;

We don't want this to fail, because it might be necessary to do
things that way to get out of circular dependencies. On the other
hand, the formerly-shell type had certainly better wind up owned
by the extension.

The general policy as the code stands seems to be that CREATE OR
REPLACE-style operations will absorb any replaced object into
the extension. IMO extension scripts generally shouldn't use
CREATE OR REPLACE unless they're sure they already have such an
object; but if one does use such a command, I think this behavior
is reasonable.

The situation is a lot less clear-cut for ALTER commands, though.
Again, it's dubious that an extension should ever apply ALTER to
an object that it doesn't know it already owns; but if it does,
should that result in absorbing the object? I'm inclined to think
not, so the attached patch just causes AlterTypeRecurse and
AlterDomainDefault to never change the object's extension membership.
That's more behavioral change than is strictly needed to fix the
bug at hand, but I think it's a consistent definition.

I looked around for other places that might have similar issues,
and the only one I could find (accepting that CREATE OR REPLACE
should work this way) is that ALTER OPERATOR ... SET applies
makeOperatorDependencies, which has the same sort of behavior as
GenerateTypeDependencies does. I'm inclined to think that for
consistency, we should make that case likewise not change extension
membership; but I've not done anything about it in the attached.

Another point that perhaps deserves discussion is whether it's
okay to change the signature of GenerateTypeDependencies in
stable branches (we need to fix this in v13 not only v14/HEAD).
I can't see a good reason for extensions to be calling that,
and codesearch.debian.net knows of no outside callers, so I'm
inclined to just change it. If anyone thinks that's too risky,
we could do something with a wrapper function in v13.

Comments?

regards, tom lane

Hi,

On 2021-08-16 15:36:59 -0400, Tom Lane wrote:

There's a policy question here, which is when does an operation on
a pre-existing object within an extension script cause the object
to be absorbed into the extension? You might naively say "never",
but that's not our historical behavior, and I think it'd clearly
be the wrong thing in some cases. For example, consider

CREATE TYPE cube; -- make a shell type
-- do something that requires type cube to exist
CREATE EXTENSION cube;

We don't want this to fail, because it might be necessary to do
things that way to get out of circular dependencies.

In what scenario would that be a good way to address a circular
dependency? ISTM that whatever depends on $ext should be using shell
types or such to avoid the circular dependency, then create $ext. Rather
than creating shell types for object in $ext and then create $ext.

Another point that perhaps deserves discussion is whether it's
okay to change the signature of GenerateTypeDependencies in
stable branches (we need to fix this in v13 not only v14/HEAD).
I can't see a good reason for extensions to be calling that,
and codesearch.debian.net knows of no outside callers, so I'm
inclined to just change it. If anyone thinks that's too risky,
we could do something with a wrapper function in v13.

Seems OK to not bother with a wrapper to me - I can't see a reason
either.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2021-08-16 15:36:59 -0400, Tom Lane wrote:

There's a policy question here, which is when does an operation on
a pre-existing object within an extension script cause the object
to be absorbed into the extension? You might naively say "never",
but that's not our historical behavior, and I think it'd clearly
be the wrong thing in some cases. For example, consider

CREATE TYPE cube; -- make a shell type
-- do something that requires type cube to exist
CREATE EXTENSION cube;

We don't want this to fail, because it might be necessary to do
things that way to get out of circular dependencies.

In what scenario would that be a good way to address a circular
dependency? ISTM that whatever depends on $ext should be using shell
types or such to avoid the circular dependency, then create $ext. Rather
than creating shell types for object in $ext and then create $ext.

Perhaps. But that does work today (I tested), and it's been the
intentional behavior ever since we invented extensions, to judge
by the fact that the existing comments on GenerateTypeDependencies
describe the current behavior explicitly. (Admitted, that's only
talking about types not other sorts of objects, which is why I moved
that comment in the proposed patch.) So I'm loath to get rid of it
in a back-patched bug fix.

Maybe there is a case for becoming stricter in HEAD, but I'm not
really clear on what would be an improvement. I do not think that
"don't adopt the object into the extension" could be sane. The only
other alternative is "throw an error", which goes against the clear
intention and definition of CREATE OR REPLACE.

The fact that you don't need to say CREATE OR REPLACE TYPE to replace
a shell type muddies the waters here a bit, but I think we should still
consider that behavior to be an instance of CREATE OR REPLACE.

regards, tom lane