Thread: EphemeralNamedRelation and materialized view
Hi,
While looking into the commit b4da732fd64e936970f38c792f8b32c4bdf2bcd5,
I noticed that we can create a materialized view using Ephemeral Named
Relation in PostgreSQL 16 or earler.
postgres=# create table tbl (i int);
CREATE TABLE
^
postgres=# create or replace function f() returns trigger as $$ begin
create materialized view mv as select * from enr; return new; end; $$ language plpgsql;
CREATE FUNCTION
postgres=# create trigger trig after insert on tbl referencing new table as enr execute function f();
CREATE TRIGGER
postgres=# insert into tbl values (10);
postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+------+-------------------+--------
public | mv | materialized view | yugo-n
public | tbl | table | yugo-n
(2 rows)
We cannot refresh or get the deinition of it, though.
postgres=# refresh materialized view mv;
ERROR: executor could not find named tuplestore "enr"
postgres=# \d+ mv
ERROR: unrecognized RTE kind: 7
In PostgreSQL 17, materialized view using ENR cannot be created
because queryEnv is not pass to RefreshMatViewByOid introduced by b4da732fd64.
When we try to create it, the error is raised.
ERROR: executor could not find named tuplestore "enr"
Although it is hard to imagine users actually try to create materialized view
using ENR, how about prohibiting it even in PG16 or earlier by passing NULL
as queryEnv arg in CreateQueryDesc to avoid to create useless matviews accidentally,
as the attached patch?
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachment
On Fri, 26 Jul 2024 at 12:07, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > Hi, > > While looking into the commit b4da732fd64e936970f38c792f8b32c4bdf2bcd5, > I noticed that we can create a materialized view using Ephemeral Named > Relation in PostgreSQL 16 or earler. > > > postgres=# create table tbl (i int); > CREATE TABLE > ^ > postgres=# create or replace function f() returns trigger as $$ begin > create materialized view mv as select * from enr; return new; end; $$ language plpgsql; > CREATE FUNCTION > > postgres=# create trigger trig after insert on tbl referencing new table as enr execute function f(); > CREATE TRIGGER > > postgres=# insert into tbl values (10); > > postgres=# \d > List of relations > Schema | Name | Type | Owner > --------+------+-------------------+-------- > public | mv | materialized view | yugo-n > public | tbl | table | yugo-n > (2 rows) > > > We cannot refresh or get the deinition of it, though. > > postgres=# refresh materialized view mv; > ERROR: executor could not find named tuplestore "enr" > > postgres=# \d+ mv > ERROR: unrecognized RTE kind: 7 > > In PostgreSQL 17, materialized view using ENR cannot be created > because queryEnv is not pass to RefreshMatViewByOid introduced by b4da732fd64. > When we try to create it, the error is raised. > > ERROR: executor could not find named tuplestore "enr" > > Although it is hard to imagine users actually try to create materialized view > using ENR, how about prohibiting it even in PG16 or earlier by passing NULL > as queryEnv arg in CreateQueryDesc to avoid to create useless matviews accidentally, > as the attached patch? > > > Regards, > Yugo Nagata > > -- > Yugo Nagata <nagata@sraoss.co.jp> Hi I think this is a clear bug fix, and should be backported in pg v12-v16. LTGM P.S should be set https://commitfest.postgresql.org/49/5153/ entry as RFC? -- Best regards, Kirill Reshke
Yugo Nagata <nagata@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?
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.
regards, tom lane
Yugo NAGATA <nagata@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.
regards, tom lane
On Fri, 15 Nov 2024 at 13:37, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Sun, 03 Nov 2024 13:42:33 -0500 > Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Yugo Nagata <nagata@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 <nagata@sraoss.co.jp> 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
Yugo NAGATA <nagata@sraoss.co.jp> writes:
> On Wed, 20 Nov 2024 12:43:16 -0500
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... 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.
> I've attached a updated patch. Use of ENRs are now checked in
> find_expr_references_walker() called from recordDependencyOnExpr().
This looks pretty good to me, except that I question the use of
getObjectTypeDescription() in the error message. There are a
few things not to like about that:
1. This is kind of an off-label use of getObjectTypeDescription,
in that we can't expect the object to be visible yet in the catalogs.
Yeah, we can hack around that by passing missing_ok = true, but it
still seems like a kluge.
2. The grammar isn't great, and translatability of the message
would be poor I think.
3. As your test case demonstrates, the message is going to complain
about a "rule" if the problem is with a view or matview, because
we represent the dependency as being from the view's ON SELECT rule.
This seems quite confusing for anyone not deeply versed in PG's inner
workings.
After some thought I propose that we just complain that a "persistent
object" can't depend on a transition table, and not try to identify
the depender any more closely than that. We can still add some
context to the message by showing the transition table's name,
since that's readily available from the RTE. See attached v3,
where I also did a bit of editing of the comments and test case.
BTW, I'm not entirely convinced that the first addition (in Var
processing) is necessary. Such a Var must refer to an RTE
somewhere, and I'm having a hard time coming up with a case
where the RTE wouldn't also be part of what we scan for
dependencies. It's harmless enough to have the extra check,
but can you think of a case where it's actually needed?
regards, tom lane
From a0014121e3ca72cb60a7fb627a19c71c85ca84ae Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 30 Dec 2024 15:59:32 -0500
Subject: [PATCH v3] Disallow NAMEDTUPLESTORE RTEs in stored views, rules, etc.
A named tuplestore is necessarily a transient object, so it makes
no sense to reference one in a persistent object such as a view.
We didn't previously prevent that, with the result that if you
tried you would get some weird failure about how the executor
couldn't find the tuplestore.
We can mechanize a check for this case cheaply by making dependency
extraction complain if it comes across such an RTE. This is a
plausible way of dealing with it since part of the problem is that we
have no way to make a pg_depend representation of a named tuplestore.
Report and fix by Yugo Nagata. Although this is an old problem,
it's a very weird corner case and there have been no reports from
end users. So it seems sufficient to fix it in master.
Discussion: https://postgr.es/m/20240726160714.e74d0db579f2c017e1ca0b7e@sraoss.co.jp
---
src/backend/catalog/dependency.c | 28 ++++++++++++++++++++++++++
src/test/regress/expected/triggers.out | 20 ++++++++++++++++++
src/test/regress/sql/triggers.sql | 19 +++++++++++++++++
3 files changed, 67 insertions(+)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 2afc550540..8355481e82 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1739,6 +1739,19 @@ 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)
+ {
+ /*
+ * Cataloged objects cannot depend on tuplestores, because those
+ * have no cataloged representation. For now we can call the
+ * tuplestore a "transition table" because that's the only kind
+ * exposed to SQL, but someday we might have to work harder.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("transition table \"%s\" cannot be referenced in a persistent object",
+ rte->eref->aliasname)));
+ }
/*
* Vars referencing other RTE types require no additional work. In
@@ -2191,7 +2204,22 @@ find_expr_references_walker(Node *node,
}
context->rtables = list_delete_first(context->rtables);
break;
+ case RTE_NAMEDTUPLESTORE:
+
+ /*
+ * Cataloged objects cannot depend on tuplestores, because
+ * those have no cataloged representation. For now we can
+ * call the tuplestore a "transition table" because that's
+ * the only kind exposed to SQL, but someday we might have
+ * to work harder.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("transition table \"%s\" cannot be referenced in a persistent object",
+ rte->eref->aliasname)));
+ break;
default:
+ /* Other RTE types can be ignored here */
break;
}
}
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index a044d6afe2..0657da1757 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3315,6 +3315,26 @@ create trigger my_table_col_update_trig
ERROR: transition tables cannot be specified for triggers with column lists
drop table my_table;
--
+-- Verify that transition tables can't be used in, eg, a view.
+--
+create table my_table (a int);
+create function make_bogus_matview() returns trigger as
+$$ begin
+ create materialized view transition_test_mv as select * from new_table;
+ return new;
+end $$
+language plpgsql;
+create trigger make_bogus_matview
+ after insert on my_table
+ referencing new table as new_table
+ for each statement execute function make_bogus_matview();
+insert into my_table values (42); -- error
+ERROR: transition table "new_table" cannot be referenced in a persistent object
+CONTEXT: SQL statement "create materialized view transition_test_mv as select * from new_table"
+PL/pgSQL function make_bogus_matview() line 2 at SQL statement
+drop table my_table;
+drop function make_bogus_matview();
+--
-- Test firing of triggers with transition tables by foreign key cascades
--
create table refd_table (a int primary key, b text);
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 51610788b2..7e2f7597c1 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2434,6 +2434,25 @@ create trigger my_table_col_update_trig
drop table my_table;
+--
+-- Verify that transition tables can't be used in, eg, a view.
+--
+
+create table my_table (a int);
+create function make_bogus_matview() returns trigger as
+$$ begin
+ create materialized view transition_test_mv as select * from new_table;
+ return new;
+end $$
+language plpgsql;
+create trigger make_bogus_matview
+ after insert on my_table
+ referencing new table as new_table
+ for each statement execute function make_bogus_matview();
+insert into my_table values (42); -- error
+drop table my_table;
+drop function make_bogus_matview();
+
--
-- Test firing of triggers with transition tables by foreign key cascades
--
--
2.43.5
Yugo NAGATA <nagata@sraoss.co.jp> writes:
> Thank you for your reviewing and editing the patch!
> I agree with your proposal on the error message handling.
Cool, pushed v4 then.
regards, tom lane