ALTER TYPE vs extension membership (was Re: BUG #17144: Upgrade from v13 to v14 with the cube extension failed) - Mailing list pgsql-hackers

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

diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 10f3119670..07bcdc463a 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -179,6 +179,13 @@ recordMultipleDependencies(const ObjectAddress *depender,
  * existed), so we must check for a pre-existing extension membership entry.
  * Passing false is a guarantee that the object is newly created, and so
  * could not already be a member of any extension.
+ *
+ * Note: isReplace = true is typically used when updating a object in
+ * CREATE OR REPLACE and similar commands.  The net effect is that if an
+ * extension script uses such a command on a pre-existing free-standing
+ * object, the object will be absorbed into the extension.  If the object
+ * is already a member of some other extension, the command will fail.
+ * This behavior is desirable for cases such as replacing a shell type.
  */
 void
 recordDependencyOnCurrentExtension(const ObjectAddress *object,
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index 6f9b5471da..cdce22f394 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -167,6 +167,7 @@ TypeShellMake(const char *typeName, Oid typeNamespace, Oid ownerId)
                                  0,
                                  false,
                                  false,
+                                 true,    /* make extension dependency */
                                  false);

     /* Post creation hook for new shell type */
@@ -504,6 +505,7 @@ TypeCreate(Oid newTypeOid,
                                  relationKind,
                                  isImplicitArray,
                                  isDependentType,
+                                 true,    /* make extension dependency */
                                  rebuildDeps);

     /* Post creation hook for new type */
@@ -537,13 +539,17 @@ TypeCreate(Oid newTypeOid,
  * isDependentType is true if this is an implicit array or relation rowtype;
  * that means it doesn't need its own dependencies on owner etc.
  *
- * If rebuild is true, we remove existing dependencies and rebuild them
- * from scratch.  This is needed for ALTER TYPE, and also when replacing
- * a shell type.  We don't remove an existing extension dependency, though.
- * (That means an extension can't absorb a shell type created in another
- * extension, nor ALTER a type created by another extension.  Also, if it
- * replaces a free-standing shell type or ALTERs a free-standing type,
- * that type will become a member of the extension.)
+ * We make an extension-membership dependency if we're in an extension
+ * script and makeExtensionDep is true (and isDependentType isn't true).
+ * makeExtensionDep should be true when creating a new type or replacing a
+ * shell type, but not for ALTER TYPE on an existing type.  Passing false
+ * causes the type's extension membership to be left alone.
+ *
+ * rebuild should be true if this is a pre-existing type.  We will remove
+ * existing dependencies and rebuild them from scratch.  This is needed for
+ * ALTER TYPE, and also when replacing a shell type.  We don't remove any
+ * existing extension dependency, though (hence, if makeExtensionDep is also
+ * true and the type belongs to some other extension, an error will occur).
  */
 void
 GenerateTypeDependencies(HeapTuple typeTuple,
@@ -553,6 +559,7 @@ GenerateTypeDependencies(HeapTuple typeTuple,
                          char relationKind, /* only for relation rowtypes */
                          bool isImplicitArray,
                          bool isDependentType,
+                         bool makeExtensionDep,
                          bool rebuild)
 {
     Form_pg_type typeForm = (Form_pg_type) GETSTRUCT(typeTuple);
@@ -611,7 +618,8 @@ GenerateTypeDependencies(HeapTuple typeTuple,
         recordDependencyOnNewAcl(TypeRelationId, typeObjectId, 0,
                                  typeForm->typowner, typacl);

-        recordDependencyOnCurrentExtension(&myself, rebuild);
+        if (makeExtensionDep)
+            recordDependencyOnCurrentExtension(&myself, rebuild);
     }

     /* Normal dependencies on the I/O and support functions */
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 93eeff950b..6bdb1a1660 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2675,6 +2675,7 @@ AlterDomainDefault(List *names, Node *defaultRaw)
                              0, /* relation kind is n/a */
                              false, /* a domain isn't an implicit array */
                              false, /* nor is it any kind of dependent type */
+                             false, /* don't touch extension membership */
                              true); /* We do need to rebuild dependencies */

     InvokeObjectPostAlterHook(TypeRelationId, domainoid, 0);
@@ -4415,6 +4416,7 @@ AlterTypeRecurse(Oid typeOid, bool isImplicitArray,
                              0, /* we rejected composite types above */
                              isImplicitArray,    /* it might be an array */
                              isImplicitArray,    /* dependent iff it's array */
+                             false, /* don't touch extension membership */
                              true);

     InvokeObjectPostAlterHook(TypeRelationId, typeOid, 0);
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index b05db9641a..e568e21dee 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -386,6 +386,7 @@ extern void GenerateTypeDependencies(HeapTuple typeTuple,
                                                          * rowtypes */
                                      bool isImplicitArray,
                                      bool isDependentType,
+                                     bool makeExtensionDep,
                                      bool rebuild);

 extern void RenameTypeInternal(Oid typeOid, const char *newTypeName,