Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add more sanity checks with callers of changeDependencyFor()

  2. Fix ALTER EXTENSION SET SCHEMA with objects outside an extension's schema

  1. Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-06-28T23:36:30Z

    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

  2. Re: Add more sanity checks around callers of changeDependencyFor()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-06-29T07:06:35Z

    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)

  3. Re: Add more sanity checks around callers of changeDependencyFor()

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-07-04T16:52:03Z

    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)

  4. Re: Add more sanity checks around callers of changeDependencyFor()

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-04T18:40:04Z

    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

  5. Re: Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-07-05T05:10:42Z

    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

  6. Re: Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-07-05T06:34:17Z

    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

  7. Re: Add more sanity checks around callers of changeDependencyFor()

    Peter Eisentraut <peter@eisentraut.org> — 2023-07-06T16:41:49Z

    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?

  8. Re: Add more sanity checks around callers of changeDependencyFor()

    Andres Freund <andres@anarazel.de> — 2023-07-06T17:09:20Z

    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

  9. Re: Add more sanity checks around callers of changeDependencyFor()

    Akshat Jaimini <destrex271@gmail.com> — 2023-07-07T18:12:48Z

    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

  10. Re: Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-07-07T23:47:21Z

    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

  11. Re: Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-07-08T04:41:13Z

    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

  12. Re: Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-07-10T04:10:45Z

    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

  13. Re: Add more sanity checks around callers of changeDependencyFor()

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-10T14:51:24Z

    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

  14. Re: Add more sanity checks around callers of changeDependencyFor()

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-07-10T14:55:06Z

    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)

  15. Re: Add more sanity checks around callers of changeDependencyFor()

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-10T15:04:48Z

    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

  16. Re: Add more sanity checks around callers of changeDependencyFor()

    Michael Paquier <michael@paquier.xyz> — 2023-07-11T01:40:37Z

    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