Re: ExecRTCheckPerms() and many prunable partitions

Amit Langote <amitlangote09@gmail.com> writes:
> [ v16 patches ]

I took a quick look at this ...

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.

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?)

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.

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?

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

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've not looked at much of the actual code, just the .h file changes.
Haven't studied 0002 either.

			regards, tom lane