EphemeralNamedRelation and materialized view

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

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>

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

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]: /messages/by-id/ZzrHUEaWB67EAZpW@paquier.xyz
[1]: /messages/by-id/222722.1732124596@sss.pgh.pa.us

--
Best regards,
Kirill Reshke

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>

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

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>

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