From: "Regina Obe" <lr@pcorp.us>
To: "'Tom Lane'" <tgl@sss.pgh.pa.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>
References: <003001d8f4ae$402282c0$c0678840$@pcorp.us> <000401d94bc8$48dff700$da9fe500$@pcorp.us> <167762779361.628976.14009177203307564086.pgcf@coridan.postgresql.org> <CAM-w4HP91M-r=h4nExwonHf03SRwKyNYnvFHX2FFoFMv0pRQBA@mail.gmail.com> <005401d95083$26ca24a0$745e6de0$@pcorp.us> <995823.1678478825@sss.pgh.pa.us>
In-Reply-To: <995823.1678478825@sss.pgh.pa.us>
Subject: RE: Ability to reference other extensions by schema in extension scripts
Date: Fri, 10 Mar 2023 15:38:25 -0500
Message-ID: <004a01d95390$43fefa40$cbfceec0$@pcorp.us>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: 7bit
Thread-Index: AQFgXZxpr0yOB+9uS1Xwc1lFBKthIAFb3UkpAlqGOBEBdWkvegLXfdbzAh2KTDWvlV6e0A==
Content-Language: en-us
Archived-At: <https://www.postgresql.org/message-id/004a01d95390%2443fefa40%24cbfceec0%24%40pcorp.us>
Precedence: bulk


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