From e5bc9454e527b1cba97553531d8d4992892fdeef Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 4 Mar 2024 14:49:31 -0500 Subject: [PATCH] Explicitly list dependent types as extension members in pg_depend. Auto-generated array types, multirange types, and relation rowtypes are treated as dependent objects: they can't be dropped separately from the base object, nor can they have their own ownership or permissions. We previously felt that, for objects that are in an extension, only the base object needs to be listed as an extension member in pg_depend. While that's sufficient to prevent inappropriate drops, it results in undesirable answers if someone asks whether a dependent type belongs to the extension. It looks like the dependent type is just some random separately-created object that happens to depend on the base object. Notably, this results in postgres_fdw concluding that expressions involving an array type are not shippable to the remote server, even when the defining extension has been whitelisted. To fix, cause GenerateTypeDependencies to make extension dependencies for dependent types as well as their base objects, and adjust ExecAlterExtensionContentsStmt so that object addition and removal operations recurse to dependent types. The latter change means that pg_upgrade of a type-defining extension will end with the dependent type(s) now also listed as extension members, even if they were not that way in the source database. Normally we want pg_upgrade to precisely reproduce the source extension's state, but it seems desirable to make an exception here. This is arguably a bug fix, but we can't back-patch it since it causes changes in the expected contents of pg_depend. (Because it does, I've bumped catversion, even though there's no change in the immediate post-initdb catalog contents.) Tom Lane and David Geier Discussion: https://postgr.es/m/4a847c55-489f-4e8d-a664-fc6b1cbe306f@gmail.com --- src/backend/catalog/pg_type.c | 17 +- src/backend/commands/extension.c | 83 +++++++-- src/include/catalog/catversion.h | 2 +- src/test/modules/test_extensions/Makefile | 3 +- .../expected/test_extensions.out | 159 +++++++++++++++++- src/test/modules/test_extensions/meson.build | 2 + .../test_extensions/sql/test_extensions.sql | 13 ++ .../test_extensions/test_ext9--1.0.sql | 8 + .../modules/test_extensions/test_ext9.control | 3 + 9 files changed, 265 insertions(+), 25 deletions(-) create mode 100644 src/test/modules/test_extensions/test_ext9--1.0.sql create mode 100644 src/test/modules/test_extensions/test_ext9.control diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c index d1d8fa274e..395dec8ed8 100644 --- a/src/backend/catalog/pg_type.c +++ b/src/backend/catalog/pg_type.c @@ -539,7 +539,7 @@ TypeCreate(Oid newTypeOid, * 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. @@ -599,7 +599,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. An exception is @@ -624,11 +624,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/backend/commands/extension.c b/src/backend/commands/extension.c index af600d7c9a..77d8c9e186 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -129,6 +129,9 @@ static void ApplyExtensionUpdates(Oid extensionOid, char *origSchemaName, bool cascade, bool is_create); +static void ExecAlterExtensionContentsRecurse(AlterExtensionContentsStmt *stmt, + ObjectAddress extension, + ObjectAddress object); static char *read_whole_file(const char *filename, int *length); @@ -3292,7 +3295,6 @@ ExecAlterExtensionContentsStmt(AlterExtensionContentsStmt *stmt, ObjectAddress extension; ObjectAddress object; Relation relation; - Oid oldExtension; switch (stmt->objtype) { @@ -3347,6 +3349,38 @@ ExecAlterExtensionContentsStmt(AlterExtensionContentsStmt *stmt, check_object_ownership(GetUserId(), stmt->objtype, object, stmt->object, relation); + /* Do the update, recursing to any dependent objects */ + ExecAlterExtensionContentsRecurse(stmt, extension, object); + + /* Finish up */ + InvokeObjectPostAlterHook(ExtensionRelationId, extension.objectId, 0); + + /* + * If get_object_address() opened the relation for us, we close it to keep + * the reference count correct - but we retain any locks acquired by + * get_object_address() until commit time, to guard against concurrent + * activity. + */ + if (relation != NULL) + relation_close(relation, NoLock); + + return extension; +} + +/* + * ExecAlterExtensionContentsRecurse + * Subroutine for ExecAlterExtensionContentsStmt + * + * Do the bare alteration of object's membership in extension, + * without permission checks. Recurse to dependent objects, if any. + */ +static void +ExecAlterExtensionContentsRecurse(AlterExtensionContentsStmt *stmt, + ObjectAddress extension, + ObjectAddress object) +{ + Oid oldExtension; + /* * Check existing extension membership. */ @@ -3430,18 +3464,47 @@ ExecAlterExtensionContentsStmt(AlterExtensionContentsStmt *stmt, removeExtObjInitPriv(object.objectId, object.classId); } - InvokeObjectPostAlterHook(ExtensionRelationId, extension.objectId, 0); - /* - * If get_object_address() opened the relation for us, we close it to keep - * the reference count correct - but we retain any locks acquired by - * get_object_address() until commit time, to guard against concurrent - * activity. + * Recurse to any dependent objects; currently, this includes the array + * type of a base type, the multirange type associated with a range type, + * and the rowtype of a table. */ - if (relation != NULL) - relation_close(relation, NoLock); + if (object.classId == TypeRelationId) + { + ObjectAddress depobject; - return extension; + depobject.classId = TypeRelationId; + depobject.objectSubId = 0; + + /* If it has an array type, update that too */ + depobject.objectId = get_array_type(object.objectId); + if (OidIsValid(depobject.objectId)) + ExecAlterExtensionContentsRecurse(stmt, extension, depobject); + + /* If it is a range type, update the associated multirange too */ + if (type_is_range(object.objectId)) + { + depobject.objectId = get_range_multirange(object.objectId); + if (!OidIsValid(depobject.objectId)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("could not find multirange type for data type %s", + format_type_be(object.objectId)))); + ExecAlterExtensionContentsRecurse(stmt, extension, depobject); + } + } + if (object.classId == RelationRelationId) + { + ObjectAddress depobject; + + depobject.classId = TypeRelationId; + depobject.objectSubId = 0; + + /* It might not have a rowtype, but if it does, update that */ + depobject.objectId = get_rel_type_id(object.objectId); + if (OidIsValid(depobject.objectId)) + ExecAlterExtensionContentsRecurse(stmt, extension, depobject); + } } /* diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 208ffbe2a3..fc48f87104 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202403041 +#define CATALOG_VERSION_NO 202403042 #endif diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile index 1388c0fb0b..7d95d6b924 100644 --- a/src/test/modules/test_extensions/Makefile +++ b/src/test/modules/test_extensions/Makefile @@ -4,7 +4,7 @@ MODULE = test_extensions PGFILEDESC = "test_extensions - regression testing for EXTENSION support" EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \ - test_ext7 test_ext8 test_ext_cine test_ext_cor \ + test_ext7 test_ext8 test_ext9 test_ext_cine test_ext_cor \ test_ext_cyclic1 test_ext_cyclic2 \ test_ext_extschema \ test_ext_evttrig \ @@ -13,6 +13,7 @@ EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \ DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \ test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \ test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \ + test_ext9--1.0.sql \ test_ext_cine--1.0.sql test_ext_cine--1.0--1.1.sql \ test_ext_cor--1.0.sql \ test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql \ diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out index 472627a232..a7ab244e87 100644 --- a/src/test/modules/test_extensions/expected/test_extensions.out +++ b/src/test/modules/test_extensions/expected/test_extensions.out @@ -53,7 +53,13 @@ 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[] + type old_table1 + type old_table1[] +(12 rows) alter extension test_ext7 update to '2.0'; \dx+ test_ext7 @@ -62,7 +68,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 +87,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 +110,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,11 +135,119 @@ 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; +-- check handling of types as extension members +create extension test_ext9; +\dx+ test_ext9 + Objects in extension "test_ext9" + Object description +---------------------------------------------------- + cast from varbitrange to varbitmultirange + function varbitmultirange() + function varbitmultirange(varbitrange) + function varbitmultirange(varbitrange[]) + function varbitrange(bit varying,bit varying) + function varbitrange(bit varying,bit varying,text) + table sometable + type somecomposite + type somecomposite[] + type sometable + type sometable[] + type varbitmultirange + type varbitmultirange[] + type varbitrange + type varbitrange[] +(15 rows) + +alter extension test_ext9 drop type varbitrange; +\dx+ test_ext9 + Objects in extension "test_ext9" + Object description +---------------------------------------------------- + cast from varbitrange to varbitmultirange + function varbitmultirange() + function varbitmultirange(varbitrange) + function varbitmultirange(varbitrange[]) + function varbitrange(bit varying,bit varying) + function varbitrange(bit varying,bit varying,text) + table sometable + type somecomposite + type somecomposite[] + type sometable + type sometable[] +(11 rows) + +alter extension test_ext9 add type varbitrange; +\dx+ test_ext9 + Objects in extension "test_ext9" + Object description +---------------------------------------------------- + cast from varbitrange to varbitmultirange + function varbitmultirange() + function varbitmultirange(varbitrange) + function varbitmultirange(varbitrange[]) + function varbitrange(bit varying,bit varying) + function varbitrange(bit varying,bit varying,text) + table sometable + type somecomposite + type somecomposite[] + type sometable + type sometable[] + type varbitmultirange + type varbitmultirange[] + type varbitrange + type varbitrange[] +(15 rows) + +alter extension test_ext9 drop table sometable; +\dx+ test_ext9 + Objects in extension "test_ext9" + Object description +---------------------------------------------------- + cast from varbitrange to varbitmultirange + function varbitmultirange() + function varbitmultirange(varbitrange) + function varbitmultirange(varbitrange[]) + function varbitrange(bit varying,bit varying) + function varbitrange(bit varying,bit varying,text) + type somecomposite + type somecomposite[] + type varbitmultirange + type varbitmultirange[] + type varbitrange + type varbitrange[] +(12 rows) + +alter extension test_ext9 add table sometable; +\dx+ test_ext9 + Objects in extension "test_ext9" + Object description +---------------------------------------------------- + cast from varbitrange to varbitmultirange + function varbitmultirange() + function varbitmultirange(varbitrange) + function varbitmultirange(varbitrange[]) + function varbitrange(bit varying,bit varying) + function varbitrange(bit varying,bit varying,text) + table sometable + type somecomposite + type somecomposite[] + type sometable + type sometable[] + type varbitmultirange + type varbitmultirange[] + type varbitrange + type varbitrange[] +(15 rows) + +drop extension test_ext9; -- Test creation of extension in temporary schema with two-phase commit, -- which should not work. This function wrapper is useful for portability. -- Avoid noise caused by CONTEXT and NOTICE messages including the temporary @@ -237,9 +363,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 +424,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 +446,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. diff --git a/src/test/modules/test_extensions/meson.build b/src/test/modules/test_extensions/meson.build index aec99dc20d..9b8d0a1016 100644 --- a/src/test/modules/test_extensions/meson.build +++ b/src/test/modules/test_extensions/meson.build @@ -18,6 +18,8 @@ test_install_data += files( 'test_ext7.control', 'test_ext8--1.0.sql', 'test_ext8.control', + 'test_ext9--1.0.sql', + 'test_ext9.control', 'test_ext_cine--1.0.sql', 'test_ext_cine--1.0--1.1.sql', 'test_ext_cine.control', diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql index 51327cc321..c5b64f47c6 100644 --- a/src/test/modules/test_extensions/sql/test_extensions.sql +++ b/src/test/modules/test_extensions/sql/test_extensions.sql @@ -67,6 +67,19 @@ end'; -- dropping it should still work drop extension test_ext8; +-- check handling of types as extension members +create extension test_ext9; +\dx+ test_ext9 +alter extension test_ext9 drop type varbitrange; +\dx+ test_ext9 +alter extension test_ext9 add type varbitrange; +\dx+ test_ext9 +alter extension test_ext9 drop table sometable; +\dx+ test_ext9 +alter extension test_ext9 add table sometable; +\dx+ test_ext9 +drop extension test_ext9; + -- Test creation of extension in temporary schema with two-phase commit, -- which should not work. This function wrapper is useful for portability. diff --git a/src/test/modules/test_extensions/test_ext9--1.0.sql b/src/test/modules/test_extensions/test_ext9--1.0.sql new file mode 100644 index 0000000000..427070bece --- /dev/null +++ b/src/test/modules/test_extensions/test_ext9--1.0.sql @@ -0,0 +1,8 @@ +/* src/test/modules/test_extensions/test_ext9--1.0.sql */ +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_ext9" to load this file. \quit + +-- check handling of types as extension members +create type varbitrange as range (subtype = varbit); +create table sometable (f1 real, f2 real); +create type somecomposite as (f1 float8, f2 float8); diff --git a/src/test/modules/test_extensions/test_ext9.control b/src/test/modules/test_extensions/test_ext9.control new file mode 100644 index 0000000000..c36eddb178 --- /dev/null +++ b/src/test/modules/test_extensions/test_ext9.control @@ -0,0 +1,3 @@ +comment = 'test_ext9' +default_version = '1.0' +relocatable = true