public inbox for [email protected]  

help / color / mirror / Atom feed

RE: Ability to reference other extensions by schema in extension scripts
14+ messages / 5 participants
[nested] [flat]

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-06 23:26  Regina Obe <[email protected]>
  0 siblings, 1 reply; 14+ messages in thread

From: Regina Obe @ 2023-03-06 23:26 UTC (permalink / raw)
  To: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; +Cc: [email protected]; 'Regina Obe' <[email protected]>

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




Attachments:

  [application/octet-stream] 0005-Allow-use-of-extschema-reqextname-to-reference.patch (15.1K, ../../[email protected]/2-0005-Allow-use-of-extschema-reqextname-to-reference.patch)
  download | inline diff:
From dce6168357e9ed410f94deb66b552129884195ba Mon Sep 17 00:00:00 2001
From: Regina Obe <[email protected]>
Date: Sun, 5 Feb 2023 01:35:29 -0500
Subject: [PATCH 5/5] Allow @extschema:<extname>@ in extension scripts

This feature allows an extension author to reference
extension schemas of extensions in the requires variable
of the control file using variable syntax
@extschema:<extname>@  where <extname> is the
name of any of the listed required extensions.

This feature is needed particularly in cases
when another extension packages a function
which references an object from one of its
required extensions and this function is used to back
indexes, constraints, and materialized views.

Without this feature, because pg_restore removes the paths,
these indexes, constraints, materialized views are not restored
because when the function is called, the function call fails
because items it depends on cannot be schema qualified.

This feature will allow extension authors to schema qualify
references to required extension objects to prevent this issue.
Aside from this issue, schema qualifying all these references will
improve security by preventing a rogue version of a function from being
used.

 - Extension tests both Makefile and meson.build
 - Documentation of the new feature
 - Prevent an extension from being relocated if
   another extension requires it

 Discussion: https://postgr.es/m/000801d94959%2463a9f520%242afddf60%24%40pcorp.us
---
 doc/src/sgml/extend.sgml                      | 17 +++++++
 src/backend/commands/extension.c              | 49 +++++++++++++++++++
 src/test/modules/test_extensions/Makefile     |  9 +++-
 .../expected/test_extensions.out              | 40 +++++++++++++++
 src/test/modules/test_extensions/meson.build  |  7 +++
 .../test_extensions/sql/test_extensions.sql   | 14 ++++++
 .../test_ext_req_schema1--1.0.sql             |  6 +++
 .../test_ext_req_schema1.control              |  3 ++
 .../test_ext_req_schema2--1.0--2.0.sql        |  7 +++
 .../test_ext_req_schema2--1.0.sql             |  9 ++++
 .../test_ext_req_schema2.control              |  4 ++
 .../test_ext_req_schema3--1.0.sql             | 13 +++++
 .../test_ext_req_schema3.control              |  4 ++
 13 files changed, 180 insertions(+), 2 deletions(-)
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema1.control
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2.control
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema3.control

diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index b70cbe83ae..6c93bddb36 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -908,6 +908,23 @@ RETURNS anycompatible AS ...
       </para>
      </listitem>
 
+     <listitem>
+      <para>
+       An extension might depend on other extensions.
+       It is useful to schema qualify calls to dependent extension to minimize reliance on search_path.
+       This is critical for cases such as functions used in indexes, materialized views, or check constraints.
+       To reference a required extension's schema, you must first have <literal>requires</literal>
+       variable specifying the list of extensions your extension requires.
+       In your extension sql scripts,
+       you can reference a required extension's schema with syntax
+       <literal>@extschema:reqextname@</literal> where <literal>reqextname</literal>
+       is the name of an extension in your <literal>requires</literal> list.
+       All occurrences of this string will be
+       replaced by the schema the required extension is installed in before the script is
+       executed.
+      </para>
+     </listitem>
+
      <listitem>
       <para>
        If the extension does not support relocation at all, set
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index b1509cc505..17ebb1a4fc 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -1030,6 +1030,41 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 											CStringGetTextDatum(qSchemaName));
 		}
 
+		/*
+		* If this extension requires other extensions
+		* Check each required extension to see if its schema
+		* is referenced by @extschema:reqextname@ syntax
+		*/
+		foreach(lc, control->requires)
+		{
+			char		*curreq = (char *) lfirst(lc);
+			Oid			reqext;
+			Oid			reqschema;
+			StringInfoData rToken;
+			char		*reqname;
+			reqext = get_required_extension(curreq,
+											control->name,
+											(char *) schemaName,
+											false,
+											NIL,
+											false);
+			reqschema = get_extension_schema(reqext);
+			reqname = get_namespace_name(reqschema);
+			initStringInfo(&rToken);
+			appendStringInfo(&rToken, "%s%s%s", "@extschema:", curreq, "@");
+
+			/*
+			* If the required extension's schema is referenced
+			* by variable name,
+			* Replace each occurence of @extschema:<reqextname>@
+			* with the required extension's schema
+			*/
+			t_sql = DirectFunctionCall3Coll(replace_text,
+								C_COLLATION_OID,
+								t_sql,
+								CStringGetTextDatum(rToken.data),
+								CStringGetTextDatum(quote_identifier(reqname)));
+		}
 		/*
 		 * If module_pathname was set in the control file, substitute its
 		 * value for occurrences of MODULE_PATHNAME.
@@ -2816,6 +2851,20 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 		ObjectAddress dep;
 		Oid			dep_oldNspOid;
 
+		/* 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))));
+
+		}
 		/*
 		 * Ignore non-membership dependencies.  (Currently, the only other
 		 * case we could see here is a normal dependency from another
diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile
index c3139ab0fc..c073df963c 100644
--- a/src/test/modules/test_extensions/Makefile
+++ b/src/test/modules/test_extensions/Makefile
@@ -6,14 +6,19 @@ PGFILEDESC = "test_extensions - regression testing for EXTENSION support"
 EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \
             test_ext7 test_ext8 test_ext_cine test_ext_cor \
             test_ext_cyclic1 test_ext_cyclic2 \
-            test_ext_evttrig
+            test_ext_evttrig \
+            test_ext_req_schema1 test_ext_req_schema2 test_ext_req_schema3
+
 DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \
        test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \
        test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \
        test_ext_cine--1.0.sql test_ext_cine--1.0--1.1.sql \
        test_ext_cor--1.0.sql \
        test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql \
-       test_ext_evttrig--1.0.sql test_ext_evttrig--1.0--2.0.sql
+       test_ext_evttrig--1.0.sql test_ext_evttrig--1.0--2.0.sql \
+       test_ext_req_schema1--1.0.sql \
+       test_ext_req_schema2--1.0.sql test_ext_req_schema2--1.0--2.0.sql \
+       test_ext_req_schema3--1.0.sql
 
 REGRESS = test_extensions test_extdepend
 
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out
index 821fed38d1..21dfd05982 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -312,3 +312,43 @@ Objects in extension "test_ext_cine"
  table ext_cine_tab3
 (9 rows)
 
+CREATE SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema3 CASCADE;
+NOTICE:  installing required extension "test_ext_req_schema2"
+SELECT dep_req();
+ dep_req 
+---------
+ 1032w
+(1 row)
+
+SELECT dep_req2();
+ dep_req2 
+----------
+ 1032w
+(1 row)
+
+SELECT dep_req3();
+ dep_req3 
+----------
+ 2032w
+(1 row)
+
+ALTER EXTENSION test_ext_req_schema2 UPDATE TO '2.0';
+CREATE SCHEMA test_s_dep2;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_s_dep2;
+ERROR:  cannot SET SCHEMA of extension test_ext_req_schema1 because other extensions require it
+DETAIL:  extension test_ext_req_schema3 requires extension test_ext_req_schema1
+SELECT dep_req();
+ dep_req 
+---------
+ 1update
+(1 row)
+
+DROP EXTENSION test_ext_req_schema1;
+ERROR:  cannot drop extension test_ext_req_schema1 because other objects depend on it
+DETAIL:  extension test_ext_req_schema2 depends on extension test_ext_req_schema1
+extension test_ext_req_schema3 depends on extension test_ext_req_schema1
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+DROP EXTENSION test_ext_req_schema3;
+DROP EXTENSION test_ext_req_schema2;
diff --git a/src/test/modules/test_extensions/meson.build b/src/test/modules/test_extensions/meson.build
index c3af3e1721..7f47a6fc23 100644
--- a/src/test/modules/test_extensions/meson.build
+++ b/src/test/modules/test_extensions/meson.build
@@ -30,6 +30,13 @@ test_install_data += files(
   'test_ext_evttrig--1.0--2.0.sql',
   'test_ext_evttrig--1.0.sql',
   'test_ext_evttrig.control',
+  'test_ext_req_schema1--1.0.sql',
+  'test_ext_req_schema1.control',
+  'test_ext_req_schema2--1.0.sql',
+  'test_ext_req_schema2.control',
+  'test_ext_req_schema2--1.0--2.0.sql',
+  'test_ext_req_schema3.control',
+  'test_ext_req_schema3--1.0.sql',
 )
 
 tests += {
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index 41b6cddf0b..81ecd13736 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -209,3 +209,17 @@ CREATE EXTENSION test_ext_cine;
 ALTER EXTENSION test_ext_cine UPDATE TO '1.1';
 
 \dx+ test_ext_cine
+
+CREATE SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema3 CASCADE;
+SELECT dep_req();
+SELECT dep_req2();
+SELECT dep_req3();
+ALTER EXTENSION test_ext_req_schema2 UPDATE TO '2.0';
+CREATE SCHEMA test_s_dep2;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_s_dep2;
+SELECT dep_req();
+DROP EXTENSION test_ext_req_schema1;
+DROP EXTENSION test_ext_req_schema3;
+DROP EXTENSION test_ext_req_schema2;
\ No newline at end of file
diff --git a/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
new file mode 100644
index 0000000000..462fb52145
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
@@ -0,0 +1,6 @@
+/* src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema1" to load this file. \quit
+
+CREATE DOMAIN req AS text
+  CONSTRAINT starts_with_1 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '1');
diff --git a/src/test/modules/test_extensions/test_ext_req_schema1.control b/src/test/modules/test_extensions/test_ext_req_schema1.control
new file mode 100644
index 0000000000..9ea4558a90
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema1.control
@@ -0,0 +1,3 @@
+comment = 'Create required extension to be referenced'
+default_version = '1.0'
+relocatable = true
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql b/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
new file mode 100644
index 0000000000..73a44a25e5
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
@@ -0,0 +1,7 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+
+CREATE OR REPLACE FUNCTION dep_req() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1update''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
new file mode 100644
index 0000000000..9fe25d48a1
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
@@ -0,0 +1,9 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+CREATE DOMAIN dreq AS text
+  CONSTRAINT starts_with_2 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '2');
+
+CREATE FUNCTION dep_req() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1032w''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2.control b/src/test/modules/test_extensions/test_ext_req_schema2.control
new file mode 100644
index 0000000000..d2ba5add97
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2.control
@@ -0,0 +1,4 @@
+comment = 'Test schema referencing of required extensions'
+default_version = '1.0'
+relocatable = true
+requires = 'test_ext_req_schema1'
diff --git a/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
new file mode 100644
index 0000000000..bd05669097
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
@@ -0,0 +1,13 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+CREATE DOMAIN req2 AS text
+  CONSTRAINT starts_with_2 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '2');
+
+CREATE FUNCTION dep_req2() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1032w''::@extschema:[email protected]';
+
+CREATE FUNCTION dep_req3() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''2032w''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema3.control b/src/test/modules/test_extensions/test_ext_req_schema3.control
new file mode 100644
index 0000000000..b052fad785
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema3.control
@@ -0,0 +1,4 @@
+comment = 'Test schema referencing of 2 required extensions'
+default_version = '1.0'
+relocatable = true
+requires = 'test_ext_req_schema1,test_ext_req_schema2'
-- 
2.21.0.windows.1



^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* Re: Ability to reference other extensions by schema in extension scripts
@ 2023-03-10 20:07  Tom Lane <[email protected]>
  parent: Regina Obe <[email protected]>
  0 siblings, 2 replies; 14+ messages in thread

From: Tom Lane @ 2023-03-10 20:07 UTC (permalink / raw)
  To: Regina Obe <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

"Regina Obe" <[email protected]> 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






^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-10 20:38  Regina Obe <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 0 replies; 14+ messages in thread

From: Regina Obe @ 2023-03-10 20:38 UTC (permalink / raw)
  To: 'Tom Lane' <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>



> -----Original Message-----
> From: Tom Lane [mailto:[email protected]]
> Sent: Friday, March 10, 2023 3:07 PM
> To: Regina Obe <[email protected]>
> Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli'
> <[email protected]>; [email protected]; 'Regina Obe'
> <[email protected]>
> Subject: Re: Ability to reference other extensions by schema in extension
> scripts
> 
> "Regina Obe" <[email protected]> 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







^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-10 21:05  Regina Obe <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 1 reply; 14+ messages in thread

From: Regina Obe @ 2023-03-10 21:05 UTC (permalink / raw)
  To: 'Tom Lane' <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

> "Regina Obe" <[email protected]> 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?








^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* Re: Ability to reference other extensions by schema in extension scripts
@ 2023-03-10 22:14  Tom Lane <[email protected]>
  parent: Regina Obe <[email protected]>
  0 siblings, 1 reply; 14+ messages in thread

From: Tom Lane @ 2023-03-10 22:14 UTC (permalink / raw)
  To: Regina Obe <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

"Regina Obe" <[email protected]> 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






^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-10 22:35  Regina Obe <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 14+ messages in thread

From: Regina Obe @ 2023-03-10 22:35 UTC (permalink / raw)
  To: 'Tom Lane' <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

> 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







^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* Re: Ability to reference other extensions by schema in extension scripts
@ 2023-03-10 22:47  Tom Lane <[email protected]>
  parent: Regina Obe <[email protected]>
  0 siblings, 1 reply; 14+ messages in thread

From: Tom Lane @ 2023-03-10 22:47 UTC (permalink / raw)
  To: Regina Obe <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

"Regina Obe" <[email protected]> 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






^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-11 08:18  Regina Obe <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 14+ messages in thread

From: Regina Obe @ 2023-03-11 08:18 UTC (permalink / raw)
  To: 'Tom Lane' <[email protected]>; +Cc: 'Gregory Stark (as CFM)' <[email protected]>; 'Sandro Santilli' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

> Subject: Re: Ability to reference other extensions by schema in extension
> scripts
> 
> "Regina Obe" <[email protected]> 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.


Attachments:

  [application/octet-stream] 0006-Allow-use-of-extschema-reqextname-to-reference.patch (20.9K, ../../[email protected]/2-0006-Allow-use-of-extschema-reqextname-to-reference.patch)
  download | inline diff:
From 32430fcec8794a0c10305bde45637d13828f0559 Mon Sep 17 00:00:00 2001
From: Regina Obe <[email protected]>
Date: Sun, 5 Feb 2023 01:35:29 -0500
Subject: [PATCH 6/6] Allow @extschema:<extname>@ in extension scripts

This feature allows an extension author to reference
extension schemas of extensions in the requires variable
of the control file using variable syntax
@extschema:<extname>@  where <extname> is the
name of any of the listed required extensions.

It also defines another optional config parameter
in the control file called no_relocate
which consists of a subset of the requires extensions
that should have ALTER EXTENSION SET SCHEMA prevented.

This feature is needed particularly in cases
when another extension packages a function
which references an object from one of its
required extensions and this function is used to back
indexes, constraints, and materialized views.

Without this feature, because pg_restore removes the paths,
these indexes, constraints, materialized views are not restored
because when the function is called, the function call fails
because items it depends on cannot be schema qualified.

This feature will allow extension authors to schema qualify
references to required extension objects to prevent this issue.
Aside from this issue, schema qualifying all these references will
improve security by preventing a rogue version of a function from being
used.

 - Extension tests both Makefile and meson.build
 - Documentation of the new feature
 - Define a new extension control parameter no_relocate
   Which is a comma separated list of required extensions
   that should be prevented from ALTER .. SET SCHEMA
 - Prevent an extension from being relocated if
   another extension requires it and has it
   listed in it's no_relocate list
 - Change execute_extension_script to take as input
   required extension oids,
   since calling functions already build this list

 Discussion: https://postgr.es/m/000801d94959%2463a9f520%242afddf60%24%40pcorp.us
---
 doc/src/sgml/extend.sgml                      | 35 +++++++
 src/backend/commands/extension.c              | 98 ++++++++++++++++++-
 src/test/modules/test_extensions/Makefile     |  9 +-
 .../expected/test_extensions.out              | 41 ++++++++
 src/test/modules/test_extensions/meson.build  |  7 ++
 .../test_extensions/sql/test_extensions.sql   | 15 +++
 .../test_ext_req_schema1--1.0.sql             |  6 ++
 .../test_ext_req_schema1.control              |  3 +
 .../test_ext_req_schema2--1.0--2.0.sql        |  7 ++
 .../test_ext_req_schema2--1.0.sql             |  9 ++
 .../test_ext_req_schema2.control              |  4 +
 .../test_ext_req_schema3--1.0.sql             | 13 +++
 .../test_ext_req_schema3.control              |  5 +
 13 files changed, 245 insertions(+), 7 deletions(-)
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema1.control
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2.control
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema3.control

diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index b70cbe83ae..da54c7feca 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -739,6 +739,22 @@ RETURNS anycompatible AS ...
       </listitem>
      </varlistentry>
 
+     <varlistentry id="extend-extensions-files-no-relocate">
+      <term><varname>no_relocate</varname> (<type>string</type>)</term>
+      <listitem>
+       <para>
+        A list of names of extensions that this extension depends on,
+        and that should be barred from SET SCHEMA.
+        This is needed if an extension script references
+        a required extensions schema by variable name
+        and the object references are in places that can't track renames.
+        for example <literal>no_relocate = 'foo, bar'</literal>.
+        This setting is checked at ALTER EXTENSION .. SET SCHEMA 
+        time of a required extension.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="extend-extensions-files-superuser">
       <term><varname>superuser</varname> (<type>boolean</type>)</term>
       <listitem>
@@ -908,6 +924,25 @@ RETURNS anycompatible AS ...
       </para>
      </listitem>
 
+     <listitem>
+      <para>
+       An extension might depend on other extensions.
+       It is useful to schema qualify calls to dependent extension to minimize reliance on search_path.
+       This is critical for cases such as functions used in indexes, materialized views, or check constraints.
+       To reference a required extension's schema, you must first have <literal>requires</literal>
+       variable specifying the list of extensions your extension requires.
+       In your extension sql scripts,
+       you can reference a required extension's schema with syntax
+       <literal>@extschema:reqextname@</literal> where <literal>reqextname</literal>
+       is the name of an extension in your <literal>requires</literal> list.
+       All occurrences of this string will be
+       replaced by the schema the required extension is installed in before the script is
+       executed.
+      </para>
+      <para>For extensions that you reference by this syntax, make sure to add them to the
+       <literal>no_relocate</literal> list as well.</para>
+     </listitem>
+
      <listitem>
       <para>
        If the extension does not support relocation at all, set
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 02ff4a9a7f..71683c547c 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -90,6 +90,8 @@ typedef struct ExtensionControlFile
 	bool		trusted;		/* allow becoming superuser on the fly? */
 	int			encoding;		/* encoding of the script file, or -1 */
 	List	   *requires;		/* names of prerequisite extensions */
+	List	   *no_relocate;	/* names of extensions that should be marked as not relocatable
+									these should be a subset of the requires */
 } ExtensionControlFile;
 
 /*
@@ -606,6 +608,21 @@ parse_extension_control_file(ExtensionControlFile *control,
 								item->name)));
 			}
 		}
+		else if (strcmp(item->name, "no_relocate") == 0)
+		{
+			/* Need a modifiable copy of string */
+			char	   *rawnames = pstrdup(item->value);
+
+			/* Parse string into list of identifiers */
+			if (!SplitIdentifierString(rawnames, ',', &control->no_relocate))
+			{
+				/* syntax error in name list */
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("parameter \"%s\" must be a list of extension names",
+								item->name)));
+			}
+		}
 		else
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -851,7 +868,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 						 const char *from_version,
 						 const char *version,
 						 List *requiredSchemas,
-						 const char *schemaName, Oid schemaOid)
+						 const char *schemaName, Oid schemaOid, List *requiredExtensions)
 {
 	bool		switch_to_superuser = false;
 	char	   *filename;
@@ -1030,6 +1047,34 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 											CStringGetTextDatum(qSchemaName));
 		}
 
+		/*
+		* If this extension requires other extensions
+		* Check each required extension to see if its schema
+		* is referenced by @extschema:reqextname@ syntax
+		*/
+		foreach(lc, requiredExtensions)
+		{
+			Oid				reqext = lfirst_oid(lc);
+			Oid				reqschema;
+			StringInfoData	rToken;
+			char			*reqname;
+			reqschema = get_extension_schema(reqext);
+			reqname = get_namespace_name(reqschema);
+			initStringInfo(&rToken);
+			appendStringInfo(&rToken, "%s%s%s", "@extschema:", get_extension_name(reqext), "@");
+
+			/*
+			* If the required extension's schema is referenced
+			* by variable name,
+			* Replace each occurence of @extschema:<reqextname>@
+			* with the required extension's schema
+			*/
+			t_sql = DirectFunctionCall3Coll(replace_text,
+								C_COLLATION_OID,
+								t_sql,
+								CStringGetTextDatum(rToken.data),
+								CStringGetTextDatum(quote_identifier(reqname)));
+		}
 		/*
 		 * If module_pathname was set in the control file, substitute its
 		 * value for occurrences of MODULE_PATHNAME.
@@ -1613,7 +1658,7 @@ CreateExtensionInternal(char *extensionName,
 	execute_extension_script(extensionOid, control,
 							 NULL, versionName,
 							 requiredSchemas,
-							 schemaName, schemaOid);
+							 schemaName, schemaOid, requiredExtensions);
 
 	/*
 	 * If additional update scripts have to be executed, apply the updates as
@@ -2094,8 +2139,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 	{
 		ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
 		ExtensionControlFile *control;
-		Datum		values[8];
-		bool		nulls[8];
+		Datum		values[9];
+		bool		nulls[9];
 		ListCell   *lc2;
 
 		if (!evi->installable)
@@ -2120,6 +2165,7 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 		values[3] = BoolGetDatum(control->trusted);
 		/* relocatable */
 		values[4] = BoolGetDatum(control->relocatable);
+
 		/* schema */
 		if (control->schema == NULL)
 			nulls[5] = true;
@@ -2131,12 +2177,19 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 			nulls[6] = true;
 		else
 			values[6] = convert_requires_to_datum(control->requires);
+
 		/* comment */
 		if (control->comment == NULL)
 			nulls[7] = true;
 		else
 			values[7] = CStringGetTextDatum(control->comment);
 
+		/* no_relocate */
+		if (control->no_relocate == NIL)
+			nulls[8] = true;
+		else
+			values[8] = convert_requires_to_datum(control->no_relocate);
+
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 
 		/*
@@ -2177,6 +2230,14 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 					nulls[6] = false;
 				}
 				/* comment stays the same */
+				/* no_relocate */
+				if (control->no_relocate == NIL)
+					nulls[8] = true;
+				else
+				{
+					values[8] = convert_requires_to_datum(control->no_relocate);
+					nulls[8] = false;
+				}
 
 				tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 			}
@@ -2816,6 +2877,33 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 		ObjectAddress dep;
 		Oid			dep_oldNspOid;
 
+		/* If an extension requires this extension
+		 * and the extension has this extension marked as no_relocate,
+		   then prevent set schema */
+		if (pg_depend->deptype == DEPENDENCY_NORMAL && pg_depend->classid == ExtensionRelationId){
+			ExtensionControlFile *dcontrol;
+			dep.classId = pg_depend->classid;
+			dep.objectId = pg_depend->objid;
+			dep.objectSubId = pg_depend->objsubid;
+			/** Check if dependent extension has a no_relocate request for this extension **/
+			dcontrol = read_extension_control_file(get_extension_name(dep.objectId));
+			if (dcontrol->no_relocate) {
+				ListCell   *lc;
+				foreach(lc, dcontrol->no_relocate)
+				{
+					char	*curreq = (char *) lfirst(lc);
+					if (strcmp(curreq, NameStr(extForm->extname)) == 0 ){
+						/** This dependent extension, has a no_relocate hold for this extension **/
+						ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							errmsg("cannot SET SCHEMA of extension %s because other extensions prevent it",
+									NameStr(extForm->extname)),
+							errdetail("%s prevents relocation of extension %s",
+									getObjectDescription(&dep, false), NameStr(extForm->extname))));
+					}
+				}
+			}
+		}
 		/*
 		 * Ignore non-membership dependencies.  (Currently, the only other
 		 * case we could see here is a normal dependency from another
@@ -3172,7 +3260,7 @@ ApplyExtensionUpdates(Oid extensionOid,
 		execute_extension_script(extensionOid, control,
 								 oldVersionName, versionName,
 								 requiredSchemas,
-								 schemaName, schemaOid);
+								 schemaName, schemaOid, requiredExtensions);
 
 		/*
 		 * Update prior-version name and loop around.  Since
diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile
index c3139ab0fc..c073df963c 100644
--- a/src/test/modules/test_extensions/Makefile
+++ b/src/test/modules/test_extensions/Makefile
@@ -6,14 +6,19 @@ PGFILEDESC = "test_extensions - regression testing for EXTENSION support"
 EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \
             test_ext7 test_ext8 test_ext_cine test_ext_cor \
             test_ext_cyclic1 test_ext_cyclic2 \
-            test_ext_evttrig
+            test_ext_evttrig \
+            test_ext_req_schema1 test_ext_req_schema2 test_ext_req_schema3
+
 DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \
        test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \
        test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \
        test_ext_cine--1.0.sql test_ext_cine--1.0--1.1.sql \
        test_ext_cor--1.0.sql \
        test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql \
-       test_ext_evttrig--1.0.sql test_ext_evttrig--1.0--2.0.sql
+       test_ext_evttrig--1.0.sql test_ext_evttrig--1.0--2.0.sql \
+       test_ext_req_schema1--1.0.sql \
+       test_ext_req_schema2--1.0.sql test_ext_req_schema2--1.0--2.0.sql \
+       test_ext_req_schema3--1.0.sql
 
 REGRESS = test_extensions test_extdepend
 
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out
index 821fed38d1..7582d2d2aa 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -312,3 +312,44 @@ Objects in extension "test_ext_cine"
  table ext_cine_tab3
 (9 rows)
 
+CREATE SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema3 CASCADE;
+NOTICE:  installing required extension "test_ext_req_schema2"
+SELECT dep_req();
+ dep_req 
+---------
+ 1032w
+(1 row)
+
+SELECT dep_req2();
+ dep_req2 
+----------
+ 1032w
+(1 row)
+
+SELECT dep_req3();
+ dep_req3 
+----------
+ 2032w
+(1 row)
+
+ALTER EXTENSION test_ext_req_schema2 UPDATE TO '2.0';
+CREATE SCHEMA test_s_dep2;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_s_dep2;
+ERROR:  cannot SET SCHEMA of extension test_ext_req_schema1 because other extensions prevent it
+DETAIL:  extension test_ext_req_schema3 prevents relocation of extension test_ext_req_schema1
+SELECT dep_req();
+ dep_req 
+---------
+ 1update
+(1 row)
+
+ALTER EXTENSION test_ext_req_schema2 SET SCHEMA test_s_dep;
+DROP EXTENSION test_ext_req_schema1;
+ERROR:  cannot drop extension test_ext_req_schema1 because other objects depend on it
+DETAIL:  extension test_ext_req_schema2 depends on extension test_ext_req_schema1
+extension test_ext_req_schema3 depends on extension test_ext_req_schema1
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+DROP EXTENSION test_ext_req_schema3;
+DROP EXTENSION test_ext_req_schema2;
diff --git a/src/test/modules/test_extensions/meson.build b/src/test/modules/test_extensions/meson.build
index c3af3e1721..7f47a6fc23 100644
--- a/src/test/modules/test_extensions/meson.build
+++ b/src/test/modules/test_extensions/meson.build
@@ -30,6 +30,13 @@ test_install_data += files(
   'test_ext_evttrig--1.0--2.0.sql',
   'test_ext_evttrig--1.0.sql',
   'test_ext_evttrig.control',
+  'test_ext_req_schema1--1.0.sql',
+  'test_ext_req_schema1.control',
+  'test_ext_req_schema2--1.0.sql',
+  'test_ext_req_schema2.control',
+  'test_ext_req_schema2--1.0--2.0.sql',
+  'test_ext_req_schema3.control',
+  'test_ext_req_schema3--1.0.sql',
 )
 
 tests += {
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index 41b6cddf0b..0f554c856b 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -209,3 +209,18 @@ CREATE EXTENSION test_ext_cine;
 ALTER EXTENSION test_ext_cine UPDATE TO '1.1';
 
 \dx+ test_ext_cine
+
+CREATE SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema3 CASCADE;
+SELECT dep_req();
+SELECT dep_req2();
+SELECT dep_req3();
+ALTER EXTENSION test_ext_req_schema2 UPDATE TO '2.0';
+CREATE SCHEMA test_s_dep2;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_s_dep2;
+SELECT dep_req();
+ALTER EXTENSION test_ext_req_schema2 SET SCHEMA test_s_dep;
+DROP EXTENSION test_ext_req_schema1;
+DROP EXTENSION test_ext_req_schema3;
+DROP EXTENSION test_ext_req_schema2;
\ No newline at end of file
diff --git a/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
new file mode 100644
index 0000000000..462fb52145
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
@@ -0,0 +1,6 @@
+/* src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema1" to load this file. \quit
+
+CREATE DOMAIN req AS text
+  CONSTRAINT starts_with_1 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '1');
diff --git a/src/test/modules/test_extensions/test_ext_req_schema1.control b/src/test/modules/test_extensions/test_ext_req_schema1.control
new file mode 100644
index 0000000000..9ea4558a90
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema1.control
@@ -0,0 +1,3 @@
+comment = 'Create required extension to be referenced'
+default_version = '1.0'
+relocatable = true
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql b/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
new file mode 100644
index 0000000000..73a44a25e5
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
@@ -0,0 +1,7 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+
+CREATE OR REPLACE FUNCTION dep_req() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1update''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
new file mode 100644
index 0000000000..b17fb82535
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
@@ -0,0 +1,9 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+CREATE DOMAIN dreq AS text
+  CONSTRAINT starts_with_2 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '2');
+
+CREATE FUNCTION dep_req() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1032w''';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2.control b/src/test/modules/test_extensions/test_ext_req_schema2.control
new file mode 100644
index 0000000000..d2ba5add97
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2.control
@@ -0,0 +1,4 @@
+comment = 'Test schema referencing of required extensions'
+default_version = '1.0'
+relocatable = true
+requires = 'test_ext_req_schema1'
diff --git a/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
new file mode 100644
index 0000000000..bd05669097
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
@@ -0,0 +1,13 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+CREATE DOMAIN req2 AS text
+  CONSTRAINT starts_with_2 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '2');
+
+CREATE FUNCTION dep_req2() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1032w''::@extschema:[email protected]';
+
+CREATE FUNCTION dep_req3() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''2032w''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema3.control b/src/test/modules/test_extensions/test_ext_req_schema3.control
new file mode 100644
index 0000000000..66c8de5e05
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema3.control
@@ -0,0 +1,5 @@
+comment = 'Test schema referencing of 2 required extensions'
+default_version = '1.0'
+relocatable = true
+requires = 'test_ext_req_schema1,test_ext_req_schema2'
+no_relocate = 'test_ext_req_schema1'
-- 
2.21.0.windows.1



^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* Re: Ability to reference other extensions by schema in extension scripts
@ 2023-03-13 11:59  'Sandro Santilli' <[email protected]>
  parent: Regina Obe <[email protected]>
  0 siblings, 2 replies; 14+ messages in thread

From: 'Sandro Santilli' @ 2023-03-13 11:59 UTC (permalink / raw)
  To: Regina Obe <[email protected]>; +Cc: 'Tom Lane' <[email protected]>; 'Gregory Stark (as CFM)' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

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






^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-13 14:28  Regina Obe <[email protected]>
  parent: 'Sandro Santilli' <[email protected]>
  1 sibling, 0 replies; 14+ messages in thread

From: Regina Obe @ 2023-03-13 14:28 UTC (permalink / raw)
  To: [email protected]; +Cc: 'Tom Lane' <[email protected]>; 'Gregory Stark (as CFM)' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

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







^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* RE: Ability to reference other extensions by schema in extension scripts
@ 2023-03-13 21:57  Regina Obe <[email protected]>
  parent: 'Sandro Santilli' <[email protected]>
  1 sibling, 1 reply; 14+ messages in thread

From: Regina Obe @ 2023-03-13 21:57 UTC (permalink / raw)
  To: [email protected]; +Cc: 'Tom Lane' <[email protected]>; 'Gregory Stark (as CFM)' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

> 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


Attachments:

  [application/octet-stream] 0007-Allow-use-of-extschema-reqextname-to-reference.patch (20.9K, ../../[email protected]/2-0007-Allow-use-of-extschema-reqextname-to-reference.patch)
  download | inline diff:
From d7b3ee51af0fbdc7e58fc43473114cd633e09c74 Mon Sep 17 00:00:00 2001
From: Regina Obe <[email protected]>
Date: Sun, 5 Feb 2023 01:35:29 -0500
Subject: [PATCH 7/7] Allow @extschema:<extname>@ in extension scripts

This feature allows an extension author to reference
extension schemas of extensions in the requires variable
of the control file using variable syntax
@extschema:<extname>@  where <extname> is the
name of any of the listed required extensions.

It also defines another optional config parameter
in the control file called no_relocate
which consists of a subset of the requires extensions
that should have ALTER EXTENSION SET SCHEMA prevented.

This feature is needed particularly in cases
when another extension packages a function
which references an object from one of its
required extensions and this function is used to back
indexes, constraints, and materialized views.

Without this feature, because pg_restore removes the paths,
these indexes, constraints, materialized views are not restored
because when the function is called, the function call fails
because items it depends on cannot be schema qualified.

This feature will allow extension authors to schema qualify
references to required extension objects to prevent this issue.
Aside from this issue, schema qualifying all these references will
improve security by preventing a rogue version of a function from being
used.

 - Extension tests both Makefile and meson.build
 - Documentation of the new feature
 - Define a new extension control parameter no_relocate
   Which is a comma separated list of required extensions
   that should be prevented from ALTER .. SET SCHEMA
 - Prevent an extension from being relocated if
   another extension requires it and has it
   listed in it's no_relocate list
 - Change execute_extension_script to take as input
   required extension oids,
   since calling functions already build this list

 Discussion: https://postgr.es/m/000801d94959%2463a9f520%242afddf60%24%40pcorp.us
---
 doc/src/sgml/extend.sgml                      | 35 +++++++
 src/backend/commands/extension.c              | 98 ++++++++++++++++++-
 src/test/modules/test_extensions/Makefile     |  9 +-
 .../expected/test_extensions.out              | 41 ++++++++
 src/test/modules/test_extensions/meson.build  |  7 ++
 .../test_extensions/sql/test_extensions.sql   | 15 +++
 .../test_ext_req_schema1--1.0.sql             |  6 ++
 .../test_ext_req_schema1.control              |  3 +
 .../test_ext_req_schema2--1.0--2.0.sql        |  7 ++
 .../test_ext_req_schema2--1.0.sql             |  9 ++
 .../test_ext_req_schema2.control              |  4 +
 .../test_ext_req_schema3--1.0.sql             | 13 +++
 .../test_ext_req_schema3.control              |  5 +
 13 files changed, 245 insertions(+), 7 deletions(-)
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema1.control
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema2.control
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_req_schema3.control

diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index b70cbe83ae..554876672b 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -739,6 +739,22 @@ RETURNS anycompatible AS ...
       </listitem>
      </varlistentry>
 
+     <varlistentry id="extend-extensions-files-no-relocate">
+      <term><varname>no_relocate</varname> (<type>string</type>)</term>
+      <listitem>
+       <para>
+        A list of names of extensions that this extension depends on,
+        and that should be barred from SET SCHEMA.
+        This is needed if an extension script references
+        a required extensions schema by variable name
+        and the object references are in places that can't track renames.
+        for example <literal>no_relocate = 'foo, bar'</literal>.
+        This setting is checked at ALTER EXTENSION .. SET SCHEMA
+        time of a required extension.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="extend-extensions-files-superuser">
       <term><varname>superuser</varname> (<type>boolean</type>)</term>
       <listitem>
@@ -908,6 +924,25 @@ RETURNS anycompatible AS ...
       </para>
      </listitem>
 
+     <listitem>
+      <para>
+       An extension might depend on other extensions.
+       It is useful to schema qualify calls to dependent extension to minimize reliance on search_path.
+       This is critical for cases such as functions used in indexes, materialized views, or check constraints.
+       To reference a required extension's schema, you must first have <literal>requires</literal>
+       variable specifying the list of extensions your extension requires.
+       In your extension sql scripts,
+       you can reference a required extension's schema with syntax
+       <literal>@extschema:reqextname@</literal> where <literal>reqextname</literal>
+       is the name of an extension in your <literal>requires</literal> list.
+       All occurrences of this string will be
+       replaced by the schema the required extension is installed in before the script is
+       executed.
+      </para>
+      <para>For extensions that you reference by this syntax, make sure to add them to the
+       <literal>no_relocate</literal> list as well.</para>
+     </listitem>
+
      <listitem>
       <para>
        If the extension does not support relocation at all, set
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 02ff4a9a7f..71683c547c 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -90,6 +90,8 @@ typedef struct ExtensionControlFile
 	bool		trusted;		/* allow becoming superuser on the fly? */
 	int			encoding;		/* encoding of the script file, or -1 */
 	List	   *requires;		/* names of prerequisite extensions */
+	List	   *no_relocate;	/* names of extensions that should be marked as not relocatable
+									these should be a subset of the requires */
 } ExtensionControlFile;
 
 /*
@@ -606,6 +608,21 @@ parse_extension_control_file(ExtensionControlFile *control,
 								item->name)));
 			}
 		}
+		else if (strcmp(item->name, "no_relocate") == 0)
+		{
+			/* Need a modifiable copy of string */
+			char	   *rawnames = pstrdup(item->value);
+
+			/* Parse string into list of identifiers */
+			if (!SplitIdentifierString(rawnames, ',', &control->no_relocate))
+			{
+				/* syntax error in name list */
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("parameter \"%s\" must be a list of extension names",
+								item->name)));
+			}
+		}
 		else
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -851,7 +868,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 						 const char *from_version,
 						 const char *version,
 						 List *requiredSchemas,
-						 const char *schemaName, Oid schemaOid)
+						 const char *schemaName, Oid schemaOid, List *requiredExtensions)
 {
 	bool		switch_to_superuser = false;
 	char	   *filename;
@@ -1030,6 +1047,34 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 											CStringGetTextDatum(qSchemaName));
 		}
 
+		/*
+		* If this extension requires other extensions
+		* Check each required extension to see if its schema
+		* is referenced by @extschema:reqextname@ syntax
+		*/
+		foreach(lc, requiredExtensions)
+		{
+			Oid				reqext = lfirst_oid(lc);
+			Oid				reqschema;
+			StringInfoData	rToken;
+			char			*reqname;
+			reqschema = get_extension_schema(reqext);
+			reqname = get_namespace_name(reqschema);
+			initStringInfo(&rToken);
+			appendStringInfo(&rToken, "%s%s%s", "@extschema:", get_extension_name(reqext), "@");
+
+			/*
+			* If the required extension's schema is referenced
+			* by variable name,
+			* Replace each occurence of @extschema:<reqextname>@
+			* with the required extension's schema
+			*/
+			t_sql = DirectFunctionCall3Coll(replace_text,
+								C_COLLATION_OID,
+								t_sql,
+								CStringGetTextDatum(rToken.data),
+								CStringGetTextDatum(quote_identifier(reqname)));
+		}
 		/*
 		 * If module_pathname was set in the control file, substitute its
 		 * value for occurrences of MODULE_PATHNAME.
@@ -1613,7 +1658,7 @@ CreateExtensionInternal(char *extensionName,
 	execute_extension_script(extensionOid, control,
 							 NULL, versionName,
 							 requiredSchemas,
-							 schemaName, schemaOid);
+							 schemaName, schemaOid, requiredExtensions);
 
 	/*
 	 * If additional update scripts have to be executed, apply the updates as
@@ -2094,8 +2139,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 	{
 		ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
 		ExtensionControlFile *control;
-		Datum		values[8];
-		bool		nulls[8];
+		Datum		values[9];
+		bool		nulls[9];
 		ListCell   *lc2;
 
 		if (!evi->installable)
@@ -2120,6 +2165,7 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 		values[3] = BoolGetDatum(control->trusted);
 		/* relocatable */
 		values[4] = BoolGetDatum(control->relocatable);
+
 		/* schema */
 		if (control->schema == NULL)
 			nulls[5] = true;
@@ -2131,12 +2177,19 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 			nulls[6] = true;
 		else
 			values[6] = convert_requires_to_datum(control->requires);
+
 		/* comment */
 		if (control->comment == NULL)
 			nulls[7] = true;
 		else
 			values[7] = CStringGetTextDatum(control->comment);
 
+		/* no_relocate */
+		if (control->no_relocate == NIL)
+			nulls[8] = true;
+		else
+			values[8] = convert_requires_to_datum(control->no_relocate);
+
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 
 		/*
@@ -2177,6 +2230,14 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
 					nulls[6] = false;
 				}
 				/* comment stays the same */
+				/* no_relocate */
+				if (control->no_relocate == NIL)
+					nulls[8] = true;
+				else
+				{
+					values[8] = convert_requires_to_datum(control->no_relocate);
+					nulls[8] = false;
+				}
 
 				tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 			}
@@ -2816,6 +2877,33 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
 		ObjectAddress dep;
 		Oid			dep_oldNspOid;
 
+		/* If an extension requires this extension
+		 * and the extension has this extension marked as no_relocate,
+		   then prevent set schema */
+		if (pg_depend->deptype == DEPENDENCY_NORMAL && pg_depend->classid == ExtensionRelationId){
+			ExtensionControlFile *dcontrol;
+			dep.classId = pg_depend->classid;
+			dep.objectId = pg_depend->objid;
+			dep.objectSubId = pg_depend->objsubid;
+			/** Check if dependent extension has a no_relocate request for this extension **/
+			dcontrol = read_extension_control_file(get_extension_name(dep.objectId));
+			if (dcontrol->no_relocate) {
+				ListCell   *lc;
+				foreach(lc, dcontrol->no_relocate)
+				{
+					char	*curreq = (char *) lfirst(lc);
+					if (strcmp(curreq, NameStr(extForm->extname)) == 0 ){
+						/** This dependent extension, has a no_relocate hold for this extension **/
+						ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							errmsg("cannot SET SCHEMA of extension %s because other extensions prevent it",
+									NameStr(extForm->extname)),
+							errdetail("%s prevents relocation of extension %s",
+									getObjectDescription(&dep, false), NameStr(extForm->extname))));
+					}
+				}
+			}
+		}
 		/*
 		 * Ignore non-membership dependencies.  (Currently, the only other
 		 * case we could see here is a normal dependency from another
@@ -3172,7 +3260,7 @@ ApplyExtensionUpdates(Oid extensionOid,
 		execute_extension_script(extensionOid, control,
 								 oldVersionName, versionName,
 								 requiredSchemas,
-								 schemaName, schemaOid);
+								 schemaName, schemaOid, requiredExtensions);
 
 		/*
 		 * Update prior-version name and loop around.  Since
diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile
index c3139ab0fc..c073df963c 100644
--- a/src/test/modules/test_extensions/Makefile
+++ b/src/test/modules/test_extensions/Makefile
@@ -6,14 +6,19 @@ PGFILEDESC = "test_extensions - regression testing for EXTENSION support"
 EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \
             test_ext7 test_ext8 test_ext_cine test_ext_cor \
             test_ext_cyclic1 test_ext_cyclic2 \
-            test_ext_evttrig
+            test_ext_evttrig \
+            test_ext_req_schema1 test_ext_req_schema2 test_ext_req_schema3
+
 DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \
        test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \
        test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \
        test_ext_cine--1.0.sql test_ext_cine--1.0--1.1.sql \
        test_ext_cor--1.0.sql \
        test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql \
-       test_ext_evttrig--1.0.sql test_ext_evttrig--1.0--2.0.sql
+       test_ext_evttrig--1.0.sql test_ext_evttrig--1.0--2.0.sql \
+       test_ext_req_schema1--1.0.sql \
+       test_ext_req_schema2--1.0.sql test_ext_req_schema2--1.0--2.0.sql \
+       test_ext_req_schema3--1.0.sql
 
 REGRESS = test_extensions test_extdepend
 
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out
index 821fed38d1..7582d2d2aa 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -312,3 +312,44 @@ Objects in extension "test_ext_cine"
  table ext_cine_tab3
 (9 rows)
 
+CREATE SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema3 CASCADE;
+NOTICE:  installing required extension "test_ext_req_schema2"
+SELECT dep_req();
+ dep_req 
+---------
+ 1032w
+(1 row)
+
+SELECT dep_req2();
+ dep_req2 
+----------
+ 1032w
+(1 row)
+
+SELECT dep_req3();
+ dep_req3 
+----------
+ 2032w
+(1 row)
+
+ALTER EXTENSION test_ext_req_schema2 UPDATE TO '2.0';
+CREATE SCHEMA test_s_dep2;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_s_dep2;
+ERROR:  cannot SET SCHEMA of extension test_ext_req_schema1 because other extensions prevent it
+DETAIL:  extension test_ext_req_schema3 prevents relocation of extension test_ext_req_schema1
+SELECT dep_req();
+ dep_req 
+---------
+ 1update
+(1 row)
+
+ALTER EXTENSION test_ext_req_schema2 SET SCHEMA test_s_dep;
+DROP EXTENSION test_ext_req_schema1;
+ERROR:  cannot drop extension test_ext_req_schema1 because other objects depend on it
+DETAIL:  extension test_ext_req_schema2 depends on extension test_ext_req_schema1
+extension test_ext_req_schema3 depends on extension test_ext_req_schema1
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+DROP EXTENSION test_ext_req_schema3;
+DROP EXTENSION test_ext_req_schema2;
diff --git a/src/test/modules/test_extensions/meson.build b/src/test/modules/test_extensions/meson.build
index c3af3e1721..7f47a6fc23 100644
--- a/src/test/modules/test_extensions/meson.build
+++ b/src/test/modules/test_extensions/meson.build
@@ -30,6 +30,13 @@ test_install_data += files(
   'test_ext_evttrig--1.0--2.0.sql',
   'test_ext_evttrig--1.0.sql',
   'test_ext_evttrig.control',
+  'test_ext_req_schema1--1.0.sql',
+  'test_ext_req_schema1.control',
+  'test_ext_req_schema2--1.0.sql',
+  'test_ext_req_schema2.control',
+  'test_ext_req_schema2--1.0--2.0.sql',
+  'test_ext_req_schema3.control',
+  'test_ext_req_schema3--1.0.sql',
 )
 
 tests += {
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index 41b6cddf0b..0f554c856b 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -209,3 +209,18 @@ CREATE EXTENSION test_ext_cine;
 ALTER EXTENSION test_ext_cine UPDATE TO '1.1';
 
 \dx+ test_ext_cine
+
+CREATE SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_s_dep;
+CREATE EXTENSION test_ext_req_schema3 CASCADE;
+SELECT dep_req();
+SELECT dep_req2();
+SELECT dep_req3();
+ALTER EXTENSION test_ext_req_schema2 UPDATE TO '2.0';
+CREATE SCHEMA test_s_dep2;
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_s_dep2;
+SELECT dep_req();
+ALTER EXTENSION test_ext_req_schema2 SET SCHEMA test_s_dep;
+DROP EXTENSION test_ext_req_schema1;
+DROP EXTENSION test_ext_req_schema3;
+DROP EXTENSION test_ext_req_schema2;
\ No newline at end of file
diff --git a/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
new file mode 100644
index 0000000000..462fb52145
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql
@@ -0,0 +1,6 @@
+/* src/test/modules/test_extensions/test_ext_req_schema1--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema1" to load this file. \quit
+
+CREATE DOMAIN req AS text
+  CONSTRAINT starts_with_1 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '1');
diff --git a/src/test/modules/test_extensions/test_ext_req_schema1.control b/src/test/modules/test_extensions/test_ext_req_schema1.control
new file mode 100644
index 0000000000..9ea4558a90
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema1.control
@@ -0,0 +1,3 @@
+comment = 'Create required extension to be referenced'
+default_version = '1.0'
+relocatable = true
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql b/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
new file mode 100644
index 0000000000..73a44a25e5
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2--1.0--2.0.sql
@@ -0,0 +1,7 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+
+CREATE OR REPLACE FUNCTION dep_req() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1update''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
new file mode 100644
index 0000000000..b17fb82535
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql
@@ -0,0 +1,9 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+CREATE DOMAIN dreq AS text
+  CONSTRAINT starts_with_2 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '2');
+
+CREATE FUNCTION dep_req() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1032w''';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema2.control b/src/test/modules/test_extensions/test_ext_req_schema2.control
new file mode 100644
index 0000000000..d2ba5add97
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema2.control
@@ -0,0 +1,4 @@
+comment = 'Test schema referencing of required extensions'
+default_version = '1.0'
+relocatable = true
+requires = 'test_ext_req_schema1'
diff --git a/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql b/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
new file mode 100644
index 0000000000..bd05669097
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema3--1.0.sql
@@ -0,0 +1,13 @@
+/* src/test/modules/test_extensions/test_ext_req_schema2--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_ext_req_schema2" to load this file. \quit
+CREATE DOMAIN req2 AS text
+  CONSTRAINT starts_with_2 check(pg_catalog.left(value,1) OPERATOR(pg_catalog.=) '2');
+
+CREATE FUNCTION dep_req2() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''1032w''::@extschema:[email protected]';
+
+CREATE FUNCTION dep_req3() RETURNS @extschema:[email protected]
+LANGUAGE SQL IMMUTABLE PARALLEL SAFE
+AS 'SELECT ''2032w''::@extschema:[email protected]';
diff --git a/src/test/modules/test_extensions/test_ext_req_schema3.control b/src/test/modules/test_extensions/test_ext_req_schema3.control
new file mode 100644
index 0000000000..66c8de5e05
--- /dev/null
+++ b/src/test/modules/test_extensions/test_ext_req_schema3.control
@@ -0,0 +1,5 @@
+comment = 'Test schema referencing of 2 required extensions'
+default_version = '1.0'
+relocatable = true
+requires = 'test_ext_req_schema1,test_ext_req_schema2'
+no_relocate = 'test_ext_req_schema1'
-- 
2.21.0.windows.1



^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* Re: Ability to reference other extensions by schema in extension scripts
@ 2023-03-16 10:14  Sandro Santilli <[email protected]>
  parent: Regina Obe <[email protected]>
  0 siblings, 1 reply; 14+ messages in thread

From: Sandro Santilli @ 2023-03-16 10:14 UTC (permalink / raw)
  To: Regina Obe <[email protected]>; +Cc: 'Tom Lane' <[email protected]>; 'Gregory Stark (as CFM)' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

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;







^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* Re: Ability to reference other extensions by schema in extension scripts
@ 2023-03-20 22:47  Tom Lane <[email protected]>
  parent: Sandro Santilli <[email protected]>
  0 siblings, 0 replies; 14+ messages in thread

From: Tom Lane @ 2023-03-20 22:47 UTC (permalink / raw)
  To: Sandro Santilli <[email protected]>; +Cc: Regina Obe <[email protected]>; 'Gregory Stark (as CFM)' <[email protected]>; [email protected]; 'Regina Obe' <[email protected]>

Sandro Santilli <[email protected]> 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






^ permalink  raw  reply  [nested|flat] 14+ messages in thread

* [PATCH v5 2/7] Row pattern recognition patch (parse/analysis).
@ 2023-09-02 06:32  Tatsuo Ishii <[email protected]>
  0 siblings, 0 replies; 14+ messages in thread

From: Tatsuo Ishii @ 2023-09-02 06:32 UTC (permalink / raw)

---
 src/backend/parser/parse_agg.c    |   7 +
 src/backend/parser/parse_clause.c | 292 +++++++++++++++++++++++++++++-
 src/backend/parser/parse_expr.c   |   4 +
 src/backend/parser/parse_func.c   |   3 +
 4 files changed, 305 insertions(+), 1 deletion(-)

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 85cd47b7ae..aa7a1cee80 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -564,6 +564,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
 			errkind = true;
 			break;
 
+		case EXPR_KIND_RPR_DEFINE:
+			errkind = true;
+			break;
+
 			/*
 			 * There is intentionally no default: case here, so that the
 			 * compiler will warn if we add a new ParseExprKind without
@@ -953,6 +957,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
 		case EXPR_KIND_CYCLE_MARK:
 			errkind = true;
 			break;
+		case EXPR_KIND_RPR_DEFINE:
+			errkind = true;
+			break;
 
 			/*
 			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 334b9b42bd..60020a7025 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
 static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
 								  Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
 								  Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
 
 /*
  * transformFromClause -
@@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate,
 											 rangeopfamily, rangeopcintype,
 											 &wc->endInRangeFunc,
 											 windef->endOffset);
+
+		/* Process Row Pattern Recognition related clauses */
+		transformRPR(pstate, wc, windef, targetlist);
+
 		wc->runCondition = NIL;
 		wc->winref = winref;
 
@@ -3815,3 +3822,286 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
 
 	return node;
 }
+
+/*
+ * transformRPR
+ *		Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+	/*
+	 * Window definition exists?
+	 */
+	if (windef == NULL)
+		return;
+
+	/*
+	 * Row Pattern Common Syntax clause exists?
+	 */
+	if (windef->rpCommonSyntax == NULL)
+		return;
+
+	/* Check Frame option. Frame must start at current row */
+	if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_SYNTAX_ERROR),
+				 errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+	/* Transform AFTER MACH SKIP TO clause */
+	wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+	/* Transform SEEK or INITIAL clause */
+	wc->initial = windef->rpCommonSyntax->initial;
+
+	/* Transform DEFINE clause into list of TargetEntry's */
+	wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+	/* Check PATTERN clause and copy to patternClause */
+	transformPatternClause(pstate, wc, windef);
+
+	/* Transform MEASURE clause */
+	transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause Process DEFINE clause and transform ResTarget into
+ *		list of TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+	/* DEFINE variable name initials */
+	static	char	*defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+	ListCell		*lc, *l;
+	ResTarget		*restarget, *r;
+	List			*restargets;
+	char			*name;
+	int				initialLen;
+	int				i;
+
+	/*
+	 * If Row Definition Common Syntax exists, DEFINE clause must exist.
+	 * (the raw parser should have already checked it.)
+	 */
+	Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+	/*
+	 * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+	 * per the SQL standard.
+	 */
+	restargets = NIL;
+	foreach(lc, windef->rpCommonSyntax->rpPatterns)
+	{
+		A_Expr	*a;
+		bool	found = false;
+
+		if (!IsA(lfirst(lc), A_Expr))
+			ereport(ERROR,
+					errmsg("node type is not A_Expr"));
+
+		a = (A_Expr *)lfirst(lc);
+		name = strVal(a->lexpr);
+
+		foreach(l, windef->rpCommonSyntax->rpDefs)
+		{
+			restarget = (ResTarget *)lfirst(l);
+
+			if (!strcmp(restarget->name, name))
+			{
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+		{
+			/*
+			 * "name" is missing. So create "name AS name IS TRUE" ResTarget
+			 * node and add it to the temporary list.
+			 */
+			A_Const	   *n;
+
+			restarget = makeNode(ResTarget);
+			n = makeNode(A_Const);
+			n->val.boolval.type = T_Boolean;
+			n->val.boolval.boolval = true;
+			n->location = -1;
+			restarget->name = pstrdup(name);
+			restarget->indirection = NIL;
+			restarget->val = (Node *)n;
+			restarget->location = -1;
+			restargets = lappend((List *)restargets, restarget);
+		}
+	}
+
+	if (list_length(restargets) >= 1)
+	{
+		/* add missing DEFINEs */
+		windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs,
+													 restargets);
+		list_free(restargets);
+	}
+
+	/*
+	 * Check for duplicate row pattern definition variables.  The standard
+	 * requires that no two row pattern definition variable names shall be
+	 * equivalent.
+	 */
+	restargets = NIL;
+	foreach(lc, windef->rpCommonSyntax->rpDefs)
+	{
+		restarget = (ResTarget *)lfirst(lc);
+		name = restarget->name;
+
+		/*
+		 * Add DEFINE expression (Restarget->val) to the targetlist as a
+		 * TargetEntry if it does not exist yet. Planner will add the column
+		 * ref var node to the outer plan's target list later on. This makes
+		 * DEFINE expression could access the outer tuple while evaluating
+		 * PATTERN.
+		 *
+		 * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+		 * not so good, because it's not necessary to evalute the expression
+		 * in the target list while running the plan. We should extract the
+		 * var nodes only then add them to the plan.targetlist.
+		 */
+		findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE);
+
+		/*
+		 * Make sure that the row pattern definition search condition is a
+		 * boolean expression.
+		 */
+		transformWhereClause(pstate, restarget->val,
+							 EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+		foreach(l, restargets)
+		{
+			char		*n;
+
+			r = (ResTarget *) lfirst(l);
+			n = r->name;
+
+			if (!strcmp(n, name))
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+								name),
+						 parser_errposition(pstate, exprLocation((Node *)r))));
+		}
+		restargets = lappend(restargets, restarget);
+	}
+	list_free(restargets);
+
+	/*
+	 * Create list of row pattern DEFINE variable name's initial.
+	 * We assign [a-z] to them (up to 26 variable names are allowed).
+	 */
+	restargets = NIL;
+	i = 0;
+	initialLen = strlen(defineVariableInitials);
+
+	foreach(lc, windef->rpCommonSyntax->rpDefs)
+	{
+		char	initial[2];
+
+		restarget = (ResTarget *)lfirst(lc);
+		name = restarget->name;
+
+		if (i >= initialLen)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("number of row pattern definition variable names exceeds %d", initialLen),
+					 parser_errposition(pstate, exprLocation((Node *)restarget))));
+		}
+		initial[0] = defineVariableInitials[i++];
+		initial[1] = '\0';
+		wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial)));
+	}
+
+	return transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+							   EXPR_KIND_RPR_DEFINE);
+}
+
+/*
+ * transformPatternClause
+ *		Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+	ListCell	*lc, *l;
+
+	/*
+	 * Row Pattern Common Syntax clause exists?
+	 */
+	if (windef->rpCommonSyntax == NULL)
+		return;
+
+	/*
+	 * Primary row pattern variable names in PATTERN clause must appear in
+	 * DEFINE clause as row pattern definition variable names.
+	 */
+	wc->patternVariable = NIL;
+	wc->patternRegexp = NIL;
+	foreach(lc, windef->rpCommonSyntax->rpPatterns)
+	{
+		A_Expr	*a;
+		char	*name;
+		char	*regexp;
+		bool	found = false;
+
+		if (!IsA(lfirst(lc), A_Expr))
+			ereport(ERROR,
+					errmsg("node type is not A_Expr"));
+
+		a = (A_Expr *)lfirst(lc);
+		name = strVal(a->lexpr);
+
+		foreach(l, windef->rpCommonSyntax->rpDefs)
+		{
+			ResTarget	*restarget = (ResTarget *)lfirst(l);
+
+			if (!strcmp(restarget->name, name))
+			{
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("primary row pattern variable name \"%s\" does not appear in DEFINE clause",
+							name),
+					 parser_errposition(pstate, exprLocation((Node *)a))));
+		}
+		wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+		regexp = strVal(lfirst(list_head(a->name)));
+		wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+	}
+}
+
+/*
+ * transformMeasureClause
+ *		Process MEASURE clause
+ *	XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+	if (windef->rowPatternMeasures == NIL)
+		return NIL;
+
+	ereport(ERROR,
+			(errcode(ERRCODE_SYNTAX_ERROR),
+			 errmsg("%s","MEASURE clause is not supported yet"),
+			 parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures))));
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 64c582c344..18b58ac263 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 		case EXPR_KIND_COPY_WHERE:
 		case EXPR_KIND_GENERATED_COLUMN:
 		case EXPR_KIND_CYCLE_MARK:
+		case EXPR_KIND_RPR_DEFINE:
 			/* okay */
 			break;
 
@@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
 		case EXPR_KIND_VALUES:
 		case EXPR_KIND_VALUES_SINGLE:
 		case EXPR_KIND_CYCLE_MARK:
+		case EXPR_KIND_RPR_DEFINE:
 			/* okay */
 			break;
 		case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind)
 			return "GENERATED AS";
 		case EXPR_KIND_CYCLE_MARK:
 			return "CYCLE";
+		case EXPR_KIND_RPR_DEFINE:
+			return "DEFINE";
 
 			/*
 			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index b3f0b6a137..2ff3699538 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
 		case EXPR_KIND_CYCLE_MARK:
 			errkind = true;
 			break;
+		case EXPR_KIND_RPR_DEFINE:
+			errkind = true;
+			break;
 
 			/*
 			 * There is intentionally no default: case here, so that the
-- 
2.25.1


----Next_Part(Sat_Sep__2_15_52_35_2023_273)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v5-0003-Row-pattern-recognition-patch-planner.patch"



^ permalink  raw  reply  [nested|flat] 14+ messages in thread

end of thread, other threads:[~2023-09-02 06:32 UTC | newest]

Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-06 23:26 RE: Ability to reference other extensions by schema in extension scripts Regina Obe <[email protected]>
2023-03-10 20:07 ` Tom Lane <[email protected]>
2023-03-10 20:38   ` Regina Obe <[email protected]>
2023-03-10 21:05   ` Regina Obe <[email protected]>
2023-03-10 22:14     ` Tom Lane <[email protected]>
2023-03-10 22:35       ` Regina Obe <[email protected]>
2023-03-10 22:47         ` Tom Lane <[email protected]>
2023-03-11 08:18           ` Regina Obe <[email protected]>
2023-03-13 11:59             ` 'Sandro Santilli' <[email protected]>
2023-03-13 14:28               ` Regina Obe <[email protected]>
2023-03-13 21:57               ` Regina Obe <[email protected]>
2023-03-16 10:14                 ` Sandro Santilli <[email protected]>
2023-03-20 22:47                   ` Tom Lane <[email protected]>
2023-09-02 06:32 [PATCH v5 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox