Subject: Re: EphemeralNamedRelation and materialized view

From: Yugo NAGATA <[email-masked]>
Date: Fri, 15 Nov 2024 17:36:47 +0900

On Sun, 03 Nov 2024 13:42:33 -0500
Tom Lane <[email-masked]> wrote:

> Yugo Nagata <[email-masked]> writes:
> > While looking into the commit b4da732fd64e936970f38c792f8b32c4bdf2bcd5,
> > I noticed that we can create a materialized view using Ephemeral Named
> > Relation in PostgreSQL 16 or earler. 
> 
> Yeah, we should reject that, but I feel like this patch is not
> ambitious enough, because the 17-and-up behavior isn't exactly
> polished either.
> 
> I tried variants of this function in HEAD:
> 
> 1. With "create table mv as select * from enr", it works and
> does what you'd expect.
> 
> 2. With "create view mv as select * from enr", you get
> 
> regression=# insert into tbl values (10);
> ERROR:  relation "enr" does not exist
> LINE 1: create view mv as select * from enr
>                                         ^
> QUERY:  create view mv as select * from enr
> CONTEXT:  PL/pgSQL function f() line 2 at SQL statement
> regression=# \errverbose 
> ERROR:  42P01: relation "enr" does not exist
> LINE 1: create view mv as select * from enr
>                                         ^
> QUERY:  create view mv as select * from enr
> CONTEXT:  PL/pgSQL function f() line 2 at SQL statement
> LOCATION:  parserOpenTable, parse_relation.c:1452
> 
> 3. With "create materialized view ..." you get
> 
> regression=# insert into tbl values (10);
> ERROR:  executor could not find named tuplestore "enr"
> CONTEXT:  SQL statement "create materialized view mv as select * from enr"
> PL/pgSQL function f() line 2 at SQL statement
> regression=# \errverbose 
> ERROR:  XX000: executor could not find named tuplestore "enr"
> CONTEXT:  SQL statement "create materialized view mv as select * from enr"
> PL/pgSQL function f() line 2 at SQL statement
> LOCATION:  ExecInitNamedTuplestoreScan, nodeNamedtuplestorescan.c:107
> 
> I don't think hitting an internal error is good enough.
> Why doesn't this case act like case 2?

I agree that raising an internal error is not enough. I attached a updated
patch that outputs a message saying that an ENR can't be used in a matview.

> You could even argue that case 2 isn't good enough either,
> and we should be delivering a specific error message saying
> that an ENR can't be used in a view/matview.  To do that,
> we'd likely need to pass down the QueryEnvironment in more
> places not fewer.

We can raise a similar error for (not materialized) views by passing
QueryEnv to DefineView() (or in ealier stage) , but there are other
objects that can contain ENR in their definition, for examle, functions,
cursor, or RLS policies. Is it worth introducing this version of error
message for all these objects? 

Regards,
Yugo Nagata

-- 
Yugo NAGATA <[email-masked]>
>From 6c72c884954a4223fad192ff021803bf7835e7d7 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email-masked]>
Date: Fri, 15 Nov 2024 15:07:17 +0900
Subject: [PATCH] Prohibit materialized views to use ephemeral named relations

---
 src/backend/parser/analyze.c        | 11 ++++++--
 src/backend/parser/parse_relation.c | 43 ++++++++++++++++++++++++++++-
 src/include/parser/parse_relation.h |  1 +
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 506e063161..c96088c2cc 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -3125,15 +3125,20 @@ transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt)
 					 errmsg("materialized views must not use data-modifying statements in WITH")));
 
 		/*
-		 * Check whether any temporary database objects are used in the
-		 * creation query. It would be hard to refresh data or incrementally
-		 * maintain it if a source disappeared.
+		 * Check whether any temporary database objects or ephemeral named
+		 * relations are used in the creation query. It would be hard to refresh
+		 * data or incrementally maintain it if a source disappeared.
 		 */
 		if (isQueryUsingTempRelation(query))
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 errmsg("materialized views must not use temporary tables or views")));
 
+		if (isQueryUsingENR(query))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("materialized views must not use ephemeral named relations")));
+
 		/*
 		 * A materialized view would either need to save parameters for use in
 		 * maintaining/loading the data or prohibit them entirely.  The latter
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 8075b1b8a1..0e9cb7a1c6 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -102,6 +102,7 @@ static int	specialAttNum(const char *attname);
 static bool rte_visible_if_lateral(ParseState *pstate, RangeTblEntry *rte);
 static bool rte_visible_if_qualified(ParseState *pstate, RangeTblEntry *rte);
 static bool isQueryUsingTempRelation_walker(Node *node, void *context);
+static bool isQueryUsingENR_walker(Node *node, void *context);

 /*
@@ -3893,7 +3894,7 @@ rte_visible_if_qualified(ParseState *pstate, RangeTblEntry *rte)
 
 /*
  * Examine a fully-parsed query, and return true iff any relation underlying
- * the query is a temporary relation (table, view, or materialized view).
+ * the query is an ephemeral named relation.
  */
 bool
 isQueryUsingTempRelation(Query *query)
@@ -3938,6 +3939,46 @@ isQueryUsingTempRelation_walker(Node *node, void *context)
 								  context);
 }
 
+/*
+ * Examine a fully-parsed query, and return true iff any relation underlying
+ * the query is an ephemeral named relation.
+ */
+bool
+isQueryUsingENR(Query *query)
+{
+	return isQueryUsingENR_walker((Node *) query, NULL);
+}
+
+static bool
+isQueryUsingENR_walker(Node *node, void *context)
+{
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, Query))
+	{
+		Query	   *query = (Query *) node;
+		ListCell   *rtable;
+
+		foreach(rtable, query->rtable)
+		{
+			RangeTblEntry *rte = lfirst(rtable);
+
+			if (rte->rtekind == RTE_NAMEDTUPLESTORE)
+				return true;
+		}
+
+		return query_tree_walker(query,
+								 isQueryUsingENR_walker,
+								 context,
+								 QTW_IGNORE_JOINALIASES);
+	}
+
+	return expression_tree_walker(node,
+								  isQueryUsingENR_walker,
+								  context);
+}
+
 /*
  * addRTEPermissionInfo
  *		Creates RTEPermissionInfo for a given RTE and adds it into the
diff --git a/src/include/parser/parse_relation.h b/src/include/parser/parse_relation.h
index 91fd8e243b..2849da7fbd 100644
--- a/src/include/parser/parse_relation.h
+++ b/src/include/parser/parse_relation.h
@@ -127,5 +127,6 @@ extern const NameData *attnumAttName(Relation rd, int attid);
 extern Oid	attnumTypeId(Relation rd, int attid);
 extern Oid	attnumCollationId(Relation rd, int attid);
 extern bool isQueryUsingTempRelation(Query *query);
+extern bool isQueryUsingENR(Query *query);
 
 #endif							/* PARSE_RELATION_H */
-- 
2.34.1

---

From: Michael Paquier <[email-masked]>
Date: Mon, 18 Nov 2024 13:49:20 +0900

> patch that outputs a message saying that an ENR can't be used in a matview.

Hmm..  To get a better idea of the scope you are foreseeing here,
should this include some test coverage?
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Wed, 20 Nov 2024 12:43:16 -0500

> message for all these objects? 

If it's worth checking for here, why not in other cases?

I'm not sure I like using isQueryUsingTempRelation as a model,
because its existing use in transformCreateTableAsStmt seems
like mostly a hack.  (And I definitely don't love introducing
yet another scan of the query.)  It seems to me that we should
think about this, for MVs as well as those other object types,
as fundamentally a dependency problem.  That is, the reason
we can't allow a reference to an ENR in a long-lived object
is that we have no catalog representation for the reference.
So that leads to thinking that the issue ought to be handled
in recordDependencyOnExpr and friends.  If we see an ENR while
scanning a rangetable to extract dependencies, then complain.
This might be a bit messy to produce good error messages for,
though.

Speaking of error messages, I'm not sure that it's okay to
use the phrase "ephemeral named relation" in a user-facing
error message.  We don't use that term in our documentation
AFAICS, except in some SPI documentation that most users
will never have read.  In the context of triggers, "transition
relation" seems to be what the docs use.

			regards, tom lane

---

From: Kirill Reshke <[email-masked]>
Date: Fri, 29 Nov 2024 16:00:13 +0500

> message for all these objects?
Hi!

There are review comments that need to be addressed.

Commitfest status is now waiting on the author.

[0] https://www.postgresql.org/message-id/ZzrHUEaWB67EAZpW%40paquier.xyz
[1] https://www.postgresql.org/message-id/222722.1732124596%40sss.pgh.pa.us

-- 
Best regards,
Kirill Reshke

---

From: Yugo NAGATA <[email-masked]>
Date: Thu, 5 Dec 2024 23:16:01 +0900

On Wed, 20 Nov 2024 12:43:16 -0500

> relation" seems to be what the docs use.

Thank you for your suggestion.

I've attached a updated patch. Use of ENRs are now checked in
find_expr_references_walker() called from recordDependencyOnExpr().

The message is changed to "transition tables cannot be used rule"
because the view definition is stored in the pg_rewrite catalog as
a rule. 

Regards,
Yugo Nagata

>From 34d09775e92b0ac39cd8656d69164b13c92f6fa2 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email-masked]>
Date: Mon, 2 Dec 2024 09:39:34 +0900
Subject: [PATCH v2] Prohibit materialized views to use ephemeral named
 relations

---
 src/backend/catalog/dependency.c       | 24 ++++++++++++++++++++++++
 src/test/regress/expected/triggers.out | 13 +++++++++++++
 src/test/regress/sql/triggers.sql      | 13 +++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 2afc550540..f323e8c91e 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -138,6 +138,7 @@ typedef struct
 /* for find_expr_references_walker */
 typedef struct
 {
+	const ObjectAddress *depender;	/* depender object */
 	ObjectAddresses *addrs;		/* addresses being accumulated */
 	List	   *rtables;		/* list of rangetables to resolve Vars */
 } find_expr_references_context;
@@ -1556,6 +1557,7 @@ recordDependencyOnExpr(const ObjectAddress *depender,
 {
 	find_expr_references_context context;
 
+	context.depender = depender;
 	context.addrs = new_object_addresses();
 
 	/* Set up interpretation for Vars at varlevelsup = 0 */
@@ -1602,6 +1604,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
 	find_expr_references_context context;
 	RangeTblEntry rte = {0};
 
+	context.depender = depender;
 	context.addrs = new_object_addresses();
 
 	/* We gin up a rather bogus rangetable list to handle Vars */
@@ -1739,6 +1742,17 @@ find_expr_references_walker(Node *node,
 			/* (done out of line, because it's a bit bulky) */
 			process_function_rte_ref(rte, var->varattno, context);
 		}
+		else if (rte->rtekind == RTE_NAMEDTUPLESTORE)
+		{
+			/*
+			 * Catalog objects cannot depend on a transtion table which has
+			 * no catalog representation.
+			 */
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("transition table cannot be used in %s",
+							getObjectTypeDescription(context->depender, true))));
+		}
 
 		/*
 		 * Vars referencing other RTE types require no additional work.  In
@@ -2191,6 +2205,16 @@ find_expr_references_walker(Node *node,
 					}
 					context->rtables = list_delete_first(context->rtables);
 					break;
+				case RTE_NAMEDTUPLESTORE:
+					/*
+					 * Catalog objects cannot depend on a transtion table
+					 * which has no catalog representation.
+					 */
+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("transition table cannot be used in %s",
+									getObjectTypeDescription(context->depender, true))));
+					break;
 				default:
 					break;
 			}
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index a044d6afe2..e3522cf3bb 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3728,3 +3728,16 @@ Inherits: parent
 
 drop table parent, child;
 drop function f();
+-- Verify prohibition of materialized views defined using tansition table
+create table my_table (a int);
+create function make_matview() returns trigger as
+$$ begin create materialized view mv as select * from new_table; return new; end $$
+language plpgsql;
+create trigger make_matview after insert on my_table
+referencing new table as new_table
+for each statement execute function make_matview();
+insert into my_table values (42);
+ERROR:  transition tables cannot be used in rule
+CONTEXT:  SQL statement "create materialized view mv as select * from new_table"
+PL/pgSQL function make_matview() line 1 at SQL statement
+drop table my_table;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 51610788b2..f83bd84e6c 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2551,6 +2551,7 @@ merge into merge_target_table t
 using merge_source_table s
 on t.a = s.a
 when matched and s.a <= 2 then
+
 	update set b = t.b || ' updated by merge'
 when matched and s.a > 2 then
 	delete
@@ -2820,3 +2821,15 @@ alter trigger parenttrig on parent rename to anothertrig;
 
 drop table parent, child;
 drop function f();
+
+-- Verify prohibition of materialized views defined using tansition table
+
+create table my_table (a int);
+create function make_matview() returns trigger as
+$$ begin create materialized view mv as select * from new_table; return new; end $$
+language plpgsql;
+create trigger make_matview after insert on my_table
+referencing new table as new_table
+for each statement execute function make_matview();
+insert into my_table values (42);
+drop table my_table;
-- 
2.34.1