Horton

Thread

Commits

  1. Retain relkind too in RTE_SUBQUERY entries for views.

  1. Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-09T08:28:48Z

    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

  2. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-09T09:04:55Z

    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

  3. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-09T11:25:32Z

    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

  4. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-09T14:46:48Z

    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

  5. Re: Views no longer in rangeTabls?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-06-09T16:14:46Z

    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

  6. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-10T06:51:34Z

    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

  7. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-10T06:57:22Z

    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

  8. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-10T07:30:29Z

    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

  9. Re: Views no longer in rangeTabls?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-06-10T12:56:47Z

    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

  10. Re: Views no longer in rangeTabls?

    Julien Rouhaud <rjuju123@gmail.com> — 2023-06-10T13:18:57Z

    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

  11. Re: Views no longer in rangeTabls?

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-06-10T13:27:38Z

    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

  12. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-13T04:09:21Z

    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

  13. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-13T07:44:44Z

    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

  14. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-13T08:27:30Z

    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

  15. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-13T08:57:36Z

    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

  16. Re: Views no longer in rangeTabls?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-06-13T09:33:40Z

    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)

  17. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-13T09:38:42Z

    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

  18. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-13T12:40:12Z

    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

  19. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-14T03:08:55Z

    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

  20. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-14T05:34:56Z

    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

  21. Re: Views no longer in rangeTabls?

    Michael Paquier <michael@paquier.xyz> — 2023-06-14T06:44:21Z

    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

  22. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-14T06:49:04Z

    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

  23. Re: Views no longer in rangeTabls?

    Amit Langote <amitlangote09@gmail.com> — 2023-06-14T10:51:18Z

    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:

https://www.postgresql.org/message-id/E1q9Gms-001h5g-8Q%40gemulon.postgresql.org


-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

  24. Re: Views no longer in rangeTabls?

    David Steele <david@pgmasters.net> — 2023-06-14T12:34:22Z

    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