Views no longer in rangeTabls?
Hackers,
While updating pgAudit for PG16 I found the following (from our
perspective) regression.
In prior versions of Postgres, views were listed in rangeTabls when
ExecutorCheckPerms_hook() was called but in PG16 the views are no longer
in this list. The permissions have been broken out into permInfos as of
a61b1f748 and this list does include the view.
It seems the thing to do here would be to scan permInfos instead, which
works fine except that we also need access to rellockmode, which is only
included in rangeTabls. We can add a scan of rangeTabls to get
rellockmode when needed and we might be better off overall since
permInfos will generally have fewer entries. I have not implemented this
yet but it seems like it will work.
From reading the discussion it appears this change to rangeTabls was
intentional, but I wonder if I am missing something. For instance, is
there a better way to get rellockmode when scanning permInfos?
It seems unlikely that we are the only ones using rangeTabls in an
extension, so others might benefit from having an answer to this on list.
Thanks,
-David
On 6/9/23 11:28, David Steele wrote:
It seems the thing to do here would be to scan permInfos instead, which
works fine except that we also need access to rellockmode, which is only
included in rangeTabls. We can add a scan of rangeTabls to get
rellockmode when needed and we might be better off overall since
permInfos will generally have fewer entries. I have not implemented this
yet but it seems like it will work.
I implemented this and it does work, but it was not as straight forward
as I would have liked. To make the relationship from RTEPermissionInfo
back to RangeTblEntry I was forced to generate my own perminfoindex.
This was not hard to do but seems a bit fragile. Perhaps we need an
rteindex in RTEPermissionInfo? This would also avoid the need to scan
rangeTabls.
Regards,
-David
Hi David,
On Fri, Jun 9, 2023 at 17:28 David Steele <david@pgmasters.net> wrote:
Hackers,
While updating pgAudit for PG16 I found the following (from our
perspective) regression.In prior versions of Postgres, views were listed in rangeTabls when
ExecutorCheckPerms_hook() was called but in PG16 the views are no longer
in this list.
I’m not exactly sure how pgAudit’s code is searching for view relations in
the range table, but if the code involves filtering on rtekind ==
RTE_RELATION, then yes, such code won’t find views anymore. That’s because
the rewriter no longer adds extraneous RTE_RELATION RTEs for views into the
range table. Views are still there, it’s just that their RTEs are of kind
RTE_SUBQUERY, but they do contain some RELATION fields like relid,
rellockmode, etc. So an extension hook’s relation RTE filtering code
should also consider relid, not just rtekind.
I’m away from a computer atm, so I am not able to easily copy-paste an
example of that from the core code, but maybe you can search for code sites
that need to filter out relation RTEs from the range table.
Perhaps, we are missing a comment near the hook definition mentioning this
detail about views.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Hi Amit,
On 6/9/23 14:25, Amit Langote wrote:
On Fri, Jun 9, 2023 at 17:28 David Steele <david@pgmasters.net
<mailto:david@pgmasters.net>> wrote:In prior versions of Postgres, views were listed in rangeTabls when
ExecutorCheckPerms_hook() was called but in PG16 the views are no
longer
in this list.I’m not exactly sure how pgAudit’s code is searching for view relations
in the range table, but if the code involves filtering on rtekind ==
RTE_RELATION, then yes, such code won’t find views anymore. That’s
because the rewriter no longer adds extraneous RTE_RELATION RTEs for
views into the range table. Views are still there, it’s just that their
RTEs are of kind RTE_SUBQUERY, but they do contain some RELATION fields
like relid, rellockmode, etc. So an extension hook’s relation RTE
filtering code should also consider relid, not just rtekind.
Thank you, this was very helpful. I am able to get the expected result
now with:
/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))
continue;
One thing, though, rte->relkind is not set for views, so I still need to
call get_rel_relkind(rte->relid). Not a big deal, but do you think it
would make sense to set rte->relkind for views?
Perhaps, we are missing a comment near the hook definition mentioning
this detail about views.
I don't see any meaningful comments near the hook definition. That would
certainly be helpful.
Thanks!
-David
David Steele <david@pgmasters.net> writes:
Thank you, this was very helpful. I am able to get the expected result
now with:
/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))
continue;
Right, that matches places like add_rtes_to_flat_rtable().
One thing, though, rte->relkind is not set for views, so I still need to
call get_rel_relkind(rte->relid). Not a big deal, but do you think it
would make sense to set rte->relkind for views?
If you see "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)",
it's dead certain that relid refers to a view, so you could just wire
in that knowledge.
regards, tom lane
On 6/9/23 19:14, Tom Lane wrote:
David Steele <david@pgmasters.net> writes:
Thank you, this was very helpful. I am able to get the expected result
now with:/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))
continue;Right, that matches places like add_rtes_to_flat_rtable().
Good to have confirmation of that, thanks!
One thing, though, rte->relkind is not set for views, so I still need to
call get_rel_relkind(rte->relid). Not a big deal, but do you think it
would make sense to set rte->relkind for views?If you see "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)",
it's dead certain that relid refers to a view, so you could just wire
in that knowledge.
Yeah, that's a good trick. Even so, I wonder why relkind is not set when
relid is set?
Regards,
-David
On Sat, Jun 10, 2023 at 15:51 David Steele <david@pgmasters.net> wrote:
On 6/9/23 19:14, Tom Lane wrote:
David Steele <david@pgmasters.net> writes:
Thank you, this was very helpful. I am able to get the expected result
now with:/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))
continue;Right, that matches places like add_rtes_to_flat_rtable().
Good to have confirmation of that, thanks!
One thing, though, rte->relkind is not set for views, so I still need to
call get_rel_relkind(rte->relid). Not a big deal, but do you think it
would make sense to set rte->relkind for views?If you see "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)",
it's dead certain that relid refers to a view, so you could just wire
in that knowledge.Yeah, that's a good trick. Even so, I wonder why relkind is not set when
relid is set?
I too have been thinking that setting relkind might be a good idea, even if
only as a crosscheck that only view relations can look like that in the
range table.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On 6/10/23 09:57, Amit Langote wrote:
On Sat, Jun 10, 2023 at 15:51 David Steele <david@pgmasters.net
<mailto:david@pgmasters.net>> wrote:
On 6/9/23 19:14, Tom Lane wrote:If you see "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)",
it's dead certain that relid refers to a view, so you could just wire
in that knowledge.Yeah, that's a good trick. Even so, I wonder why relkind is not set
when
relid is set?I too have been thinking that setting relkind might be a good idea, even
if only as a crosscheck that only view relations can look like that in
the range table.
+1. Even better if we can do it for PG16.
Regards,
-David
David Steele <david@pgmasters.net> writes:
On 6/10/23 09:57, Amit Langote wrote:
I too have been thinking that setting relkind might be a good idea, even
if only as a crosscheck that only view relations can look like that in
the range table.
+1. Even better if we can do it for PG16.
Well, if we're gonna do it we should do it for v16, rather than
change the data structure twice. It wouldn't be hard exactly:
/*
* 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.
*/
- rte->relkind = 0;
rte->tablesample = NULL;
rte->inh = false; /* must not be set for a subquery */
plus adjustment of that comment and probably also the comment
for struct RangeTblEntry.
regards, tom lane
On Sat, Jun 10, 2023 at 08:56:47AM -0400, Tom Lane wrote:
Well, if we're gonna do it we should do it for v16, rather than
change the data structure twice. It wouldn't be hard exactly:/*
* 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.
*/
- rte->relkind = 0;
rte->tablesample = NULL;
rte->inh = false; /* must not be set for a subquery */plus adjustment of that comment and probably also the comment
for struct RangeTblEntry.
and also handle that field in (read|out)funcs.c
Julien Rouhaud <rjuju123@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.
regards, tom lane
On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Julien Rouhaud <rjuju123@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
Attachments:
v1-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patchapplication/x-patch; name=v1-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patchDownload
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
On 6/13/23 06:09, Amit Langote wrote:
On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Julien Rouhaud <rjuju123@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?
The patch looks good to me. I also tested it against pgAudit and
everything worked. I decided to go with the following because I think it
is easier to read:
/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && rte->relkind == RELKIND_VIEW)))
continue;
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.
This seems like a good place for the assert.
Thanks!
-David
On Tue, Jun 13, 2023 at 4:44 PM David Steele <david@pgmasters.net> wrote:
On 6/13/23 06:09, Amit Langote wrote:
On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Julien Rouhaud <rjuju123@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?
The patch looks good to me. I also tested it against pgAudit and
everything worked.
Thanks for the review.
I decided to go with the following because I think it
is easier to read:/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && rte->relkind == RELKIND_VIEW)))
continue;
It didn't occur to me so far to mention it but this could be replaced with:
if (rte->perminfoindex != 0)
and turn that condition into an Assert instead, like the loop over
range table in ExecCheckPermissions() does.
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.This seems like a good place for the assert.
I added a comment above this Assert.
I'd like to push this tomorrow barring objections.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
v2-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patchapplication/octet-stream; name=v2-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patchDownload
From afa3d889d82e2bd274fc6b2d28f13f8b4b280e28 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Tue, 13 Jun 2023 12:52:47 +0900
Subject: [PATCH v2] 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>
Reviewed-by: David Steele <david@pgmasters.net>
Discussion: https://postgr.es/m/3953179e-9540-e5d1-a743-4bef368785b0%40pgmasters.net
---
src/backend/executor/execMain.c | 9 +++++++++
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, 22 insertions(+), 12 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c76fdf59ec..4c5a7bbf62 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -595,6 +595,15 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos,
if (rte->perminfoindex != 0)
{
/* Sanity checks */
+
+ /*
+ * Only relation RTEs and subquery RTEs that were once relation
+ * RTEs (views) have their perminfoindex set.
+ */
+ 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
On 6/13/23 10:27, Amit Langote wrote:
On Tue, Jun 13, 2023 at 4:44 PM David Steele <david@pgmasters.net> wrote:
I decided to go with the following because I think it
is easier to read:/* We only care about tables/views and can ignore subqueries, etc. */
if (!(rte->rtekind == RTE_RELATION ||
(rte->rtekind == RTE_SUBQUERY && rte->relkind == RELKIND_VIEW)))
continue;It didn't occur to me so far to mention it but this could be replaced with:
if (rte->perminfoindex != 0)
and turn that condition into an Assert instead, like the loop over
range table in ExecCheckPermissions() does.
Hmmm, that might work, and save us a filter on rte->perminfoindex later
on (to filter out table partitions). Thanks for the tip!
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.This seems like a good place for the assert.
I added a comment above this Assert.
I'd like to push this tomorrow barring objections.
+1 for the new comment.
Regards,
-David
Note that you changed one comment from "permission checks" to
"permission hecks".
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)
On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Note that you changed one comment from "permission checks" to
"permission hecks".
Oops, thanks for pointing that out.
Fixed in the attached.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
v3-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patchapplication/octet-stream; name=v3-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patchDownload
From f3363cad90af434f2cabfdc3cf1402d9a22edf55 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Tue, 13 Jun 2023 12:52:47 +0900
Subject: [PATCH v3] 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>
Reviewed-by: David Steele <david@pgmasters.net>
Discussion: https://postgr.es/m/3953179e-9540-e5d1-a743-4bef368785b0%40pgmasters.net
---
src/backend/executor/execMain.c | 9 +++++++++
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, 22 insertions(+), 12 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c76fdf59ec..4c5a7bbf62 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -595,6 +595,15 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos,
if (rte->perminfoindex != 0)
{
/* Sanity checks */
+
+ /*
+ * Only relation RTEs and subquery RTEs that were once relation
+ * RTEs (views) have their perminfoindex set.
+ */
+ 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..a2fd8c4690 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 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, RTE_NAMEDTUPLESTORE can also set relid to indicate
* that the tuple format of the tuplestore is the same as the referenced
--
2.35.3
On 6/13/23 11:38, Amit Langote wrote:
On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Note that you changed one comment from "permission checks" to
"permission hecks".Oops, thanks for pointing that out.
Fixed in the attached.
I have done another (more careful) review of the comments and I don't
see any other issues.
Regards,
-David
On Tue, Jun 13, 2023 at 9:40 PM David Steele <david@pgmasters.net> wrote:
On 6/13/23 11:38, Amit Langote wrote:
On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Note that you changed one comment from "permission checks" to
"permission hecks".Oops, thanks for pointing that out.
Fixed in the attached.
I have done another (more careful) review of the comments and I don't
see any other issues.
Thanks for checking.
Seeing no other comments, I've pushed this after rewriting the
comments that needed to be changed to mention "relkind" right after
"relid".
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On Wed, Jun 14, 2023 at 12:08 Amit Langote <amitlangote09@gmail.com> wrote:
On Tue, Jun 13, 2023 at 9:40 PM David Steele <david@pgmasters.net> wrote:
On 6/13/23 11:38, Amit Langote wrote:
On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera <
alvherre@alvh.no-ip.org> wrote:
Note that you changed one comment from "permission checks" to
"permission hecks".Oops, thanks for pointing that out.
Fixed in the attached.
I have done another (more careful) review of the comments and I don't
see any other issues.Thanks for checking.
Seeing no other comments, I've pushed this after rewriting the
comments that needed to be changed to mention "relkind" right after
"relid".
This being my first commit, I intently looked to check if everything’s set
up correctly. While it seemed to have hit gitweb and GitHub, it didn’t
pgsql-committers for some reason.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On Wed, Jun 14, 2023 at 02:34:56PM +0900, Amit Langote wrote:
This being my first commit, I intently looked to check if everything’s set
up correctly. While it seemed to have hit gitweb and GitHub, it didn’t
pgsql-committers for some reason.
It seems to me that the email of pgsql-committers is just being held
in moderation. Your first commit is in the tree, so this worked fine
seen from here. Congrats!
--
Michael
On Wed, Jun 14, 2023 at 15:44 Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 14, 2023 at 02:34:56PM +0900, Amit Langote wrote:
This being my first commit, I intently looked to check if everything’s
set
up correctly. While it seemed to have hit gitweb and GitHub, it didn’t
pgsql-committers for some reason.It seems to me that the email of pgsql-committers is just being held
in moderation. Your first commit is in the tree, so this worked fine
seen from here. Congrats!
Ah, did think it might be moderation. Thanks for the confirmation, Michael.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On Wed, Jun 14, 2023 at 15:49 Amit Langote <amitlangote09@gmail.com> wrote:
On Wed, Jun 14, 2023 at 15:44 Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 14, 2023 at 02:34:56PM +0900, Amit Langote wrote:
This being my first commit, I intently looked to check if everything’s
set
up correctly. While it seemed to have hit gitweb and GitHub, it didn’t
pgsql-committers for some reason.It seems to me that the email of pgsql-committers is just being held
in moderation. Your first commit is in the tree, so this worked fine
seen from here. Congrats!Ah, did think it might be moderation. Thanks for the confirmation,
Michael.
It’s out now:
/messages/by-id/E1q9Gms-001h5g-8Q@gemulon.postgresql.org
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On 6/14/23 12:51, Amit Langote wrote:
Ah, did think it might be moderation. Thanks for the confirmation,
Michael.It’s out now:
/messages/by-id/E1q9Gms-001h5g-8Q@gemulon.postgresql.org </messages/by-id/E1q9Gms-001h5g-8Q@gemulon.postgresql.org>
Thank you for committing this and congratulations on your first commit!
Regards,
-David