Subject: RE: Ability to reference other extensions by schema in extension scripts

From: "Regina Obe" <[email-masked]>
Date: Mon, 6 Mar 2023 18:26:59 -0500

> It looks like this patch needs a quick rebase, there's a conflict in the
> meson.build.
> 
> I'll leave the state since presumably this would be easy to resolve but it would
> be more likely to get attention if it's actually building cleanly.
> 
> http://cfbot.cputube.org/patch_42_4023.log
> --
> Gregory Stark
> As Commitfest Manager

Attach is the patch rebased against master.

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 Mar 2023 15:07:05 -0500

"Regina Obe" <[email-masked]> writes:
> [ 0005-Allow-use-of-extschema-reqextname-to-reference.patch ]

I took a look at this.  I'm on board with the feature design,
but not so much with this undocumented restriction you added
to ALTER EXTENSION SET SCHEMA:

+		/* If an extension requires this extension
+		 * do not allow relocation */
+		if (pg_depend->deptype == DEPENDENCY_NORMAL && pg_depend->classid == ExtensionRelationId){
+			dep.classId = pg_depend->classid;
+			dep.objectId = pg_depend->objid;
+			dep.objectSubId = pg_depend->objsubid;
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot SET SCHEMA of extension %s because other extensions require it",
+							NameStr(extForm->extname)),
+					 errdetail("%s requires extension %s",
+							   getObjectDescription(&dep, false), NameStr(extForm->extname))));

That seems quite disastrous for usability, and it's making an assumption
unsupported by any evidence: that it will be a majority use-case for
dependent extensions to have used @extschema:myextension@ in a way that
would be broken by ALTER EXTENSION SET SCHEMA.

I think we should just drop this.  It might be worth putting in some
documentation notes about the hazard, instead.

If you want to work harder, perhaps a reasonable way to deal with
the issue would be to allow dependent extensions to declare that
they don't want your extension relocated.  But I do not think it's
okay to make that the default behavior, much less the only behavior.
And really, since we've gotten along without it so far, I'm not
sure that it's necessary to have it.

Another thing that's bothering me a bit is the use of
get_required_extension in execute_extension_script.  That does way
more than you really need, and passing a bunch of bogus parameter
values to it makes me uncomfortable.  The callers already have
the required extensions' OIDs at hand; it'd be better to add that list
to execute_extension_script's API instead of redoing the lookups.

			regards, tom lane

---

From: "Regina Obe" <[email-masked]>
Date: Fri, 10 Mar 2023 15:38:25 -0500

> -----Original Message-----
> From: Tom Lane [mailto:[email-masked]]
> Sent: Friday, March 10, 2023 3:07 PM
> To: Regina Obe <[email-masked]>
> Cc: 'Gregory Stark (as CFM)' <[email-masked]>; 'Sandro Santilli'
> <[email-masked]>; [email-masked]; 'Regina Obe'
> <[email-masked]>
> Subject: Re: Ability to reference other extensions by schema in extension
> scripts
> 
> I took a look at this.  I'm on board with the feature design, but not so
much
> with this undocumented restriction you added to ALTER EXTENSION SET
> SCHEMA:
> 
> +		if (pg_depend->deptype == DEPENDENCY_NORMAL &&
> pg_depend->classid == ExtensionRelationId){
> +					 errmsg("cannot SET SCHEMA of
> extension %s because other extensions require it",
> +							NameStr(extForm-
> >extname)),
> +
> getObjectDescription(&dep, false),
> +NameStr(extForm->extname))));
> 
> If you want to work harder, perhaps a reasonable way to deal with the
issue
> would be to allow dependent extensions to declare that they don't want
your
> extension relocated.  But I do not think it's okay to make that the
default
> behavior, much less the only behavior.
> And really, since we've gotten along without it so far, I'm not sure that
it's
> necessary to have it.
get_required_extension
> in execute_extension_script.  That does way more than you really need, and
> passing a bunch of bogus parameter values to it makes me uncomfortable.
> The callers already have the required extensions' OIDs at hand; it'd be
better
> to add that list to execute_extension_script's API instead of redoing the
> lookups.
> 
> 			regards, tom lane

---

From: "Regina Obe" <[email-masked]>
Date: Fri, 10 Mar 2023 16:05:46 -0500

much
> 	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
That was my thought originally too and also given the rarity of people
changing schemas
I wasn't that bothered with not forcing this.  Sandro was a bit more
bothered by not forcing it and given the default for extensions is not
relocatable, we didn't see that much of an issue with it.

issue
your
default
> behavior, much less the only behavior.

I had done that in one iteration of the patch.
We discussed this here
https://www.postgresql.org/message-id/000001d949ad%241159adc0%24340d0940%24%
40pcorp.us 

and here
https://www.postgresql.org/message-id/20230223183906.6rhtybwdpe37sri7%40c19

-  the main issue I ran into is I have to introduce another dependency type
or go with Sandro's idea of using refsubobjid for this purpose.  I think
defining a new dependency type is less likely to cause unforeseen
complications elsewhere, but did require me to expand the scope (to make
changes to pg_depend).  Which I am fine with doing, but didn't want to over
extend my reach too much.

One of my revisions tried to use DEPENDENCY_AUTO which did not work (as
Sandro discovered) and I had some other annoyances with lack of helper
functions
https://www.postgresql.org/message-id/000401d93a14%248647f540%2492d7dfc0%24%
40pcorp.us

key point:
"Why isn't there a variant getAutoExtensionsOfObject take a DEPENDENCY type
as an option so it would be more useful or is there functionality for that I
missed?"

it's
get_required_extension
better
> to add that list to execute_extension_script's API instead of redoing the

So you are proposing I change the execute_extension_scripts input args to
take more args?

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 Mar 2023 17:14:36 -0500

"Regina Obe" <[email-masked]> writes:
>> If you want to work harder, perhaps a reasonable way to deal with the issue
>> would be to allow dependent extensions to declare that they don't want your
>> extension relocated.  But I do not think it's okay to make that the default

> or go with Sandro's idea of using refsubobjid for this purpose.

No, pg_depend is not the thing to use for this.  I was thinking of a new
field in the extension's control file, right beside where it says it's
dependent on such-and-such extensions in the first place.  Say like

	requires = 'extfoo, extbar'
	no_relocate = 'extfoo'

> So you are proposing I change the execute_extension_scripts input args to

Why not?  It's local to that file, so you won't break anything.

			regards, tom lane

---

From: "Regina Obe" <[email-masked]>
Date: Fri, 10 Mar 2023 17:35:05 -0500

> No, pg_depend is not the thing to use for this.  I was thinking of a new
field in
> the extension's control file, right beside where it says it's dependent on
such-
> and-such extensions in the first place.  Say like
> 

So when no_relocate is specified, where would that live?

Would I mark the extfoo as not relocatable on CREATE / ALTER of said
extension?
Or add an extra field to pg_extension

I had tried to do that originally, e.g. instead of even bothering with such
an extra arg, just mark it as not relocatable if the extension's script
contains references to the required extension's schema.

But then what if extfoo is upgraded?

ALTER EXTENSION extfoo UPDATE;

Wipes out the not relocatable of extfoo set. 
So in order to prevent that, I have to 

a) check the control files of all extensions that depend on foo to see if
they made such a request.
or 
b) "Seeing if the extension is marked as not relocatable, prevent ALTER
EXTENSION from marking it as relocatable"
problem with b is what if the extension author changed their mind and wanted
it to be relocatable? Given the default is (not relocatable), it's possible
the author didn't know this and later decided to put in an explicit
relocate=false.
c) define a new column in pg_extension to hold this bit of info.  I was
hoping I could reuse pg_extension.extconfig, but it seems that's hardwired
to be only used for backup.

Am I missing something or is this really as complicated as I think it is?

If we go with b) I'm not sure why I need to bother defining a no_relocate,
as it's obvious looking at the extension install/upgrade script that it
should not be relocatable.

> > So you are proposing I change the execute_extension_scripts input args
> > to take more args?
> 

Okay, I wasn't absolutely sure if it was.  If it is then I'll change.

> 			regards, tom lane

Thanks,
Regina

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 Mar 2023 17:47:17 -0500

> So when no_relocate is specified, where would that live?

In the control file.

> Or add an extra field to pg_extension

We don't record dependent extensions in pg_extension now, so that
doesn't seem like it would fit well.  I was envisioning that
ALTER EXTENSION SET SCHEMA would do something along the lines of

(1) scrape the list of dependent extensions out of pg_depend
(2) open and parse each of their control files
(3) fail if any of their control files mentions the target one in
    no_relocate.

Admittedly, this'd be a bit slow, but I doubt that ALTER EXTENSION
SET SCHEMA is a performance bottleneck for anybody.

> contains references to the required extension's schema.

I don't think that's a great approach, because those references might
appear in places that can track a rename (ie, in an object name that's
resolved to a stored OID).  Short of fully parsing the script file you
aren't going to get a reliable answer.  I'm content to lay that problem
off on the extension authors.

> But then what if extfoo is upgraded?

We already have mechanisms for version-dependent control files, so
I don't see where there's a problem.

			regards, tom lane

---

From: "Regina Obe" <[email-masked]>
Date: Sat, 11 Mar 2023 03:18:18 -0500

> We don't record dependent extensions in pg_extension now, so that doesn't
> seem like it would fit well.  I was envisioning that ALTER EXTENSION SET
> SCHEMA would do something along the lines of
> 
> Admittedly, this'd be a bit slow, but I doubt that ALTER EXTENSION SET
> SCHEMA is a performance bottleneck for anybody.
> 
> > I had tried to do that originally, e.g. instead of even bothering with
> > such an extra arg, just mark it as not relocatable if the extension's
> > script contains references to the required extension's schema.
> 
appear
> in places that can track a rename (ie, in an object name that's resolved
to a
> stored OID).  Short of fully parsing the script file you aren't going to
get a
> reliable answer.  I'm content to lay that problem off on the extension
authors.
> We already have mechanisms for version-dependent control files, so I don't
> see where there's a problem.
> 
> 			regards, tom lane

Attached is a revised patch with these changes in place.

---

From: 'Sandro Santilli' <[email-masked]>
Date: Mon, 13 Mar 2023 12:59:16 +0100

> Attached is a revised patch with these changes in place.

I've given a try to this patch. It builds and regresses fine.

My own tests also worked fine. As long as ext1 was found
in the ext2's no_relocate list it could not be relocated,
and proper error message is given to user trying it.

Nitpicking, there are a few things that are weird to me:

1) I don't get any error/warning if I put an arbitrary
string into no_relocate (there's no check to verify the
no_relocate is a subset of the requires).

2) An extension can still reference extensions it depends on
without putting them in no_relocate. This may be intentional,
as some substitutions may not require blocking relocation, but
felt inconsistent with the normal @extschema@ which is never
replaced unless an extension is marked as non-relocatable.

--strk;

  Libre GIS consultant/developer
  https://strk.kbt.io/services.html

---

From: "Regina Obe" <[email-masked]>
Date: Mon, 13 Mar 2023 10:28:04 -0400

> My own tests also worked fine. As long as ext1 was found in the ext2's
> no_relocate list it could not be relocated, and proper error message is
given
> to user trying it.
> 1) I don't get any error/warning if I put an arbitrary string into
no_relocate
> (there's no check to verify the no_relocate is a subset of the requires).
> 

I thought about that and decided it wasn't worth checking for.  If an
extension author puts in an extension not in requires it's on them as the
docs say it should be in requires.

It will just pretend that extension is not listed in no_relocate.

> 2) An extension can still reference extensions it depends on without
putting
> them in no_relocate. This may be intentional, as some substitutions may
not
> require blocking relocation, but felt inconsistent with the normal
> @extschema@ which is never replaced unless an extension is marked as non-
> relocatable.
> 
> --strk;
> 

Yes this is intentional.  As Tom mentioned, if for example an extension
author decides
To schema qualify @extschema:foo@ in their table definition, and they marked
as requiring foo
since such a reference 
is captured by a schema move, there is no need for them to prevent relocate
of the foo extension (assuming foo was relocatable to begin with)

---

From: "Regina Obe" <[email-masked]>
Date: Mon, 13 Mar 2023 17:57:57 -0400

given
no_relocate
putting
not
>   https://strk.kbt.io/services.html

Attached is a slightly revised patch to fix the extra whitespace in the
extend.gml 
document that Sandro noted to me.

Thanks,
Regina

---

From: Sandro Santilli <[email-masked]>
Date: Thu, 16 Mar 2023 11:14:18 +0100

On Mon, Mar 13, 2023 at 05:57:57PM -0400, Regina Obe wrote:
> extend.gml document that Sandro noted to me.

Thanks Regina.
I've tested attached patch (md5 0b652a8271fc7e71ed5f712ac162a0ef)
against current master (hash 4ef1be5a0b676a9f030cc2e4837f4b5650ecb069).
The patch applies cleanly, builds cleanly, regresses cleanly.

I've also run my quick test and I'm satisfied with it:

  test=# create extension ext2 cascade;
  NOTICE:  installing required extension "ext1"
  CREATE EXTENSION

  test=# select ext2log('h');
  ext1: ext2: h

  test=# alter extension ext1 set schema n1;
  ERROR:  cannot SET SCHEMA of extension ext1 because other extensions prevent it
  DETAIL:  extension ext2 prevents relocation of extension ext1

  test=# drop extension ext2;
  DROP EXTENSION

  test=# alter extension ext1 set schema n1;
  ALTER EXTENSION

  test=# create extension ext2;
  CREATE EXTENSION

  test=# select ext2log('h');
  ext1: ext2: h

--strk;

---

From: Tom Lane <[email-masked]>
Date: Mon, 20 Mar 2023 18:47:20 -0400

> The patch applies cleanly, builds cleanly, regresses cleanly.

Pushed with some mostly-cosmetic adjustments (in particular I tried
to make the docs and tests neater).

I did not commit the changes in get_available_versions_for_extension
to add no_relocate as an output column.  Those were dead code because
you hadn't done anything to connect them up to an actual output parameter
of pg_available_extension_versions().  While I'm not necessarily averse
to making the no_relocate values visible somehow, I'm not convinced that
pg_available_extension_versions should be the place to do it.  ISTM what's
relevant is the no_relocate values of *installed* extensions, not those of
potentially-installable extensions.  If we had a view over pg_extension
then that might be a place to add this, but we don't.  On the whole it
didn't seem important enough to pursue, so I just left it out.

			regards, tom lane