Re: Add more sanity checks around callers of changeDependencyFor()
On Mon, Jul 10, 2023 at 11:04:48AM -0400, Tom Lane wrote: > ISTR that we discussed forbidding such changes way back when the > extension mechanism was invented, and decided against it on the > grounds that (a) it'd be nanny-ism, (b) we'd have to add checks in an > awful lot of places and it'd be easy to miss some, The namepace modifications depending on the object types are quite centralized lately, FWIW. And that was the case in 9.3 as well since we have ExecAlterObjectSchemaStmt(). It would be easy to miss a new code path if somebody introduces a new object type that needs its own update path, but based on the last 15 years of experience on the matter, that would be unlikely? Adding a note at the top of ExecAlterObjectSchemaStmt() would make that even harder to miss. > and (c) forbidding > superusers from doing anything they want is generally not our style. Yeah. -- Michael signature.asc Description: PGP signature
Re: Add more sanity checks around callers of changeDependencyFor()
Alvaro Herrera writes: > On 2023-Jul-10, Tom Lane wrote: >> The user has altered properties of an extension member >> object locally within the database, but has not changed the extension's >> installation script to match. > If I were developing an extension and decided, down the line, to have > some objects in another schema, I would certainly increment the > extension's version number and have a new script to move the object. I > would never expect the user to do an ALTER directly (and it makes no > sense for me as an extension developer to do it manually, either.) It's certainly poor practice, but I could see doing it early in an extension's development (while you're still working towards 1.0). ISTR that we discussed forbidding such changes way back when the extension mechanism was invented, and decided against it on the grounds that (a) it'd be nanny-ism, (b) we'd have to add checks in an awful lot of places and it'd be easy to miss some, and (c) forbidding superusers from doing anything they want is generally not our style. We could reconsider that now, but I think we'd probably land on the same place. regards, tom lane
Re: Add more sanity checks around callers of changeDependencyFor()
On 2023-Jul-10, Tom Lane wrote: > Michael Paquier writes: > > On Thu, Jul 06, 2023 at 10:09:20AM -0700, Andres Freund wrote: > >> I also don't think pg_dump will dump the changed schema, which means a > >> dump/restore leads to a different schema - IMO something to avoid. > > > Yes, you're right here. The function dumped is restored in the same > > schema as the extension. > > Actually, I think the given example demonstrates pilot error rather > than a bug. Well, if this is pilot error, why don't we throw an error ourselves? > The user has altered properties of an extension member > object locally within the database, but has not changed the extension's > installation script to match. If I were developing an extension and decided, down the line, to have some objects in another schema, I would certainly increment the extension's version number and have a new script to move the object. I would never expect the user to do an ALTER directly (and it makes no sense for me as an extension developer to do it manually, either.) -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
Re: Add more sanity checks around callers of changeDependencyFor()
Michael Paquier writes: > On Thu, Jul 06, 2023 at 10:09:20AM -0700, Andres Freund wrote: >> I also don't think pg_dump will dump the changed schema, which means a >> dump/restore leads to a different schema - IMO something to avoid. > Yes, you're right here. The function dumped is restored in the same > schema as the extension. Actually, I think the given example demonstrates pilot error rather than a bug. The user has altered properties of an extension member object locally within the database, but has not changed the extension's installation script to match. The fact that after restore, the object does again match the script is intended behavior. We've made some exceptions to that rule for permissions, but not anything else. I don't see a reason to consider the objects' schema assignments differently from other properties for this purpose. regards, tom lane
Re: Add more sanity checks around callers of changeDependencyFor()
On Fri, Jul 07, 2023 at 06:12:48PM +, Akshat Jaimini wrote: > I believe it is ready to be committed! Okay, thanks. Please note that I have backpatched the bug and added the checks for the callers of changeDependencyFor() on HEAD on top of the bugfix. -- Michael signature.asc Description: PGP signature
Re: Add more sanity checks around callers of changeDependencyFor()
On Thu, Jul 06, 2023 at 10:09:20AM -0700, Andres Freund wrote: > Well, it adds an exploitation opportunity. If other functions in the extension > reference the original location (explicitly or via search_path), somebody else > can create a function there, which might be called from a more privileged > context. Obviously permissions limit the likelihood of this being a real > issue. Yeah.. > I also don't think pg_dump will dump the changed schema, which means a > dump/restore leads to a different schema - IMO something to avoid. Yes, you're right here. The function dumped is restored in the same schema as the extension. For instance: psql postgres << EOF CREATE SCHEMA test_func_dep1; CREATE SCHEMA test_func_dep2; CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1; ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2; EOF pg_dump -f dump.sql postgres createdb popo psql -f dump.sql popo psql -c '\dx+ test_ext_req_schema1' popo Objects in extension "test_ext_req_schema1" Object description function test_func_dep1.dep_req1() (1 row) I am honestly not sure how much restrictions we should have here, as this could hurt anybody relying on the existing behavior, as well, if there are any. (It seems that that schema modification restrictions for extension objects would need to go through ExecAlterObjectSchemaStmt().) Anyway, I think that I'll just go ahead and fix the SET SCHEMA bug, as that's wrong as it stands. Regarding these restrictions, perhaps something could be done on HEAD, though it impacts usability IMO. -- Michael signature.asc Description: PGP signature
Re: Add more sanity checks around callers of changeDependencyFor()
On Thu, Jul 06, 2023 at 06:41:49PM +0200, Peter Eisentraut wrote: > On 29.06.23 01:36, Michael Paquier wrote: >> While working on a different patch, I have noted three code paths that >> call changeDependencyFor() but don't check that they do not return >> errors. In all the three cases (support function, extension/schema >> and object/schema), it seems to me that only one dependency update is >> expected. > > Why can't changeDependencyFor() raise the error itself? There is appeal in that, but I can't really get excited for any out-of-core callers of this routine. Even if you would not lose much error context, it would not be completely flexible if the number of dependencies to switch is a variable number. -- Michael signature.asc Description: PGP signature
Re: Add more sanity checks around callers of changeDependencyFor()
The patch looks fine and passes all the tests. I am using Arch Linux on an x86_64 system. The patch does not cause any unnecessary bugs and does not make any non trivial changes to the source code. I believe it is ready to be committed! The new status of this patch is: Ready for Committer
Re: Add more sanity checks around callers of changeDependencyFor()
Hi, On 2023-07-05 14:10:42 +0900, Michael Paquier wrote: > On Tue, Jul 04, 2023 at 02:40:04PM -0400, Tom Lane wrote: > > Alvaro Herrera writes: > >> Hmm, shouldn't we disallow moving the function to another schema, if the > >> function's schema was originally determined at extension creation time? > >> I'm not sure we really want to allow moving objects of an extension to a > >> different schema. > > > > Why not? I do not believe that an extension's objects are required > > to all be in the same schema. > > Yes, I don't see what we would gain by putting restrictions regarding > which schema an object is located in, depending on which schema an > extension uses. Well, it adds an exploitation opportunity. If other functions in the extension reference the original location (explicitly or via search_path), somebody else can create a function there, which might be called from a more privileged context. Obviously permissions limit the likelihood of this being a real issue. I also don't think pg_dump will dump the changed schema, which means a dump/restore leads to a different schema - IMO something to avoid. Greetings, Andres Freund
Re: Add more sanity checks around callers of changeDependencyFor()
On 29.06.23 01:36, Michael Paquier wrote: While working on a different patch, I have noted three code paths that call changeDependencyFor() but don't check that they do not return errors. In all the three cases (support function, extension/schema and object/schema), it seems to me that only one dependency update is expected. Why can't changeDependencyFor() raise the error itself?
Re: Add more sanity checks around callers of changeDependencyFor()
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 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 fo
Re: Add more sanity checks around callers of changeDependencyFor()
On Tue, Jul 04, 2023 at 02:40:04PM -0400, Tom Lane wrote: > Alvaro Herrera writes: >> Hmm, shouldn't we disallow moving the function to another schema, if the >> function's schema was originally determined at extension creation time? >> I'm not sure we really want to allow moving objects of an extension to a >> different schema. > > Why not? I do not believe that an extension's objects are required > to all be in the same schema. Yes, I don't see what we would gain by putting restrictions regarding which schema an object is located in, depending on which schema an extension uses. -- Michael signature.asc Description: PGP signature
Re: Add more sanity checks around callers of changeDependencyFor()
Alvaro Herrera writes: > Hmm, shouldn't we disallow moving the function to another schema, if the > function's schema was originally determined at extension creation time? > I'm not sure we really want to allow moving objects of an extension to a > different schema. Why not? I do not believe that an extension's objects are required to all be in the same schema. regards, tom lane
Re: Add more sanity checks around callers of changeDependencyFor()
On 2023-Jun-29, Heikki Linnakangas wrote: > 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. > +CREATE SCHEMA test_func_dep1; > +CREATE SCHEMA test_func_dep2; > +CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1; > +ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2; > + > +ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep2; > + > +DROP EXTENSION test_ext_req_schema1 CASCADE; Hmm, shouldn't we disallow moving the function to another schema, if the function's schema was originally determined at extension creation time? I'm not sure we really want to allow moving objects of an extension to a different schema. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
Re: Add more sanity checks around callers of changeDependencyFor()
On 29/06/2023 02:36, Michael Paquier wrote: Hi all, While working on a different patch, I have noted three code paths that call changeDependencyFor() but don't check that they do not return errors. In all the three cases (support function, extension/schema and object/schema), it seems to me that only one dependency update is expected. Makes sense. /* update dependencies to point to the new schema */ Suggest: "update dependency ..." in singular, as there should be only one. if (changeDependencyFor(ExtensionRelationId, extensionOid, NamespaceRelationId, oldNspOid, nspOid) != 1) elog(ERROR, "failed to change schema dependency for extension %s", NameStr(extForm->extname)); 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. 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. -- Heikki Linnakangas Neon (https://neon.tech) diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql index f4947e7da6..b6b8d9d31f 100644 --- a/src/test/modules/test_extensions/sql/test_extensions.sql +++ b/src/test/modules/test_extensions/sql/test_extensions.sql @@ -210,6 +210,17 @@ ALTER EXTENSION test_ext_cine UPDATE TO '1.1'; \dx+ test_ext_cine + +-- +CREATE SCHEMA test_func_dep1; +CREATE SCHEMA test_func_dep2; +CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1; +ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2; + +ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep2; + +DROP EXTENSION test_ext_req_schema1 CASCADE; + -- -- Test @extschema:extname@ syntax and no_relocate option --
Add more sanity checks around callers of changeDependencyFor()
Hi all,
While working on a different patch, I have noted three code paths that
call changeDependencyFor() but don't check that they do not return
errors. In all the three cases (support function, extension/schema
and object/schema), it seems to me that only one dependency update is
expected.
I am adding that to the next CF. Thoughts or comments about the
attached?
--
Michael
From 2b66afedbfd27a80beae3a48e0eb362e21ef4547 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 29 Jun 2023 08:35:23 +0900
Subject: [PATCH] Add checks for values returned by changeDependencyFor()
---
src/backend/commands/alter.c| 6 --
src/backend/commands/extension.c| 6 --
src/backend/commands/functioncmds.c | 10 +++---
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index e95dc31bde..455d8dd86a 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, "failed to change schema dependency for object %u",
+ objid);
InvokeObjectPostAlterHook(classId, objid, 0);
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 0eabe18335..e95642e14f 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -2948,8 +2948,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, "failed to 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..5ec2c002d2 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, "failed to change support dependency for function %s",
+ get_func_name(funcOid));
+ }
else
{
ObjectAddress referenced;
--
2.40.1
signature.asc
Description: PGP signature
