Обсуждение: 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 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 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

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

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

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 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 6/14/23 12:51, Amit Langote wrote:
> 
>     Ah, did think it might be moderation.  Thanks for the confirmation,
>     Michael.
> 
> It’s out now:
> 
> https://www.postgresql.org/message-id/E1q9Gms-001h5g-8Q%40gemulon.postgresql.org
<https://www.postgresql.org/message-id/E1q9Gms-001h5g-8Q%40gemulon.postgresql.org>

Thank you for committing this and congratulations on your first commit!

Regards,
-David