Horton

Thread

Commits

  1. Prevent ALTER TYPE/DOMAIN/OPERATOR from changing extension membership.

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

    The Post Office <noreply@postgresql.org> — 2021-08-15T17:23:58Z

    The following bug has been logged on the website:

Bug reference:      17144
Logged by:          alex kozhemyakin
Email address:      a.kozhemyakin@postgrespro.ru
PostgreSQL version: 14beta1
Operating system:   ubuntu 20.04
Description:        

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"

The reproducing script:
PGBIN_OLD=/home/test/rel13
PGBIN_NEW=/home/test/rel14
PGDATA_OLD=/home/test/data_old
PGDATA_NEW=/home/test/data_new

git clone https://github.com/postgres/postgres.git postgres &&
cd ./postgres &&
git checkout REL_13_STABLE && git reset --hard HEAD && git rebase
./configure --prefix=$PGBIN_OLD
make -j2 > /dev/null && make -j2 -C contrib > /dev/null &&.
make install > /dev/null && make install -C contrib > /dev/null &&
git checkout REL_14_STABLE && rm -rf * && git reset --hard HEAD && git
rebase
./configure --prefix=$PGBIN_NEW
make -j2 > /dev/null && make -j2 -C contrib > /dev/null &&.
make install > /dev/null && make install -C contrib > /dev/null &&
cd ..
$PGBIN_OLD/bin/initdb -D $PGDATA_OLD &&.
$PGBIN_OLD/bin/pg_ctl -w -D $PGDATA_OLD -l logfile_old start &&
$PGBIN_OLD/bin/psql -c 'create extension earthdistance cascade;' postgres
&&
$PGBIN_OLD/bin/pg_ctl -w -D $PGDATA_OLD stop &&
$PGBIN_NEW/bin/initdb -D $PGDATA_NEW &&.
$PGBIN_NEW/bin/pg_upgrade -b $PGBIN_OLD/bin -d $PGDATA_OLD -B $PGBIN_NEW/bin
-D $PGDATA_NEW &&
$PGBIN_NEW/bin/pg_ctl -w -D $PGDATA_NEW -l logfile_new start &&
$PGBIN_NEW/bin/psql -f ./update_extensions.sql postgres

Removing the earthdistance extension in PG 13 before the upgrade resolves
the issue.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-16T16:05:53Z

    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

  3. ALTER TYPE vs extension membership (was Re: BUG #17144: Upgrade from v13 to v14 with the cube extension failed)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-16T19:36:59Z

    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

  4. Re: ALTER TYPE vs extension membership (was Re: BUG #17144: Upgrade from v13 to v14 with the cube extension failed)

    Andres Freund <andres@anarazel.de> — 2021-08-17T11:22:33Z

    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

  5. Re: ALTER TYPE vs extension membership (was Re: BUG #17144: Upgrade from v13 to v14 with the cube extension failed)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-17T13:35:50Z

    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