On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Julien Rouhaud <rjuju...@gmail.com> writes: > > On Sat, Jun 10, 2023 at 08:56:47AM -0400, Tom Lane wrote: > >> - rte->relkind = 0; > > > and also handle that field in (read|out)funcs.c > > Oh, right. Ugh, that means a catversion bump. It's not like > we've never done that during beta, but it's kind of an annoying > cost for a detail as tiny as this.
OK, so how about the attached? I considered adding Assert(relkind == RELKIND_VIEW) in all places that have the "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)" condition, but that seemed like an overkill, so only added one in the #ifdef USE_ASSERT_CHECKING block in ExecCheckPermissions() that f75cec4fff877 added. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
From f7390a898b7e156d75372d28ea5698d2ced9795b Mon Sep 17 00:00:00 2001 From: Amit Langote <amitlan@postgresql.org> Date: Tue, 13 Jun 2023 12:52:47 +0900 Subject: [PATCH v1] Retain relkind too in RTE_SUBQUERY entries for views. 47bb9db75 modified the ApplyRetrieveRule()'s conversion of a view's original RTE_RELATION entry into an RTE_SUBQUERY one to retain relid, rellockmode, and perminfoindex so that the executor can lock the view and check its permissions. It seems better to also retain relkind for cross-checking that the exception of an RTE_SUBQUERY entry being allowed to carry relation details only applies to views, so do so. Bump catversion because this changes the output format of RTE_SUBQUERY RTEs. Suggested-by: David Steele <david@pgmasters.net> Discussion: https://postgr.es/m/3953179e-9540-e5d1-a743-4bef368785b0%40pgmasters.net --- src/backend/executor/execMain.c | 3 +++ src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/rewrite/rewriteHandler.c | 7 +++---- src/include/catalog/catversion.h | 2 +- src/include/nodes/parsenodes.h | 14 +++++++------- 6 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index c76fdf59ec..7aed5e7b36 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -595,6 +595,9 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos, if (rte->perminfoindex != 0) { /* Sanity checks */ + Assert(rte->rtekind == RTE_RELATION || + (rte->rtekind == RTE_SUBQUERY && + rte->relkind == RELKIND_VIEW)); (void) getRTEPermissionInfo(rteperminfos, rte); /* Many-to-one mapping not allowed */ Assert(!bms_is_member(rte->perminfoindex, indexset)); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index ba00b99249..955286513d 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -513,6 +513,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node) WRITE_BOOL_FIELD(security_barrier); /* we re-use these RELATION fields, too: */ WRITE_OID_FIELD(relid); + WRITE_CHAR_FIELD(relkind); WRITE_INT_FIELD(rellockmode); WRITE_UINT_FIELD(perminfoindex); break; diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 597e5b3ea8..a136ae1d60 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -503,6 +503,7 @@ _readRangeTblEntry(void) READ_BOOL_FIELD(security_barrier); /* we re-use these RELATION fields, too: */ READ_OID_FIELD(relid); + READ_CHAR_FIELD(relkind); READ_INT_FIELD(rellockmode); READ_UINT_FIELD(perminfoindex); break; diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 0e4f76efa8..0967be762c 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1849,11 +1849,10 @@ ApplyRetrieveRule(Query *parsetree, /* * Clear fields that should not be set in a subquery RTE. Note that we - * leave the relid, rellockmode, and perminfoindex fields set, so that the - * view relation can be appropriately locked before execution and its - * permissions checked. + * leave the relid, rellockmode, relkind, and perminfoindex fields set, so + * that the view relation can be appropriately locked before execution and + * its permissions checked. */ - rte->relkind = 0; rte->tablesample = NULL; rte->inh = false; /* must not be set for a subquery */ diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index c784937a0e..fe70d8396d 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202305211 +#define CATALOG_VERSION_NO 202306141 #endif diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 0ca298f5a1..786692781d 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1056,13 +1056,13 @@ typedef struct RangeTblEntry * this RTE in the containing struct's list of same; 0 if permissions need * not be checked for this RTE. * - * As a special case, relid, rellockmode, and perminfoindex can also be - * set (nonzero) in an RTE_SUBQUERY RTE. This occurs when we convert an - * RTE_RELATION RTE naming a view into an RTE_SUBQUERY containing the - * view's query. We still need to perform run-time locking and permission - * checks on the view, even though it's not directly used in the query - * anymore, and the most expedient way to do that is to retain these - * fields from the old state of the RTE. + * As a special case, relid, rellockmode, relkind, and perminfoindex can + * also be set (nonzero) in an RTE_SUBQUERY RTE. This occurs when we + * convert an RTE_RELATION RTE naming a view into an RTE_SUBQUERY + * containing the view's query. We still need to perform run-time locking + * and permission hecks on the view, even though it's not directly used in + * the query anymore, and the most expedient way to do that is to retain + * these fields from the old state of the RTE. * * As a special case, RTE_NAMEDTUPLESTORE can also set relid to indicate * that the tuple format of the tuplestore is the same as the referenced -- 2.35.3
