Re: EphemeralNamedRelation and materialized view - Mailing list pgsql-hackers

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