Date: Thu, 23 Feb 2023 19:39:06 +0100
From: Sandro Santilli <strk@kbt.io>
Sender: strk@kbt.io
To: Regina Obe <lr@pcorp.us>
Cc: 'Tom Lane' <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org
Subject: Re: Ability to reference other extensions by schema in extension
 scripts
Message-ID: <20230223183906.6rhtybwdpe37sri7@c19>
Mail-Followup-To: Sandro Santilli <strk@kbt.io>, Regina Obe <lr@pcorp.us>,
	'Tom Lane' <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org
References: <003001d8f4ae$402282c0$c0678840$@pcorp.us>
 <79300.1668052170@sss.pgh.pa.us>
 <004501d8f4bf$1213fba0$363bf2e0$@pcorp.us>
 <175298.1668101858@sss.pgh.pa.us>
 <004b01d8fef3$752dbf30$5f893d90$@pcorp.us>
 <20221215085234.rpjzlm7hmsmbanqv@c19>
 <000401d93a14$8647f540$92d7dfc0$@pcorp.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <000401d93a14$8647f540$92d7dfc0$@pcorp.us>
Archived-At: <https://www.postgresql.org/message-id/20230223183906.6rhtybwdpe37sri7%40c19>
Precedence: bulk

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