Thread: Ability to reference other extensions by schema in extension scripts

I think I had brought this up a while ago, but I forget what the opinion was
on the matter.

PostGIS has a number of extensions that rely on it.  For the extensions that
are packaged with PostGIS, we force them all into the same schema except for
the postgis_topology and postgis_tiger_geocoder extensions which are already
installed in dedicated schemas.

This makes it impossible for postgis_topology and postgis_tiger_geocoder to
schema qualify their use of postgis.  Other extensions like pgRouting,
h3-pg, mobilitydb have similar issues.

My proposal is this.  If you think it's a good enough idea I can work up a
patch for this.

Extensions currently are allowed to specify a requires in the control file.

I propose to use this information, to allow replacement of phrases

@extschema_nameofextension@  as a variable, where nameofextension has to be
one of the extensions listed in the requires.

The extension plumbing will then use this information to look up the schema
that the current required extensions are installed in, and replace the
variables with the schema of where the dependent extension is installed.

Does anyone see any issue with this idea. 

Thanks,
Regina

"Regina Obe" <lr@pcorp.us> writes:
> My proposal is this.  If you think it's a good enough idea I can work up a
> patch for this.
> Extensions currently are allowed to specify a requires in the control file.
> I propose to use this information, to allow replacement of phrases
> @extschema_nameofextension@  as a variable, where nameofextension has to be
> one of the extensions listed in the requires.

I have a distinct sense of deja vu here.  I think this idea, or something
isomorphic to it, was previously discussed with some other syntax details.
I'm too lazy to go searching the archives right now, but I suggest that
you try to find that discussion and see if the discussed syntax seems
better or worse than what you mention.

I think it might've been along the line of @extschema:nameofextension@,
which seems like it might be superior because colon isn't a valid
identifier character so there's less risk of ambiguity.

            regards, tom lane

> "Regina Obe" <lr@pcorp.us> writes:
> > My proposal is this.  If you think it's a good enough idea I can work
> > up a patch for this.
> > Extensions currently are allowed to specify a requires in the control
file.
> > I propose to use this information, to allow replacement of phrases
> > @extschema_nameofextension@  as a variable, where nameofextension has
> > to be one of the extensions listed in the requires.
> 
> I have a distinct sense of deja vu here.  I think this idea, or something
> isomorphic to it, was previously discussed with some other syntax details.
> I'm too lazy to go searching the archives right now, but I suggest that
you try to
> find that discussion and see if the discussed syntax seems better or worse
than
> what you mention.
> 
> I think it might've been along the line of @extschema:nameofextension@,
> which seems like it might be superior because colon isn't a valid
identifier
> character so there's less risk of ambiguity.
> 
>             regards, tom lane
I found the old discussion I recalled having and Stephen had suggested using

@extschema{'postgis'}@

On this thread --
https://www.postgresql.org/message-id/20160425232251.GR10850@tamriel.snowman
.net

Is that the one you remember?

Thanks,
Regina

"Regina Obe" <lr@pcorp.us> writes:
>> I have a distinct sense of deja vu here.  I think this idea, or something
>> isomorphic to it, was previously discussed with some other syntax details.

> I found the old discussion I recalled having and Stephen had suggested using
> @extschema{'postgis'}@
> On this thread --
> https://www.postgresql.org/message-id/20160425232251.GR10850@tamriel.snowman.net
> Is that the one you remember?

Hmmm ... no, ISTM it was considerably more recent than that.
[ ...digs... ]  Here we go, it was in the discussion around
converting contrib SQL functions to new-style:

https://www.postgresql.org/message-id/flat/3395418.1618352794%40sss.pgh.pa.us

There are a few different ideas bandied around in there.
Personally I still like the @extschema:extensionname@
option the best, though.

            regards, tom lane

> "Regina Obe" <lr@pcorp.us> writes:
> >> I have a distinct sense of deja vu here.  I think this idea, or
> >> something isomorphic to it, was previously discussed with some other
> syntax details.
> 
> > I found the old discussion I recalled having and Stephen had suggested
> > using @extschema{'postgis'}@ On this thread --
> > https://www.postgresql.org/message-
> id/20160425232251.GR10850@tamriel.s
> > nowman.net
> > Is that the one you remember?
> 
> Hmmm ... no, ISTM it was considerably more recent than that.
> [ ...digs... ]  Here we go, it was in the discussion around converting
contrib SQL
> functions to new-style:
> 
> https://www.postgresql.org/message-
> id/flat/3395418.1618352794%40sss.pgh.pa.us
> 
> There are a few different ideas bandied around in there.
> Personally I still like the @extschema:extensionname@ option the best,
> though.
> 
>             regards, tom lane

I had initially thought of a syntax that could always be used even outside
of extension install as some mentioned. Like the PG_EXTENSION_SCHEMA(cube)
example. Main benefit I see with that is that even if an extension is moved,
all the dependent extensions that reference it would still work fine.

I had dismissed that because it seemed too invasive.  Seems like it would
require changes to the parser and possibly add query performance overhead to
resolve the schema. Not to mention the added testing required to do no harm.

The other reason I dismissed it is because at least for PostGIS it would be
harder to conditionally replace. The issue with
PG_EXTENSION_SCHEMA(cube) is we can't support that in lower PG versions so
we'd need to strip for lower versions, and that would introduce the
possibility of missing
PG_EXTENSION_SCHEMA(cube) vs. PG_EXTENSION_SCHEMA( cube ), not a huge deal
though, but not quite as easy and precise as just stripping
@extschema:extensionname@. References.

With the @extschema:extensionname@, it doesn't solve all problems, but the
key ones we care about like breakage of functions used in indexes,
materialized views, and added security and is a little easier to strip out.

I'll work on producing a patch.

Thanks,
Regina

> 
> "Regina Obe" <lr@pcorp.us> writes:
> >> I have a distinct sense of deja vu here.  I think this idea, or
> >> something isomorphic to it, was previously discussed with some other
> syntax details.
> 
> > I found the old discussion I recalled having and Stephen had suggested
> > using @extschema{'postgis'}@ On this thread --
> > https://www.postgresql.org/message-
> id/20160425232251.GR10850@tamriel.s
> > nowman.net
> > Is that the one you remember?
> 
> Hmmm ... no, ISTM it was considerably more recent than that.
> [ ...digs... ]  Here we go, it was in the discussion around converting
contrib SQL
> functions to new-style:
> 
> https://www.postgresql.org/message-
> id/flat/3395418.1618352794%40sss.pgh.pa.us
> 
> There are a few different ideas bandied around in there.
> Personally I still like the @extschema:extensionname@ option the best,
> though.
> 
>             regards, tom lane

Here is first version of my patch using the @extschema:extensionname@ syntax
you proposed.

This patch includes:
1) Changes to replace references of @extschema:extensionname@ with the
schema of the required extension
2) Documentation for the feature
3) Tests for the feature.

There is one issue I thought about that is not addressed by this.

If an extension is required by another extension and that required extension
schema is referenced in the extension scripts using the
@extschema:extensionname@ syntax, then ideally we should prevent the
required extension from being relocatable.  This would prevent a user from
accidentally moving the required extension, thus breaking the dependent
extensions.

I didn't add that feature cause I wasn't sure if it was overstepping the
bounds of what should be done, or if we leave it up to the user to just know
better.

Thanks,
Regina

On Tue, Nov 22, 2022 at 11:24:19PM -0500, Regina Obe wrote:

> Here is first version of my patch using the @extschema:extensionname@ syntax
> you proposed.
> 
> This patch includes:
> 1) Changes to replace references of @extschema:extensionname@ with the
> schema of the required extension
> 2) Documentation for the feature
> 3) Tests for the feature.
> 
> There is one issue I thought about that is not addressed by this.
> 
> If an extension is required by another extension and that required extension
> schema is referenced in the extension scripts using the
> @extschema:extensionname@ syntax, then ideally we should prevent the
> required extension from being relocatable.  This would prevent a user from
> accidentally moving the required extension, thus breaking the dependent
> extensions.
> 
> I didn't add that feature cause I wasn't sure if it was overstepping the
> bounds of what should be done, or if we leave it up to the user to just know
> better.

An alternative would be to forbid using @extschema:extensionname@ to
reference relocatable extensions. DBA can toggle relocatability of an
extension to allow it to be referenced.

--strk;

> On Tue, Nov 22, 2022 at 11:24:19PM -0500, Regina Obe wrote:
> 
> > Here is first version of my patch using the @extschema:extensionname@
> > syntax you proposed.
> >
> > This patch includes:
> > 1) Changes to replace references of @extschema:extensionname@ with the
> > schema of the required extension
> > 2) Documentation for the feature
> > 3) Tests for the feature.
> >
> > There is one issue I thought about that is not addressed by this.
> >
> > If an extension is required by another extension and that required
> > extension schema is referenced in the extension scripts using the
> > @extschema:extensionname@ syntax, then ideally we should prevent the
> > required extension from being relocatable.  This would prevent a user
> > from accidentally moving the required extension, thus breaking the
> > dependent extensions.
> >
> > I didn't add that feature cause I wasn't sure if it was overstepping
> > the bounds of what should be done, or if we leave it up to the user to
> > just know better.
> 
> An alternative would be to forbid using @extschema:extensionname@ to
> reference relocatable extensions. DBA can toggle relocatability of an
extension
> to allow it to be referenced.
> 
> --strk;
That would be hard to do in a DbaaS setup and not many users know they can
fiddle with extension control files.
Plus those would get overwritten with upgrades.

In my case for example I have postgis_tiger_geocoder that relies on both
postgis and fuzzystrmatch.
I'd rather not have to explain to users how to fiddle with the
fuzzystrmatch.control file to make it not relocatable.

But I don't think anyone would mind if it's forced after install because
it's a rare thing for people to be moving extensions to different schemas
after install.  

Thanks,
Regina

On Thu, Dec 15, 2022 at 08:04:22AM -0500, Regina Obe wrote:
> > On Tue, Nov 22, 2022 at 11:24:19PM -0500, Regina Obe wrote:
> > 
> > > If an extension is required by another extension and that required
> > > extension schema is referenced in the extension scripts using the
> > > @extschema:extensionname@ syntax, then ideally we should prevent the
> > > required extension from being relocatable.  This would prevent a user
> > > from accidentally moving the required extension, thus breaking the
> > > dependent extensions.
> > >
> > > I didn't add that feature cause I wasn't sure if it was overstepping
> > > the bounds of what should be done, or if we leave it up to the user to
> > > just know better.
> > 
> > An alternative would be to forbid using @extschema:extensionname@ to
> > reference relocatable extensions. DBA can toggle relocatability of an
> > extension to allow it to be referenced.
> 
> That would be hard to do in a DbaaS setup and not many users know they can
> fiddle with extension control files.
> Plus those would get overwritten with upgrades.

Wouldn't this also be the case if you override relocatability ?
Case:

  - Install fuzzystrmatch, marked as relocatable
  - Install ext2 depending on the former, which is them marked
    non-relocatable
  - Upgrade database -> fuzzystrmatch becomes relocatable again
  - Change fuzzystrmatch schema BREAKING ext2

Allowing to relocate a dependency of other extensions using the
@extschema@ syntax is very dangerous.

I've seen that PostgreSQL itself doesn't even bother to replace
@extschema@ IF the extension using it doesn't mark itself as
non-relocatable. For consistency this patch should basically refuse
to expand @extschema:fuzzystrmatch@ if "fuzzystrmatch" extension
is relocatable.

Changing the current behaviour of PostgreSQL could be proposed but
I don't think it's to be done in this thread ?

So my suggestion is to start consistent (do not expand if referenced
extension is relocatable).


--strk;

> 
> On Thu, Dec 15, 2022 at 08:04:22AM -0500, Regina Obe wrote:
> > > On Tue, Nov 22, 2022 at 11:24:19PM -0500, Regina Obe wrote:
> > >
> > > > If an extension is required by another extension and that required
> > > > extension schema is referenced in the extension scripts using the
> > > > @extschema:extensionname@ syntax, then ideally we should prevent
> > > > the required extension from being relocatable.  This would prevent
> > > > a user from accidentally moving the required extension, thus
> > > > breaking the dependent extensions.
> > > >
> > > > I didn't add that feature cause I wasn't sure if it was
> > > > overstepping the bounds of what should be done, or if we leave it
> > > > up to the user to just know better.
> > >
> > > An alternative would be to forbid using @extschema:extensionname@ to
> > > reference relocatable extensions. DBA can toggle relocatability of
> > > an extension to allow it to be referenced.
> >
> > That would be hard to do in a DbaaS setup and not many users know they
> > can fiddle with extension control files.
> > Plus those would get overwritten with upgrades.
> 
> Wouldn't this also be the case if you override relocatability ?
> Case:
> 
>   - Install fuzzystrmatch, marked as relocatable
>   - Install ext2 depending on the former, which is them marked
>     non-relocatable
>   - Upgrade database -> fuzzystrmatch becomes relocatable again
>   - Change fuzzystrmatch schema BREAKING ext2
> 

Somewhat. It would be an issue if someone does

ALTER EXTENSION fuzzystrmatch UPDATE;

And 

ALTER EXTENSION fuzzystrmatch SET SCHEMA a_different_schema;

Otherwise the relocatability of an already installed extension wouldn't
change even during upgrade. I haven't checked pg_upgrade, but I suspect it
wouldn't change there either.

It's my understanding that once an extension is installed, it's relocatable
status is recorded in the pg_extension table.  So it doesn't matter at that
point what the control file says. However if someone does update the
extension, then yes it would look at the control file and make it updatable
again.

I just tested this fiddling with postgis extension and moving it and then
upgrading.

UPDATE pg_extension SET extrelocatable = true where extname = 'postgis';
ALTER EXTENSION postgis SET schema postgis;

ALTER EXTENSION postgis UPDATE;
e.g. if the above is already at latest version, get notice
NOTICE:  version "3.3.2" of extension "postgis" is already installed
(and the extension is still relocatable)

-- if the extension can be upgraded
ALTER EXTENSION postgis UPDATE;

-- no longer relocatable (because postgis control file has relocatable =
false)

But honestly I don't expect this to be a huge issue, more of just an extra
safety block.
Not a bullet-proof safety block though.

> Allowing to relocate a dependency of other extensions using the
> @extschema@ syntax is very dangerous.
> 
> I've seen that PostgreSQL itself doesn't even bother to replace
@extschema@
> IF the extension using it doesn't mark itself as non-relocatable. For
consistency
> this patch should basically refuse to expand @extschema:fuzzystrmatch@ if
> "fuzzystrmatch" extension is relocatable.
> 
> Changing the current behaviour of PostgreSQL could be proposed but I don't
> think it's to be done in this thread ?
> 
> So my suggestion is to start consistent (do not expand if referenced
extension
> is relocatable).
> 
> 
> --strk;

I don't agree. That would make this patch of not much use.
For example lets revisit my postgis_tiger_geocoder which is a good bulk of
the reason why I want this.

I use indexes that use postgis_tiger_geocoder functions that call
fuzzystrmatch which causes pg_restore to break on reload and other issues,
because I'm not explicitly referencing the function schema.  With your
proposal now I got to demand the postgresql project to make fuzzystrmatch
not relocatable so I can use this feature.  It is so rare for people to go
around moving the locations of their extensions once set, that I honestly
don't think 
the ALTER EXTENSION .. UPDATE  hole is a huge deal.

I'd be more annoyed having to beg an extension provider to mark their
extension as not relocatable so that I could explicitly reference the
location of their extensions.

And even then - think about it.  I ask extension provider to make their
extension schema relocatable.  They do, but some people are using a version
before they marked it as schema relocatable.  So now if I change my code,
users can't install my extension, cause they are using a version before it
was schema relocatable and I'm using the new syntax.

What would be more bullet-proof is having an extra column in pg_extension or
adding an extra array element to pg_extension.extcondition[] that allows you
to say "Hey, don't allow this to be relocatable cause other extensions
depend on it that have explicitly referenced the schema."

Thanks,
Regina

On Mon, Jan 16, 2023 at 11:57:30PM -0500, Regina Obe wrote:

> What would be more bullet-proof is having an extra column in pg_extension or
> adding an extra array element to pg_extension.extcondition[] that allows you
> to say "Hey, don't allow this to be relocatable cause other extensions
> depend on it that have explicitly referenced the schema."

I've given this some more thoughts and I think a good 
compromise could be to add the safety net in ALTER EXTESION SET SCHEMA
so that it does not only check "extrelocatable" but also the presence
of any extension effectively depending on it, in which case the
operation could be prevented with a more useful message than
"extension does not support SET SCHEMA" (what is currently output).

Example query to determine those cases:

  SELECT e.extname, array_agg(v.name)
     FROM pg_extension e, pg_available_extension_versions v
     WHERE e.extname = ANY( v.requires )
     AND e.extrelocatable
     AND v.installed group by e.extname;

      extname    |        array_agg
  ---------------+--------------------------
   fuzzystrmatch | {postgis_tiger_geocoder}

--strk;

> On Mon, Jan 16, 2023 at 11:57:30PM -0500, Regina Obe wrote:
> 
> > What would be more bullet-proof is having an extra column in
> > pg_extension or adding an extra array element to
> > pg_extension.extcondition[] that allows you to say "Hey, don't allow
> > this to be relocatable cause other extensions depend on it that have
explicitly
> referenced the schema."
> 
> I've given this some more thoughts and I think a good compromise could be
to
> add the safety net in ALTER EXTESION SET SCHEMA so that it does not only
> check "extrelocatable" but also the presence of any extension effectively
> depending on it, in which case the operation could be prevented with a
more
> useful message than "extension does not support SET SCHEMA" (what is
> currently output).
> 
> Example query to determine those cases:
> 
>   SELECT e.extname, array_agg(v.name)
>      FROM pg_extension e, pg_available_extension_versions v
>      WHERE e.extname = ANY( v.requires )
>      AND e.extrelocatable
>      AND v.installed group by e.extname;
> 
>       extname    |        array_agg
>   ---------------+--------------------------
>    fuzzystrmatch | {postgis_tiger_geocoder}
> 
> --strk;

The only problem with the above is then it bars an extension from being
relocated even if no extensions reference their schema.  Note you wouldn't
be able to tell if an extension references a schema without analyzing the
install script.  Which is why I was thinking another property would be
better, cause that could be checked during the install/upgrade of the
dependent extensions.

I personally would be okay with this and is easier to code I think and
doesn't require structural changes, but not sure others would be as it's
taking away permissions they had before when it wasn't necessary to do so.

Thanks,
Regina

> > > Here is first version of my patch using the
> > > @extschema:extensionname@ syntax you proposed.
> > >
> > > This patch includes:
> > > 1) Changes to replace references of @extschema:extensionname@ with
> > > the schema of the required extension
> > > 2) Documentation for the feature
> > > 3) Tests for the feature.
> > >

Attached is a revised version of the original patch. It is revised to
prevent 

ALTER EXTENSION .. SET SCHEMA  if there is a dependent extension that 
references the extension in their scripts using @extschema:extensionname@
It also adds additional tests to verify that new feature.

In going thru the code base, I was tempted to add a new dependency type
instead of using the existing DEPENDENCY_AUTO.  I think this would be
cleaner, but I felt that was overstepping the area a bit, since it requires
making changes to dependency.h and dependency.c

My main concern with using DEPENDENCY_AUTO is because it was designed for
cases where an object can be dropped without need for CASCADE.  In this
case, we don't want a dependent extension to be dropped if it's required is
dropped.  However since there will already exist 
a DEPENDENCY_NORMAL between the 2 extensions, I figure we are protected
against that issue already.

The issue I ran into is there doesn't seem to be an easy way of checking if
a pg_depend record is already in place, so I ended up dropping it first with
deleteDependencyRecordsForSpecific so I wouldn't need to check and then
reading it.

The reason for that is during CREATE EXTENSION it would need to create the
dependency.
It would also need to do so with ALTER EXTENSION  .. UPDATE, since extension
could later on add it in their upgrade scripts and so there end up being
dupes after many ALTER EXTENSION UPDATE calls.


pg_depends getAutoExtensionsOfObject  seemed suited to that check, as is
done in 

alter.c ExecAlterObjectDependsStmt
        /* Avoid duplicates */
        currexts = getAutoExtensionsOfObject(address.classId,
    
address.objectId);
        if (!list_member_oid(currexts, refAddr.objectId))
            recordDependencyOn(&address, &refAddr,
DEPENDENCY_AUTO_EXTENSION);

but it is hard-coded to only check  DEPENDENCY_AUTO_EXTENSION

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?

Thanks,
Regina

On Mon, Feb 06, 2023 at 05:19:39AM -0500, Regina Obe wrote:
> 
> Attached is a revised version of the original patch. It is revised to
> prevent 
> 
> ALTER EXTENSION .. SET SCHEMA  if there is a dependent extension that 
> references the extension in their scripts using @extschema:extensionname@
> It also adds additional tests to verify that new feature.
> 
> In going thru the code base, I was tempted to add a new dependency type
> instead of using the existing DEPENDENCY_AUTO.  I think this would be
> cleaner, but I felt that was overstepping the area a bit, since it requires
> making changes to dependency.h and dependency.c
> 
> My main concern with using DEPENDENCY_AUTO is because it was designed for
> cases where an object can be dropped without need for CASCADE.  In this
> case, we don't want a dependent extension to be dropped if it's required is
> dropped.  However since there will already exist 
> a DEPENDENCY_NORMAL between the 2 extensions, I figure we are protected
> against that issue already.

I was thinking: how about using the "refobjsubid" to encode the
"level" of dependency on an extension ? Right now "refobjsubid" is
always 0 when the referenced object is an extension.
Could we consider subid=1 to mean the dependency is not only
on the extension but ALSO on it's schema location ?

Also: should we really allow extensions to rely on other extension
w/out fully-qualifying calls to their functions ? Or should it be
discouraged and thus forbidden ? If we wanted to forbid it we then
would not need to encode any additional dependency but rather always
forbid `ALTER EXTENSION .. SET SCHEMA` whenever the extension is
a dependency of any other extension.

On the code in the patch itself, I tried with this simple use case:

  - ext1, relocatable, exposes an ext1log(text) function

  - ext2, relocatable, exposes an ext2log(text) function
    calling @extschema:ext1@.ext1log()

What is not good:

    - Drop of ext1 automatically cascades to drop of ext2 without even a notice:

        test=# create extension ext2 cascade;
        NOTICE:  installing required extension "ext1"
        CREATE EXTENSION
        test=# drop extension ext1;
        DROP EXTENSION -- no WARNING, no NOTICE, ext2 is gone

What is good:

    - ext1 cannot be relocated while ext2 is loaded:

        test=# create extension ext2 cascade;
        NOTICE:  installing required extension "ext1"
        CREATE EXTENSION
        test=# alter extension ext1 set schema n1;
        ERROR:  Extension can not be relocated because dependent extension references it's location
        test=# drop extension ext2;
        DROP EXTENSION
        test=# alter extension ext1 set schema n1;
        ALTER EXTENSION

--strk;

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

> On Mon, Feb 06, 2023 at 05:19:39AM -0500, Regina Obe wrote:
> >
> > Attached is a revised version of the original patch. It is revised to
> > prevent
> >
> > ALTER EXTENSION .. SET SCHEMA  if there is a dependent extension that
> > references the extension in their scripts using
> > @extschema:extensionname@ It also adds additional tests to verify that
> new feature.
> >
> > In going thru the code base, I was tempted to add a new dependency
> > type instead of using the existing DEPENDENCY_AUTO.  I think this
> > would be cleaner, but I felt that was overstepping the area a bit,
> > since it requires making changes to dependency.h and dependency.c
> >
> > My main concern with using DEPENDENCY_AUTO is because it was designed
> > for cases where an object can be dropped without need for CASCADE.  In
> > this case, we don't want a dependent extension to be dropped if it's
> > required is dropped.  However since there will already exist a
> > DEPENDENCY_NORMAL between the 2 extensions, I figure we are protected
> > against that issue already.
> 
> I was thinking: how about using the "refobjsubid" to encode the "level" of
> dependency on an extension ? Right now "refobjsubid" is always 0 when the
> referenced object is an extension.
> Could we consider subid=1 to mean the dependency is not only on the
> extension but ALSO on it's schema location ?
> 

I like that idea.  It's only been ever used for tables I think, but I don't
see why it wouldn't apply in this case as the concept is kinda the same.
Only concern if other parts rely on this being 0.

The other question, should this just update the existing DEPENDENCY_NORMAL
extension or add a new DEPENDENCY_NORMAL between the extensions with
subid=1?


> Also: should we really allow extensions to rely on other extension w/out
fully-
> qualifying calls to their functions ? Or should it be discouraged and thus
> forbidden ? If we wanted to forbid it we then would not need to encode any
> additional dependency but rather always forbid `ALTER EXTENSION .. SET
> SCHEMA` whenever the extension is a dependency of any other extension.
> 
> On the code in the patch itself, I tried with this simple use case:
> 
>   - ext1, relocatable, exposes an ext1log(text) function
> 
>   - ext2, relocatable, exposes an ext2log(text) function
>     calling @extschema:ext1@.ext1log()
> 

This would be an okay solution to me too if everyone is okay with it.


> What is not good:
> 
>     - Drop of ext1 automatically cascades to drop of ext2 without even a
> notice:
> 
>         test=# create extension ext2 cascade;
>         NOTICE:  installing required extension "ext1"
>         CREATE EXTENSION
>         test=# drop extension ext1;
>         DROP EXTENSION -- no WARNING, no NOTICE, ext2 is gone
> 

Oops.  I don't know why I thought the normal dependency would protect
against this.  I should have tested that.  So DEPENDENCY_AUTO is not an
option to use and creating a new type of dependency seems like over stepping
the bounds of this patch.


> What is good:
> 
>     - ext1 cannot be relocated while ext2 is loaded:
> 
>         test=# create extension ext2 cascade;
>         NOTICE:  installing required extension "ext1"
>         CREATE EXTENSION
>         test=# alter extension ext1 set schema n1;
>         ERROR:  Extension can not be relocated because dependent
> extension references it's location
>         test=# drop extension ext2;
>         DROP EXTENSION
>         test=# alter extension ext1 set schema n1;
>         ALTER EXTENSION
> 
> --strk;
> 
>   Libre GIS consultant/developer
>   https://strk.kbt.io/services.html

So in conclusion we have 3 possible paths to go with this

1) Just don't allow any extensions referenced by other extensions to be
relocatable.
It will show a message something like 
"SET SCHEMA not allowed because other extensions depend on it"
Given that if you don't specify relocatable in you .control file, the assume
is relocatable = false , this isn't too far off from standard protocol.

2) Use objsubid=1 to denote that another extension explicitly references the
schema of another extension so setting schema of other extension is not
okay.  So instead of introducing another dependency, we'd update the
DEPENDENCY_NORMAL one between the two schemas with objsubid=1 instead of 0.

This has 2 approaches:

a) Update the existing DEPENDENCY_NORMAL between the two extensions setting
the objsubid=1

or 
b) Create a new DEPEDENCY_NORMAL between the two extensions with objsubid=1

I'm not sure if either has implications in backup / restore .  I suspect b
would be safer since I  suspect objsubid might be checked and this
dependency only needs checking during SET SCHEMA time.

3) Create a whole new DEPENDENCY type, perhaps calling it something like
DEPENDENCY_EXTENSION_SCHEMA

4) Just don't allow @extschema:<reqextension>@ syntax to be used unless the
<reqextension> is marked as relocatable=false.  This one I don't like
because it doesn't solve my fundamental issue of 

postgis_tiger_geocoder relying on fuzzystrmatch, which is marked as
relocatable.

The main issue I was trying to solve is my extension references
fuzzystrmatch functions in  a function used for functional indexes, and this
fails restore of table indexes because I can't schema qualify the
fuzzystrmatch extension in the backing function. 

 
If no one has any opinion, I'll go with option 1 which is the one that strk
had actually proposed before and seems least programmatically invasive, but
perhaps more annoying user facing.

My preferred would be #2

Thanks,
Regina

> So in conclusion we have 3 possible paths to go with this
> 
> 1) Just don't allow any extensions referenced by other extensions to be
> relocatable.
> It will show a message something like
> "SET SCHEMA not allowed because other extensions depend on it"
> Given that if you don't specify relocatable in you .control file, the
assume is
> relocatable = false , this isn't too far off from standard protocol.
> 
> 2) Use objsubid=1 to denote that another extension explicitly references
the
> schema of another extension so setting schema of other extension is not
okay.
> So instead of introducing another dependency, we'd update the
> DEPENDENCY_NORMAL one between the two schemas with objsubid=1
> instead of 0.
> 
> This has 2 approaches:
> 
> a) Update the existing DEPENDENCY_NORMAL between the two extensions
> setting the objsubid=1
> 
> or
> b) Create a new DEPEDENCY_NORMAL between the two extensions with
> objsubid=1
> 
> I'm not sure if either has implications in backup / restore .  I suspect b
would
> be safer since I  suspect objsubid might be checked and this dependency
only
> needs checking during SET SCHEMA time.
> 
> 3) Create a whole new DEPENDENCY type, perhaps calling it something like
> DEPENDENCY_EXTENSION_SCHEMA
> 
> 4) Just don't allow @extschema:<reqextension>@ syntax to be used unless
> the <reqextension> is marked as relocatable=false.  This one I don't like
> because it doesn't solve my fundamental issue of
> 
> postgis_tiger_geocoder relying on fuzzystrmatch, which is marked as
> relocatable.
> 
> The main issue I was trying to solve is my extension references
fuzzystrmatch
> functions in  a function used for functional indexes, and this fails
restore of
> table indexes because I can't schema qualify the fuzzystrmatch extension
in
> the backing function.
> 
> 
> If no one has any opinion, I'll go with option 1 which is the one that
strk had
> actually proposed before and seems least programmatically invasive, but
> perhaps more annoying user facing.
> 
> My preferred would be #2
> 
> Thanks,
> Regina

Attached is my revision 3 patch, which follows the proposed #1.
Don't allow schema relocation of an extension if another extension requires
it.

On Sat, Feb 25, 2023 at 03:40:24PM -0500, Regina Obe wrote:
> > On Mon, Feb 06, 2023 at 05:19:39AM -0500, Regina Obe wrote:
> > 
> > I was thinking: how about using the "refobjsubid" to encode the "level" of
> > dependency on an extension ? Right now "refobjsubid" is always 0 when the
> > referenced object is an extension.
> > Could we consider subid=1 to mean the dependency is not only on the
> > extension but ALSO on it's schema location ?
> 
> I like that idea.  It's only been ever used for tables I think, but I don't
> see why it wouldn't apply in this case as the concept is kinda the same.
> Only concern if other parts rely on this being 0.

This has to be verified, yes. But it feels to me like "must be 0" was
mostly to _allow_ for future extensions like the proposed one.

> The other question, should this just update the existing DEPENDENCY_NORMAL
> extension or add a new DEPENDENCY_NORMAL between the extensions with
> subid=1?

I'd use the existing record.

--strk;

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

On Sun, Feb 26, 2023 at 01:39:24AM -0500, Regina Obe wrote:

> > 1) Just don't allow any extensions referenced by other
> >    extensions to be relocatable.
> 
> Attached is my revision 3 patch, which follows the proposed #1.
> Don't allow schema relocation of an extension if another extension
> requires it.

I've built a version of PostgreSQL with this patch applied and I
confirm it works as expected.

The "ext1" is relocatable and creates a function ext1log():

  =# create extension ext1 schema n1;
  CREATE EXTENSION

The "ext2" is relocatable and creates a function ext2log() relying
on the ext1log() function from "ext1" extension, referencing
it via @extschema:ext1@:

  =# create extension ext2 schema n2;
  CREATE EXTENSION
  =# select n2.ext2log('hello'); -- things work here
  ext1: ext2: hello

By creating "ext2", "ext1" becomes effectively non-relocatable:

  =# alter extension ext1 set schema n2;
  ERROR:  cannot SET SCHEMA of extension ext1 because other extensions
  require it
  DETAIL:  extension ext2 requires extension ext1

Drop "ext2" makes "ext1" relocatable again:

  =# drop extension ext2;
  DROP EXTENSION
  =# alter extension ext1 set schema n2;
  ALTER EXTENSION

Upon re-creating "ext2" the referenced ext1 schema will be
the correct one:

  =# create extension ext2 schema n1;
  CREATE EXTENSION
  =# select n1.ext2log('hello');
  ext1: ext2: hello
  
The code itself builds w/out warnings with:

  mkdir build
  cd build
  ../configure
  make 2> ERR # ERR is empty

The testsuite reports all successes:

  make check
  [...]
  =======================
   All 213 tests passed.
  =======================

Since I didn't see the tests for extension in there, I've also
explicitly run that portion:

  make -C src/test/modules/test_extensions/ check
  [...]
  test test_extensions              ... ok           32 ms
  test test_extdepend               ... ok           12 ms
  [...]
  =====================
   All 2 tests passed.
  =====================


As mentioned already the downside of this patch is that it would
not be possibile to change the schema of an otherwise relocatable
extension once other extension depend on it, but I can't think of
any good reason to allow that, as it would mean dependent code
would need to always dynamically determine the install location
of the objects in that extension, which sounds dangerous, security
wise.

--strk; 

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

> On Sun, Feb 26, 2023 at 01:39:24AM -0500, Regina Obe wrote:
> 
> > > 1) Just don't allow any extensions referenced by other
> > >    extensions to be relocatable.
> >
> > Attached is my revision 3 patch, which follows the proposed #1.
> > Don't allow schema relocation of an extension if another extension
> > requires it.
> 
> I've built a version of PostgreSQL with this patch applied and I confirm
it
> works as expected.
> 
> The "ext1" is relocatable and creates a function ext1log():
> 
>   =# create extension ext1 schema n1;
>   CREATE EXTENSION
> 
> The "ext2" is relocatable and creates a function ext2log() relying on the
> ext1log() function from "ext1" extension, referencing it via
> @extschema:ext1@:
> 
>   =# create extension ext2 schema n2;
>   CREATE EXTENSION
>   =# select n2.ext2log('hello'); -- things work here
>   ext1: ext2: hello
> 
> By creating "ext2", "ext1" becomes effectively non-relocatable:
> 
>   =# alter extension ext1 set schema n2;
>   ERROR:  cannot SET SCHEMA of extension ext1 because other extensions
>   require it
>   DETAIL:  extension ext2 requires extension ext1
> 
> Drop "ext2" makes "ext1" relocatable again:
> 
>   =# drop extension ext2;
>   DROP EXTENSION
>   =# alter extension ext1 set schema n2;
>   ALTER EXTENSION
> 
> Upon re-creating "ext2" the referenced ext1 schema will be the correct
one:
> 
>   =# create extension ext2 schema n1;
>   CREATE EXTENSION
>   =# select n1.ext2log('hello');
>   ext1: ext2: hello
> 
> The code itself builds w/out warnings with:
> 
>   mkdir build
>   cd build
>   ../configure
>   make 2> ERR # ERR is empty
> 
> The testsuite reports all successes:
> 
>   make check
>   [...]
>   =======================
>    All 213 tests passed.
>   =======================
> 
> Since I didn't see the tests for extension in there, I've also explicitly
run that
> portion:
> 
>   make -C src/test/modules/test_extensions/ check
>   [...]
>   test test_extensions              ... ok           32 ms
>   test test_extdepend               ... ok           12 ms
>   [...]
>   =====================
>    All 2 tests passed.
>   =====================
> 
> 
> As mentioned already the downside of this patch is that it would not be
> possibile to change the schema of an otherwise relocatable extension once
> other extension depend on it, but I can't think of any good reason to
allow
> that, as it would mean dependent code would need to always dynamically
> determine the install location of the objects in that extension, which
sounds
> dangerous, security wise.
> 
> --strk;
> 
>   Libre GIS consultant/developer
>   https://strk.kbt.io/services.html

Oops I had forgotten to submit the updated patch strk was testing against in
my fork.
He had asked me to clean up the warnings off list and the description.

Attached is the revised.
Thanks strk for the patient help and guidance.

Thanks,
Regina

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

I've applied the patch attached to message
https://www.postgresql.org/message-id/000401d94bc8%2448dff700%24da9fe500%24%40pcorp.us(md5sum
a7d45a32c054919d94cd4a26d7d07c20)onto current tip of the master branch being 128dd9f9eca0b633b51ffcd5b0f798fbc48ec4c0
 

The review written in https://www.postgresql.org/message-id/20230228224608.ak7br5shev4wic5a%40c19 all still applies.

The `make installcheck-world`  test fails for me but the failures seem unrelated to the patch (many occurrences of
"+ERROR: function pg_input_error_info(unknown, unknown) does not exist" in the regression.diff).
 

Documentation exists for the new feature

The new status of this patch is: Ready for Committer

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

On Tue, 28 Feb 2023 at 18:44, Sandro Santilli <strk@kbt.io> wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, failed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
>
> I've applied the patch attached to message
https://www.postgresql.org/message-id/000401d94bc8%2448dff700%24da9fe500%24%40pcorp.us(md5sum
a7d45a32c054919d94cd4a26d7d07c20)onto current tip of the master branch being 128dd9f9eca0b633b51ffcd5b0f798fbc48ec4c0 
>
> The review written in https://www.postgresql.org/message-id/20230228224608.ak7br5shev4wic5a%40c19 all still applies.
>
> The `make installcheck-world`  test fails for me but the failures seem unrelated to the patch (many occurrences of
"+ERROR: function pg_input_error_info(unknown, unknown) does not exist" in the regression.diff). 
>
> Documentation exists for the new feature
>
> The new status of this patch is: Ready for Committer



--
Gregory Stark
As Commitfest Manager

> 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
>
> On Tue, 28 Feb 2023 at 18:44, Sandro Santilli <strk@kbt.io> wrote:
> >
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, failed
> > Implements feature:       tested, passed
> > Spec compliant:           tested, passed
> > Documentation:            tested, passed
> >
> > I've applied the patch attached to message
> > https://www.postgresql.org/message-
> id/000401d94bc8%2448dff700%24da9fe5
> > 00%24%40pcorp.us (md5sum a7d45a32c054919d94cd4a26d7d07c20) onto
> > current tip of the master branch being
> > 128dd9f9eca0b633b51ffcd5b0f798fbc48ec4c0
> >
> > The review written in https://www.postgresql.org/message-
> id/20230228224608.ak7br5shev4wic5a%40c19 all still applies.
> >
> > The `make installcheck-world`  test fails for me but the failures seem
> unrelated to the patch (many occurrences of "+ERROR:  function
> pg_input_error_info(unknown, unknown) does not exist" in the
> regression.diff).
> >
> > Documentation exists for the new feature
> >
> > The new status of this patch is: Ready for Committer
>
>
>
> --
> Gregory Stark
> As Commitfest Manager

Just sent a note about the wildcard one.  Was this conflicting with the wildcard one or some other.
I can rebase if it was conflicting with another one, if it was the wildcard one, then maybe we should commit this one
andwe'll rebase the wildcard one. 

We would like to submit the wildcard one too, but I think Tom had some reservations on that one.

Thanks,
Regina

> 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.

"Regina Obe" <lr@pcorp.us> 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

> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Sent: Friday, March 10, 2023 3:07 PM
> To: Regina Obe <lr@pcorp.us>
> Cc: 'Gregory Stark (as CFM)' <stark.cfm@gmail.com>; 'Sandro Santilli'
> <strk@kbt.io>; pgsql-hackers@lists.postgresql.org; 'Regina Obe'
> <r@pcorp.us>
> Subject: Re: Ability to reference other extensions by schema in extension
> scripts
> 
> "Regina Obe" <lr@pcorp.us> 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

> "Regina Obe" <lr@pcorp.us> 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.
> 
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.


> 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.

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?"


> 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

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

"Regina Obe" <lr@pcorp.us> 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
>> behavior, much less the only behavior.

> -  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.

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
> take more args?

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

            regards, tom lane

> 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 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?
> 
> Why not?  It's local to that file, so you won't break anything.
> 

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

>             regards, tom lane


Thanks,
Regina

"Regina Obe" <lr@pcorp.us> writes:
>> requires = 'extfoo, extbar'
>> no_relocate = 'extfoo'

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

In the control file.

> Would I mark the extfoo as not relocatable on CREATE / ALTER of said
> extension?
> 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.

> 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.

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

> "Regina Obe" <lr@pcorp.us> writes:
> >> requires = 'extfoo, extbar'
> >> no_relocate = 'extfoo'
> 
> > So when no_relocate is specified, where would that live?
> 
> In the control file.
> 
> > Would I mark the extfoo as not relocatable on CREATE / ALTER of said
> > extension?
> > 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.
> 

Okay I'll move ahead with this approach.

Thanks,
Regina

> Subject: Re: Ability to reference other extensions by schema in extension
> scripts
> 
> "Regina Obe" <lr@pcorp.us> writes:
> >> requires = 'extfoo, extbar'
> >> no_relocate = 'extfoo'
> 
> > So when no_relocate is specified, where would that live?
> 
> In the control file.
> 
> > Would I mark the extfoo as not relocatable on CREATE / ALTER of said
> > extension?
> > 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.
> 
> > 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.
> 
> 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

Attached is a revised patch with these changes in place.

On Sat, Mar 11, 2023 at 03:18:18AM -0500, Regina Obe wrote:
> 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

> 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).
> 

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;
> 
>   Libre GIS consultant/developer
>   https://strk.kbt.io/services.html


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)

> On Sat, Mar 11, 2023 at 03:18:18AM -0500, Regina Obe wrote:
> > 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

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

Thanks,
Regina

On Mon, Mar 13, 2023 at 05:57:57PM -0400, Regina Obe wrote:
> 
> Attached is a slightly revised patch to fix the extra whitespace in the
> 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;

Sandro Santilli <strk@kbt.io> writes:
> On Mon, Mar 13, 2023 at 05:57:57PM -0400, Regina Obe wrote:
>> Attached is a slightly revised patch to fix the extra whitespace in the
>> 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.

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

> 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


Thanks.  Agree with get_available_versions_for_extension, not necessary.

Thanks,
Regina

"Regina Obe" <lr@pcorp.us> writes:
>> 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.

> Thanks.  Agree with get_available_versions_for_extension, not necessary.

If we did feel like doing something about this, on reflection I think
the thing to do would be to add no_relocate as an actual column in
pg_extension, probably of type "oid[]".  Then we could modify the
SET SCHEMA code to check that instead of parsing the extension control
files.  That'd be a little cleaner, but I can't say that I'm hugely
excited about it.

            regards, tom lane