Обсуждение: 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
Вложения
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)
Вложения
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 Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On Tue, Jul 04, 2023 at 02:40:04PM -0400, Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> 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
Вложения
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
Вложения
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?
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 <alvherre@alvh.no-ip.org> 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
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
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
Вложения
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
Вложения
On Fri, Jul 07, 2023 at 06:12:48PM +0000, 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
Вложения
Michael Paquier <michael@paquier.xyz> 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
On 2023-Jul-10, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> 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)
Alvaro Herrera <alvherre@alvh.no-ip.org> 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
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