Re: ExecRTCheckPerms() and many prunable partitions

On Thu, Jul 28, 2022 at 6:04 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > [ v16 patches ]
>
> I took a quick look at this ...

Thanks for the review and sorry about the delay.

> I think that the notion behind MergeRelPermissionInfos, ie that
> a single RelPermissionInfo can represent *all* the checks for
> a given table OID, is fundamentally wrong. For example, when
> merging a view into an outer query that references a table
> also used by the view, the checkAsUser fields might be different,
> and the permissions to check might be different, and the columns
> those permissions need to hold for might be different. Blindly
> bms_union'ing the column sets will lead to requiring far more
> permissions than the query should require. Conversely, this
> approach could lead to allowing cases we should reject, if you
> happen to "merge" checkAsUser in a way that ends in checking as a
> higher-privilege user than should be checked.
>
> I'm inclined to think that you should abandon the idea of
> merging RelPermissionInfos at all. It can only buy us much
> in the case of self-joins, which ought to be rare. It'd
> be better to just say "there is one RelPermissionInfo for
> each RTE requiring any sort of permissions check". Either
> that or you need to complicate RelPermissionInfo a lot, but
> I don't see the advantage.

OK, I agree that the complexity of sharing a RelPermissionInfo between
RTEs far exceeds any performance benefit to be had from it.

I have changed things so that there's one RelPermissionInfo for every
RTE_RELATION entry in the range table, except those that the planner
adds when expanding inheritance.

> It'd likely be better to rename ExecutorCheckPerms_hook,
> say to ExecCheckPermissions_hook given the rename of
> ExecCheckRTPerms. As it stands, it *looks* like the API
> of that hook has not changed, when it has. Better to
> break calling code visibly than to make people debug their
> way to an understanding that the List contents are no longer
> what they expected. A different idea could be to pass both
> the rangetable and relpermlist, again making the API break obvious
> (and who's to say a hook might not still want the rangetable?)

I agree it'd be better to break the API more explicitly. Actually, I
decided to adopt both of these suggestions: renamed the hook and kept
the rangeTable parameter.

> In parsenodes.h:
> + List *relpermlist; /* list of RTEPermissionInfo nodes for
> + * the RTE_RELATION entries in rtable */
>
> I find this comment not very future-proof, if indeed it's strictly
> correct even today. Maybe better "list of RelPermissionInfo nodes for
> rangetable entries having perminfoindex > 0". Likewise for the comment
> in RangeTableEntry: there's no compelling reason to assume that all and
> only RELATION RTEs will have RelPermissionInfo. Even if that remains
> true at parse time it's falsified during planning.

Ah right, inheritance children's RTE_RELATION entries don't have one.
I've fixed the comment.

> Also note typo in node name: that comment is the only reference to
> "RTEPermissionInfo" AFAICS. Although, given the redefinition I
> suggest above, arguably "RTEPermissionInfo" is the better name?

Agreed. I've renamed RelPermissionInfo to RTEPermissionInfo and
relpermlist to rtepermlist.

> I'm confused as to why RelPermissionInfo.inh exists. It doesn't
> seem to me that permissions checking should care about child rels.

I had to do this for contrib/sepgsql, sepgsql_dml_privileges() has this:

/*
* If this RangeTblEntry is also supposed to reference inherited
* tables, we need to check security label of the child tables. So, we
* expand rte->relid into list of OIDs of inheritance hierarchy, then
* checker routine will be invoked for each relations.
*/
if (!rte->inh)
tableIds = list_make1_oid(rte->relid);
else
tableIds = find_all_inheritors(rte->relid, NoLock, NULL);

> Why did you add checkAsUser to ForeignScan (and not any other scan
> plan nodes)? At best that's pretty asymmetric, but it seems mighty
> bogus: under what circumstances would an FDW need to know that but
> not any of the other RelPermissionInfo fields? This seems to
> indicate that someplace we should work harder at making the
> RelPermissionInfo list available to FDWs. (CustomScan providers
> might have similar issues, btw.)

I think I had tried doing what you are suggesting -- getting the
checkAsUser from a RelPermissionInfo rather than putting that in
ForeignScan -- though we can't do it, because we need the userid for
child foreign table relations, for which we don't create a
RelPermissionInfo. ForeignScan nodes for child relations don't store
their root parent's RT index, so we can't get the checkAsUser using
the root parent's RelPermissionInfo, like I could do for child foreign
table "result" relations using ResultRelInfo.ri_RootResultRelInfo.

As to why an FDW may not need to know any of the other
RelPermissionInfo fields, IIUC, ExecCheckPermissions() would have done
everything that ought to be done *locally* using that information.
Whatever the remote side needs to know wrt access permission checking
should have been put in fdw_private, no?

On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> ... One more thing: maybe we should rethink where to put
> extraUpdatedCols. Between the facts that it's not used for
> actual permissions checks, and that it's calculated by the
> rewriter not parser, it doesn't seem like it really belongs
> in RelPermissionInfo. Should we keep it in RangeTblEntry?
> Should it go somewhere else entirely? I'm just speculating,
> but now is a good time to think about it.

Indeed, extraUpdatedCols doesn't really seem to belong in
RelPermissionInfo, so I have left it in RangeTblEntry.

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