Subject: Re: Views no longer in rangeTabls?

From: David Steele <[email-masked]>
Date: Sat, 10 Jun 2023 09:51:34 +0300

On 6/9/23 19:14, Tom Lane wrote:
> David Steele <[email-masked]> 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

---

From: Amit Langote <[email-masked]>
Date: Sat, 10 Jun 2023 15:57:22 +0900

> Yeah, that's a good trick. Even so, I wonder why relkind is not set when

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

---

From: David Steele <[email-masked]>
Date: Sat, 10 Jun 2023 10:30:29 +0300

On 6/10/23 09:57, Amit Langote wrote:
> On Sat, Jun 10, 2023 at 15:51 David Steele <[email-masked] 
> <mailto:[email-masked]>> 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.

Regards,
-David

---

From: Tom Lane <[email-masked]>
Date: Sat, 10 Jun 2023 08:56:47 -0400

> +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

---

From: Julien Rouhaud <[email-masked]>
Date: Sat, 10 Jun 2023 21:18:57 +0800

> for struct RangeTblEntry.

and also handle that field in (read|out)funcs.c

---

From: Tom Lane <[email-masked]>
Date: Sat, 10 Jun 2023 09:27:38 -0400

> 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

---

From: Amit Langote <[email-masked]>
Date: Tue, 13 Jun 2023 13:09:21 +0900

> 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.

---

From: David Steele <[email-masked]>
Date: Tue, 13 Jun 2023 09:44:44 +0200

> 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;

> #ifdef USE_ASSERT_CHECKING block in ExecCheckPermissions() that

This seems like a good place for the assert.

Thanks!
-David

---

From: Amit Langote <[email-masked]>
Date: Tue, 13 Jun 2023 17:27:30 +0900

On Tue, Jun 13, 2023 at 4:44 PM David Steele <[email-masked]> wrote:
> everything worked.

Thanks for the review.

> I decided to go with the following because I think it
> is easier to read:
>

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.

> 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.

---

From: David Steele <[email-masked]>
Date: Tue, 13 Jun 2023 10:57:36 +0200

> 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'd like to push this tomorrow barring objections.

+1 for the new comment.

Regards,
-David