Re: ExecRTCheckPerms() and many prunable partitions - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: ExecRTCheckPerms() and many prunable partitions |
Date | |
Msg-id | CA+HiwqEz4=cjQAYNu1_KU0uOJzaTbunTPjypFCMGNuSYTgCRuA@mail.gmail.com Whole thread Raw |
In response to | Re: ExecRTCheckPerms() and many prunable partitions (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: ExecRTCheckPerms() and many prunable partitions
|
List | pgsql-hackers |
On Thu, Jul 28, 2022 at 6:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Langote <amitlangote09@gmail.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@sss.pgh.pa.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
Attachment
pgsql-hackers by date: