Thread
Commits
-
Disallow NAMEDTUPLESTORE RTEs in stored views, rules, etc.
- 3c49d462dbcf 18.0 landed
-
When creating materialized views, use REFRESH to load data.
- b4da732fd64e 17.0 cited
-
EphemeralNamedRelation and materialized view
Yugo Nagata <nagata@sraoss.co.jp> — 2024-07-26T07:07:14Z
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> -
Re: EphemeralNamedRelation and materialized view
Kirill Reshke <reshkekirill@gmail.com> — 2024-08-10T07:22:00Z
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
-
Re: EphemeralNamedRelation and materialized view
Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-03T18:42:33Z
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 -
Re: EphemeralNamedRelation and materialized view
Yugo Nagata <nagata@sraoss.co.jp> — 2024-11-15T08:36:47Z
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>
-
Re: EphemeralNamedRelation and materialized view
Michael Paquier <michael@paquier.xyz> — 2024-11-18T04:49:20Z
On Fri, Nov 15, 2024 at 05:36:47PM +0900, Yugo NAGATA wrote: > 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. Hmm.. To get a better idea of the scope you are foreseeing here, should this include some test coverage? -- Michael
-
Re: EphemeralNamedRelation and materialized view
Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-20T17:43:16Z
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
-
Re: EphemeralNamedRelation and materialized view
Kirill Reshke <reshkekirill@gmail.com> — 2024-11-29T11:00:13Z
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
-
Re: EphemeralNamedRelation and materialized view
Yugo Nagata <nagata@sraoss.co.jp> — 2024-12-05T14:16:01Z
On Wed, 20 Nov 2024 12:43:16 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. 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 <nagata@sraoss.co.jp>
-
Re: EphemeralNamedRelation and materialized view
Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-30T21:06:06Z
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
-
Re: EphemeralNamedRelation and materialized view
Yugo Nagata <nagata@sraoss.co.jp> — 2025-01-08T08:14:30Z
On Mon, 30 Dec 2024 16:06:06 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. Thank you for your reviewing and editing the patch! I agree with your proposal on the error message handling. > 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? On second thought, I could not think of such a case. This part can be removed. I attached v4 patch. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
-
Re: EphemeralNamedRelation and materialized view
Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-08T21:37:35Z
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