Re: postgres_fdw fails to see that array type belongs to extension

David Geier <geidav.pg@gmail.com> writes:
> On 12/27/23 18:38, Tom Lane wrote:
>> Hmm.  It seems odd that if an extension defines a type, the type is
>> listed as a member of the extension but the array type is not.
>> That makes it look like the array type is an externally-created
>> thing that happens to depend on the extension, when it's actually
>> part of the extension.  I'm surprised we've not run across other
>> misbehaviors traceable to that.

> Agreed.
> The attached patch just adds a 2nd dependency between the array type and 
> the extension, using recordDependencyOnCurrentExtension().

I don't like this patch too much: it fixes the problem in a place far
away from GenerateTypeDependencies' existing treatment of extension
dependencies, and relatedly to that, fails to update the comments
it has falsified.  I think we should do it more like the attached.
This approach means that relation rowtypes will also be explicitly
listed as extension members, which seems like it is fixing the same
sort of bug as the array case.

I also noted that you'd not run check-world, because there's a test
case that changes output.  This is good though, because we don't need
to add any new test to prove it does what we want.

There's one big remaining to-do item, I think: experimentation with
pg_upgrade proves that a binary upgrade fails to fix the extension
membership of arrays/rowtypes.  That's because pg_dump needs to
manually reconstruct the membership dependencies, and it thinks that
it doesn't need to do anything for implicit arrays.  Normally the
point of that is that we want to exactly reconstruct the extension's
contents, but I think in this case we'd really like to add the
additional pg_depend entries even if they weren't there before.
Otherwise people wouldn't get the new behavior until they do a
full dump/reload.

I can see two ways we could do that:

* add logic to pg_dump

* modify ALTER EXTENSION ADD TYPE so that it automatically recurses
from a base type to its array type (and I guess we'd need to add
something for relation rowtypes and multiranges, too).

I think I like the latter approach because it's like how we
handle ownership: pg_dump doesn't emit any commands to explicitly
change the ownership of dependent types, either.  (But see [1].)
We could presumably steal some logic from ALTER TYPE OWNER.
I've not tried to code that here, though.

            regards, tom lane

[1] https://www.postgresql.org/message-id/1580383.1705343264%40sss.pgh.pa.us

diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index d7167108fb..5691be26a6 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -538,7 +538,7 @@ TypeCreate(Oid newTypeOid,
  * that means it doesn't need its own dependencies on owner etc.
  *
  * We make an extension-membership dependency if we're in an extension
- * script and makeExtensionDep is true (and isDependentType isn't true).
+ * script and makeExtensionDep is 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.
@@ -598,7 +598,7 @@ GenerateTypeDependencies(HeapTuple typeTuple,
     ObjectAddressSet(myself, TypeRelationId, typeObjectId);

     /*
-     * Make dependencies on namespace, owner, ACL, extension.
+     * Make dependencies on namespace, owner, ACL.
      *
      * Skip these for a dependent type, since it will have such dependencies
      * indirectly through its depended-on type or relation.
@@ -618,11 +618,18 @@ GenerateTypeDependencies(HeapTuple typeTuple,

         recordDependencyOnNewAcl(TypeRelationId, typeObjectId, 0,
                                  typeForm->typowner, typacl);
-
-        if (makeExtensionDep)
-            recordDependencyOnCurrentExtension(&myself, rebuild);
     }

+    /*
+     * Make extension dependency if requested.
+     *
+     * We used to skip this for dependent types, but it seems better to record
+     * their extension membership explicitly; otherwise code such as
+     * postgres_fdw's shippability test will be fooled.
+     */
+    if (makeExtensionDep)
+        recordDependencyOnCurrentExtension(&myself, rebuild);
+
     /* Normal dependencies on the I/O and support functions */
     if (OidIsValid(typeForm->typinput))
     {
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out
b/src/test/modules/test_extensions/expected/test_extensions.out
index 472627a232..9528448e09 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -53,7 +53,11 @@ Objects in extension "test_ext7"
  table ext7_table1
  table ext7_table2
  table old_table1
-(6 rows)
+ type ext7_table1
+ type ext7_table1[]
+ type ext7_table2
+ type ext7_table2[]
+(10 rows)

 alter extension test_ext7 update to '2.0';
 \dx+ test_ext7
@@ -62,7 +66,9 @@ Objects in extension "test_ext7"
 -------------------------------
  sequence ext7_table2_col2_seq
  table ext7_table2
-(2 rows)
+ type ext7_table2
+ type ext7_table2[]
+(4 rows)

 -- test handling of temp objects created by extensions
 create extension test_ext8;
@@ -79,8 +85,13 @@ order by 1;
  function pg_temp.ext8_temp_even(posint)
  table ext8_table1
  table ext8_temp_table1
+ type ext8_table1
+ type ext8_table1[]
+ type ext8_temp_table1
+ type ext8_temp_table1[]
  type posint
-(5 rows)
+ type posint[]
+(10 rows)

 -- Should be possible to drop and recreate this extension
 drop extension test_ext8;
@@ -97,8 +108,13 @@ order by 1;
  function pg_temp.ext8_temp_even(posint)
  table ext8_table1
  table ext8_temp_table1
+ type ext8_table1
+ type ext8_table1[]
+ type ext8_temp_table1
+ type ext8_temp_table1[]
  type posint
-(5 rows)
+ type posint[]
+(10 rows)

 -- here we want to start a new session and wait till old one is gone
 select pg_backend_pid() as oldpid \gset
@@ -117,8 +133,11 @@ Objects in extension "test_ext8"
 ----------------------------
  function ext8_even(posint)
  table ext8_table1
+ type ext8_table1
+ type ext8_table1[]
  type posint
-(3 rows)
+ type posint[]
+(6 rows)

 -- dropping it should still work
 drop extension test_ext8;
@@ -237,9 +256,12 @@ Objects in extension "test_ext_cor"
 ------------------------------
  function ext_cor_func()
  operator <<@@(point,polygon)
+ type ext_cor_view
+ type ext_cor_view[]
  type test_ext_type
+ type test_ext_type[]
  view ext_cor_view
-(4 rows)
+(7 rows)

 --
 -- CREATE IF NOT EXISTS is an entirely unsound thing for an extension
@@ -295,7 +317,13 @@ Objects in extension "test_ext_cine"
  server ext_cine_srv
  table ext_cine_tab1
  table ext_cine_tab2
-(8 rows)
+ type ext_cine_mv
+ type ext_cine_mv[]
+ type ext_cine_tab1
+ type ext_cine_tab1[]
+ type ext_cine_tab2
+ type ext_cine_tab2[]
+(14 rows)

 ALTER EXTENSION test_ext_cine UPDATE TO '1.1';
 \dx+ test_ext_cine
@@ -311,7 +339,15 @@ Objects in extension "test_ext_cine"
  table ext_cine_tab1
  table ext_cine_tab2
  table ext_cine_tab3
-(9 rows)
+ type ext_cine_mv
+ type ext_cine_mv[]
+ type ext_cine_tab1
+ type ext_cine_tab1[]
+ type ext_cine_tab2
+ type ext_cine_tab2[]
+ type ext_cine_tab3
+ type ext_cine_tab3[]
+(17 rows)

 --
 -- Test @extschema@ syntax.