public inbox for [email protected]help / color / mirror / Atom feed
Re: Views no longer in rangeTabls? 11+ messages / 5 participants [nested] [flat]
* Re: Views no longer in rangeTabls? @ 2023-06-10 06:51 David Steele <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: David Steele @ 2023-06-10 06:51 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Amit Langote <[email protected]>; pgsql-hackers On 6/9/23 19:14, Tom Lane wrote: > David Steele <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-10 06:57 Amit Langote <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Langote @ 2023-06-10 06:57 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> On Sat, Jun 10, 2023 at 15:51 David Steele <[email protected]> wrote: > On 6/9/23 19:14, Tom Lane wrote: > > David Steele <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-10 07:30 David Steele <[email protected]> parent: Amit Langote <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: David Steele @ 2023-06-10 07:30 UTC (permalink / raw) To: Amit Langote <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> On 6/10/23 09:57, Amit Langote wrote: > On Sat, Jun 10, 2023 at 15:51 David Steele <[email protected] > <mailto:[email protected]>> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-10 12:56 Tom Lane <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2023-06-10 12:56 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Amit Langote <[email protected]>; pgsql-hackers David Steele <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-10 13:18 Julien Rouhaud <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Julien Rouhaud @ 2023-06-10 13:18 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: David Steele <[email protected]>; Amit Langote <[email protected]>; pgsql-hackers 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-10 13:27 Tom Lane <[email protected]> parent: Julien Rouhaud <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2023-06-10 13:27 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: David Steele <[email protected]>; Amit Langote <[email protected]>; pgsql-hackers Julien Rouhaud <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-13 04:09 Amit Langote <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Langote @ 2023-06-13 04:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; David Steele <[email protected]>; pgsql-hackers On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <[email protected]> wrote: > Julien Rouhaud <[email protected]> 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: [application/x-patch] v1-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patch (5.3K, ../../CA+HiwqF1CgsZKvBW_Gz9tErKfOp4t=PQgUKvrvWBG0HP8LdAXQ@mail.gmail.com/2-v1-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patch) download | inline diff: From f7390a898b7e156d75372d28ea5698d2ced9795b Mon Sep 17 00:00:00 2001 From: Amit Langote <[email protected]> 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 <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-13 07:44 David Steele <[email protected]> parent: Amit Langote <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: David Steele @ 2023-06-13 07:44 UTC (permalink / raw) To: Amit Langote <[email protected]>; Tom Lane <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; pgsql-hackers On 6/13/23 06:09, Amit Langote wrote: > On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <[email protected]> wrote: >> Julien Rouhaud <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-13 08:27 Amit Langote <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Langote @ 2023-06-13 08:27 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers On Tue, Jun 13, 2023 at 4:44 PM David Steele <[email protected]> wrote: > On 6/13/23 06:09, Amit Langote wrote: > > On Sat, Jun 10, 2023 at 10:27 PM Tom Lane <[email protected]> wrote: > >> Julien Rouhaud <[email protected]> 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: [application/octet-stream] v2-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patch (5.5K, ../../CA+HiwqHAZVm+qDmNLvS6CT2OiNoiRhr-00B3XEkVNOi26+Rpug@mail.gmail.com/2-v2-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patch) download | inline diff: From afa3d889d82e2bd274fc6b2d28f13f8b4b280e28 Mon Sep 17 00:00:00 2001 From: Amit Langote <[email protected]> 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 <[email protected]> Reviewed-by: David Steele <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Views no longer in rangeTabls? @ 2023-06-13 08:57 David Steele <[email protected]> parent: Amit Langote <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: David Steele @ 2023-06-13 08:57 UTC (permalink / raw) To: Amit Langote <[email protected]>; +Cc: Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers On 6/13/23 10:27, Amit Langote wrote: > On Tue, Jun 13, 2023 at 4:44 PM David Steele <[email protected]> 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 ^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v2 2/2] fixups @ 2026-07-07 16:35 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Álvaro Herrera @ 2026-07-07 16:35 UTC (permalink / raw) --- src/backend/commands/repack.c | 54 ++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 2879c8af574..fcc401ccdb9 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -2152,6 +2152,10 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) /* * For USING INDEX, scan pg_index to find those with indisclustered. + * + * Note we don't obtain lock of any kind on the index, which means the + * index or its owning table could be gone or change at any point. We + * have to be extra careful when examining catalog state for them. */ catalog = table_open(IndexRelationId, AccessShareLock); ScanKeyInit(&entry, @@ -2169,7 +2173,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) index = (Form_pg_index) GETSTRUCT(tuple); - classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(index->indrelid)); + classtup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(index->indrelid)); if (!HeapTupleIsValid(classtup)) continue; classForm = (Form_pg_class) GETSTRUCT(classtup); @@ -2178,11 +2182,11 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) if (classForm->relpersistence == RELPERSISTENCE_TEMP && !isTempOrTempToastNamespace(classForm->relnamespace)) { - ReleaseSysCache(classtup); + heap_freetuple(classtup); continue; } - ReleaseSysCache(classtup); + heap_freetuple(classtup); /* noisily skip rels which the user can't process */ if (!repack_is_permitted_for_relation(cmd, index->indrelid, @@ -2274,7 +2278,9 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid, if (get_rel_relkind(child_oid) != RELKIND_INDEX) continue; - table_oid = IndexGetRelation(child_oid, false); + table_oid = IndexGetRelation(child_oid, true); + if (!OidIsValid(table_oid)) + continue; index_oid = child_oid; } else @@ -2309,33 +2315,41 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid, /* - * Return whether userid has privileges to REPACK relid. If not, this - * function emits a WARNING. + * Return whether userid has privileges to execute REPACK on relid. + * + * Caller may not have a lock on the relation, so it could have been + * dropped concurrently. In that case, silently return false. + * + * If the relation does exist but the user doesn't have the required + * privs, emit a WARNING and return false. Otherwise, return true. */ static bool repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid) { bool is_missing = false; + AclResult result; + char *relname; Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK); - if (pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing) == ACLCHECK_OK) + result = pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing); + if (is_missing) + return false; + + if (result == ACLCHECK_OK) return true; - /* Report a warning if the relation still exists. */ - if (!is_missing) + /* + * The relation can also be dropped after we tested its ACL and before we + * read its relname, so be careful. + */ + relname = get_rel_name(relid); + if (relname != NULL) { - char *relname; - - relname = get_rel_name(relid); - if (relname != NULL) - { - ereport(WARNING, - errmsg("permission denied to execute %s on \"%s\", skipping it", - RepackCommandAsString(cmd), relname)); - - pfree(relname); - } + ereport(WARNING, + errmsg("permission denied to execute %s on \"%s\", skipping it", + RepackCommandAsString(cmd), relname)); + pfree(relname); } return false; -- 2.47.3 --op6mnexl7cn72cto-- ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-07-07 16:35 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-06-10 06:51 Re: Views no longer in rangeTabls? David Steele <[email protected]> 2023-06-10 06:57 ` Amit Langote <[email protected]> 2023-06-10 07:30 ` David Steele <[email protected]> 2023-06-10 12:56 ` Tom Lane <[email protected]> 2023-06-10 13:18 ` Julien Rouhaud <[email protected]> 2023-06-10 13:27 ` Tom Lane <[email protected]> 2023-06-13 04:09 ` Amit Langote <[email protected]> 2023-06-13 07:44 ` David Steele <[email protected]> 2023-06-13 08:27 ` Amit Langote <[email protected]> 2023-06-13 08:57 ` David Steele <[email protected]> 2026-07-07 16:35 [PATCH v2 2/2] fixups Álvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox