On Thu, Jun 29, 2023 at 10:06:35AM +0300, Heikki Linnakangas wrote: > The error messages like "failed to change schema dependency for extension" > don't conform to the usual error message style. "could not change schema > dependency for extension" would be more conformant. I see that you > copy-pasted that from existing messages, and we have a bunch of other > "failed to" messages in the repository too, so I'm OK with leaving it as it > is for now. Or maybe change the wording of all the changeDependencyFor() > callers now, and consider all the other "failed to" messages separately > later.
I'm OK to change the messages for all changeDependencyFor() now that these are being touched. I am counting 7 of them. > If changeDependencyFor() returns >= 2, the message is a bit misleading. > That's what the existing callers did too, so maybe that's fine. > > I can hit the above error with the attached test case. That seems wrong, > although I don't know if it means that the check is wrong or it exposed a > long-standing bug. Coming back to this one, I think that my check and you have found an old bug in AlterExtensionNamespace() where the sequence of objects manipulated breaks the namespace OIDs used to change the normal dependency of the extension when calling changeDependencyFor(). The check I have added looks actually correct to me because there should be always have one 'n' pg_depend entry to change between the extension and its schema, and we should always change it. A little bit of debugging is showing me that at the stage of "ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep3;", oldNspOid is set to the OID of test_func_dep2, and nspOid is the OID of test_func_dep3. So the new OID is correct, but the old one points to the schema test_func_dep2 used by the function because it is the first object it has been picked up while scanning pg_depend, and not the schema test_func_dep1 used by the extension. This causes the command to fail to update the schema dependency between the schema and the extension. The origin of the confusing comes to the handling of oldNspOid, in my opinion. I don't quite see why it is necessary to save the old OID of the namespace from the object scanned while we know the previous schema used by the extension thanks to its pg_extension entry. Also, note that there is a check in AlterExtensionNamespace() to prevent the command from happening if an object is not in the same schema as the extension, but it fails to trigger here. I have written a couple of extra queries to show the difference. Please find attached a patch to fix this issue with ALTER EXTENSION .. SET SCHEMA, and the rest. The patch does everything discussed, but it had better be split into two patches for different branches. Here are my thoughts: - Fix and backpatch the ALTER EXTENSION business, *without* the new sanity check for changeDependencyFor() in AlterExtensionNamespace(), with its regression test. - Add all the sanity checks and reword the error messages related to changeDependencyFor() only on HEAD. Thoughts? -- Michael
From c2ae83420f8bc4b9b80bfbad313b240bc406a60c Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 5 Jul 2023 15:30:01 +0900
Subject: [PATCH v2] Fix dependency handling with ALTER EXTENSION .. SET SCHEMA
The error message rewordings related to changeDependencyFor() should be
split into their own patch..
---
src/backend/commands/alter.c | 6 ++-
src/backend/commands/cluster.c | 4 +-
src/backend/commands/extension.c | 17 ++++----
src/backend/commands/functioncmds.c | 10 +++--
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/typecmds.c | 2 +-
.../expected/test_extensions.out | 43 +++++++++++++++++++
.../test_extensions/sql/test_extensions.sql | 32 ++++++++++++++
8 files changed, 98 insertions(+), 18 deletions(-)
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index e95dc31bde..e12db795b4 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -848,8 +848,10 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
pfree(replaces);
/* update dependencies to point to the new schema */
- changeDependencyFor(classId, objid,
- NamespaceRelationId, oldNspOid, nspOid);
+ if (changeDependencyFor(classId, objid,
+ NamespaceRelationId, oldNspOid, nspOid) != 1)
+ elog(ERROR, "could not change schema dependency for object %u",
+ objid);
InvokeObjectPostAlterHook(classId, objid, 0);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 03b24ab90f..4d10cc0483 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1273,7 +1273,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
AccessMethodRelationId,
relam1,
relam2) != 1)
- elog(ERROR, "failed to change access method dependency for relation \"%s.%s\"",
+ elog(ERROR, "could not change access method dependency for relation \"%s.%s\"",
get_namespace_name(get_rel_namespace(r1)),
get_rel_name(r1));
if (changeDependencyFor(RelationRelationId,
@@ -1281,7 +1281,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
AccessMethodRelationId,
relam2,
relam1) != 1)
- elog(ERROR, "failed to change access method dependency for relation \"%s.%s\"",
+ elog(ERROR, "could not change access method dependency for relation \"%s.%s\"",
get_namespace_name(get_rel_namespace(r2)),
get_rel_name(r2));
}
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 0eabe18335..db3d860e38 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -2750,7 +2750,7 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
{
Oid extensionOid;
Oid nspOid;
- Oid oldNspOid = InvalidOid;
+ Oid oldNspOid;
AclResult aclresult;
Relation extRel;
ScanKeyData key[2];
@@ -2833,6 +2833,9 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
objsMoved = new_object_addresses();
+ /* store the OID of the namespace to-be-changed */
+ oldNspOid = extForm->extnamespace;
+
/*
* Scan pg_depend to find objects that depend directly on the extension,
* and alter each one's schema.
@@ -2912,12 +2915,6 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
nspOid,
objsMoved);
- /*
- * Remember previous namespace of first object that has one
- */
- if (oldNspOid == InvalidOid && dep_oldNspOid != InvalidOid)
- oldNspOid = dep_oldNspOid;
-
/*
* If not all the objects had the same old namespace (ignoring any
* that are not in namespaces), complain.
@@ -2948,8 +2945,10 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
table_close(extRel, RowExclusiveLock);
/* update dependencies to point to the new schema */
- changeDependencyFor(ExtensionRelationId, extensionOid,
- NamespaceRelationId, oldNspOid, nspOid);
+ if (changeDependencyFor(ExtensionRelationId, extensionOid,
+ NamespaceRelationId, oldNspOid, nspOid) != 1)
+ elog(ERROR, "could not change schema dependency for extension %s",
+ NameStr(extForm->extname));
InvokeObjectPostAlterHook(ExtensionRelationId, extensionOid, 0);
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 49c7864c7c..7ba6a86ebe 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -1450,9 +1450,13 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
/* Add or replace dependency on support function */
if (OidIsValid(procForm->prosupport))
- changeDependencyFor(ProcedureRelationId, funcOid,
- ProcedureRelationId, procForm->prosupport,
- newsupport);
+ {
+ if (changeDependencyFor(ProcedureRelationId, funcOid,
+ ProcedureRelationId, procForm->prosupport,
+ newsupport) != 1)
+ elog(ERROR, "could not change support dependency for function %s",
+ get_func_name(funcOid));
+ }
else
{
ObjectAddress referenced;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fce5e6f220..bb90aedbac 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16608,7 +16608,7 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
NamespaceRelationId,
oldNspOid,
newNspOid) != 1)
- elog(ERROR, "failed to change schema dependency for relation \"%s\"",
+ elog(ERROR, "could not change schema dependency for relation \"%s\"",
NameStr(classForm->relname));
}
if (!already_done)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 216482095d..682332bdfb 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -4059,7 +4059,7 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
!isImplicitArray)
if (changeDependencyFor(TypeRelationId, typeOid,
NamespaceRelationId, oldNspOid, nspOid) != 1)
- elog(ERROR, "failed to change schema dependency for type %s",
+ elog(ERROR, "could not change schema dependency for type %s",
format_type_be(typeOid));
InvokeObjectPostAlterHook(TypeRelationId, typeOid, 0);
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out
index a31775a260..254bd938be 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -312,6 +312,49 @@ Objects in extension "test_ext_cine"
table ext_cine_tab3
(9 rows)
+--
+-- Test extension with objects outside the extension's schema.
+--
+CREATE SCHEMA test_func_dep1;
+CREATE SCHEMA test_func_dep2;
+CREATE SCHEMA test_func_dep3;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1;
+ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2;
+SELECT pg_describe_object(classid, objid, objsubid) as obj,
+ pg_describe_object(refclassid, refobjid, refobjsubid) as objref,
+ deptype
+ FROM pg_depend
+ WHERE classid = 'pg_extension'::regclass AND
+ objid = (SELECT oid FROM pg_extension WHERE extname = 'test_ext_req_schema1')
+ ORDER BY 1, 2;
+ obj | objref | deptype
+--------------------------------+-----------------------+---------
+ extension test_ext_req_schema1 | schema test_func_dep1 | n
+(1 row)
+
+-- fails, as function dep_req1 is not in the same schema as the extension.
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep3;
+ERROR: extension "test_ext_req_schema1" does not support SET SCHEMA
+DETAIL: function test_func_dep2.dep_req1() is not in the extension's schema "test_func_dep1"
+-- Move back the function, and the extension can be moved.
+ALTER FUNCTION test_func_dep2.dep_req1() SET SCHEMA test_func_dep1;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep3;
+SELECT pg_describe_object(classid, objid, objsubid) as obj,
+ pg_describe_object(refclassid, refobjid, refobjsubid) as objref,
+ deptype
+ FROM pg_depend
+ WHERE classid = 'pg_extension'::regclass AND
+ objid = (SELECT oid FROM pg_extension WHERE extname = 'test_ext_req_schema1')
+ ORDER BY 1, 2;
+ obj | objref | deptype
+--------------------------------+-----------------------+---------
+ extension test_ext_req_schema1 | schema test_func_dep3 | n
+(1 row)
+
+DROP EXTENSION test_ext_req_schema1 CASCADE;
+DROP SCHEMA test_func_dep1;
+DROP SCHEMA test_func_dep2;
+DROP SCHEMA test_func_dep3;
--
-- Test @extschema:extname@ syntax and no_relocate option
--
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index f4947e7da6..5011086183 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -210,6 +210,38 @@ ALTER EXTENSION test_ext_cine UPDATE TO '1.1';
\dx+ test_ext_cine
+--
+-- Test extension with objects outside the extension's schema.
+--
+CREATE SCHEMA test_func_dep1;
+CREATE SCHEMA test_func_dep2;
+CREATE SCHEMA test_func_dep3;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1;
+ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2;
+SELECT pg_describe_object(classid, objid, objsubid) as obj,
+ pg_describe_object(refclassid, refobjid, refobjsubid) as objref,
+ deptype
+ FROM pg_depend
+ WHERE classid = 'pg_extension'::regclass AND
+ objid = (SELECT oid FROM pg_extension WHERE extname = 'test_ext_req_schema1')
+ ORDER BY 1, 2;
+-- fails, as function dep_req1 is not in the same schema as the extension.
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep3;
+-- Move back the function, and the extension can be moved.
+ALTER FUNCTION test_func_dep2.dep_req1() SET SCHEMA test_func_dep1;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep3;
+SELECT pg_describe_object(classid, objid, objsubid) as obj,
+ pg_describe_object(refclassid, refobjid, refobjsubid) as objref,
+ deptype
+ FROM pg_depend
+ WHERE classid = 'pg_extension'::regclass AND
+ objid = (SELECT oid FROM pg_extension WHERE extname = 'test_ext_req_schema1')
+ ORDER BY 1, 2;
+DROP EXTENSION test_ext_req_schema1 CASCADE;
+DROP SCHEMA test_func_dep1;
+DROP SCHEMA test_func_dep2;
+DROP SCHEMA test_func_dep3;
+
--
-- Test @extschema:extname@ syntax and no_relocate option
--
--
2.40.1
signature.asc
Description: PGP signature
