On Mon, 30 Dec 2024 16:06:06 -0500 Tom Lane <t...@sss.pgh.pa.us> wrote:
> Yugo NAGATA <nag...@sraoss.co.jp> writes: > > On Wed, 20 Nov 2024 12:43:16 -0500 > > Tom Lane <t...@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 <nag...@sraoss.co.jp>
>From 40f96b4ce8e93e8920e840e52b6a78f60a319efd Mon Sep 17 00:00:00 2001 From: Tom Lane <t...@sss.pgh.pa.us> Date: Mon, 30 Dec 2024 15:59:32 -0500 Subject: [PATCH v4] 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.e74d0db579f2c017e1ca0...@sraoss.co.jp --- src/backend/catalog/dependency.c | 15 +++++++++++++++ src/test/regress/expected/triggers.out | 20 ++++++++++++++++++++ src/test/regress/sql/triggers.sql | 19 +++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 096b68c7f3..18316a3968 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -2191,7 +2191,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.34.1
