Ability to reference other extensions by schema in extension scripts

"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 --
/messages/by-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 --
/messages/by-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:

/messages/by-id/3395418.1618352794@sss.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 /messages/by-id/000401d94bc8$48dff700$da9fe500$@pcorp.us (md5sum a7d45a32c054919d94cd4a26d7d07c20) onto current tip of the master branch being 128dd9f9eca0b633b51ffcd5b0f798fbc48ec4c0

The review written in /messages/by-id/20230228224608.ak7br5shev4wic5a@c19 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