On Wed, 20 Nov 2024 12:43:16 -0500
Tom Lane <t...@sss.pgh.pa.us> wrote:
> Yugo NAGATA <nag...@sraoss.co.jp> writes:
> >> 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?
>
> 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.
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
--
Yugo NAGATA <nag...@sraoss.co.jp>
>From 34d09775e92b0ac39cd8656d69164b13c92f6fa2 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nag...@sraoss.co.jp>
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
