On Sun, 03 Nov 2024 13:42:33 -0500
Tom Lane <t...@sss.pgh.pa.us> wrote:
> Yugo Nagata <nag...@sraoss.co.jp> 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 <nag...@sraoss.co.jp>
>From 6c72c884954a4223fad192ff021803bf7835e7d7 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
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
