Re: ExecRTCheckPerms() and many prunable partitions - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: ExecRTCheckPerms() and many prunable partitions |
Date | |
Msg-id | CA+HiwqGFCs2uq7VRKi7g+FFKbP6Ea_2_HkgZb2HPhUfaAKT3ng@mail.gmail.com Whole thread Raw |
In response to | Re: ExecRTCheckPerms() and many prunable partitions (Alvaro Herrera <alvherre@alvh.no-ip.org>) |
Responses |
Re: ExecRTCheckPerms() and many prunable partitions
Re: ExecRTCheckPerms() and many prunable partitions Re: ExecRTCheckPerms() and many prunable partitions Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser) Re: ExecRTCheckPerms() and many prunable partitions (sqlsmith) |
List | pgsql-hackers |
Hi Alvaro, Thanks for taking a look and all the fixup patches. Was working on that test I said we should add and then was spending some time cleaning things up and breaking some things out into their patches, mainly for the ease of review. On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Thanks for the new version, in particular thank you for fixing the > annoyance with the CombineRangeTables API. OK, now that you seem to think that looks good, I've merged it into the main patch. > 0002 was already pushed upstream, so we can forget about it. I also > pushed the addition of missing_ok to build_attrmap_by_name{,_if_req}. Yeah, I thought that needed to be broken out and had done so in my local repo. Thanks for pushing that bit. > As for 0001+0003, here it is once again with a few fixups. There are > two nontrivial changes here: > > 1. in get_rel_all_updated_cols (née GetRelAllUpdatedCols, which I > changed because it didn't match the naming style in inherits.c) you were > doing determining the relid to use in a roundabout way, then asserting > it is a value you already know: > > - use_relid = rel->top_parent_relids == NULL ? rel->relid : > - bms_singleton_member(rel->top_parent_relids); > - Assert(use_relid == root->parse->resultRelation); > Why not just use root->parse->resultRelation in the first place? Facepalm, yes. > My 0002 does that. Merged. > 2. my 0005 moves a block in add_rte_to_flat_rtable one level out: > there's no need to put it inside the rtekind == RTE_RELATION block, and > the comment in that block failed to mention that we copied the > RTEPermissionInfo; we can just let it work on the 'perminfoindex > 0' > condition. Yes, agree that's better. > Also, the comment is a bit misleading, and I changed it > some, but maybe not sufficiently: after add_rte_to_flat_rtable, the same > RTEPermissionInfo node will serve two RTEs: one in the Query struct, > whose perminfoindex corresponds to Query->rtepermlist; and the other in > PlannerGlobal->finalrtable, whose index corresponds to > PlannerGlobal->finalrtepermlist. I was initially thinking that the old > RTE would result in a "corrupted" state, but that doesn't appear to be > the case. (Also: I made it grab the RTEPermissionInfo using > rte->perminfoindex, not newrte->perminfoindex, because that seems > slightly bogus, even if they are identical because of the memcpy.) Interesting point about two different RTEs (in different lists) pointing to the same RTEPermissionInfo, also in different lists. Maybe, we should have the following there so that the PlannedStmt's contents don't point into the Query? newperminfo = copyObject(perminfo); > The other changes are cosmetic. Thanks, I've merged all. I do wonder that it is only in PlannedStmt that the list is called something that is not "rtepermlist", but I'm fine with it if you prefer that. > I do not include here your 0004 and 0005. (I think we can deal with > those separately later.) OK, I have not attached them with this email either. As I mentioned above, I've broken a couple of other changes out into their own patches that I've put before the main patch. 0001 adds ExecGetRootToChildMap(). I thought it would be better to write in the commit message why the new map is necessary for the main patch. 0002 contains changes that has to do with changing how we access checkAsUser in some foreign table planning/execution code sites. Thought it might be better to describe it separately too. 0003 is the main patch into which I've merged both my patch that invents CombineRangeTables() that I had posted separately before and all of your fixups. In it, you will see a new test case that I have added in rules.sql to exercise the permission checking order stuff that I had said I may have broken with this patch, especially the hunks that change rewriteRuleAction(). That test would be broken with v24, but not after the changes to add_rtes_to_flat_rtable() that I made to address your review comment that blindly list_concat'ing finalrtepermlist and Query's rtepermlist doesn't look very robust, which it indeed wasn't [1]. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] So, rewriteRuleAction(), with previous "wrong" versions of the patch (~v26), would combine the original query's and action query's rtepermlists in the "wrong" order, that is, not in the order in which RTEs appear in the combined rtable. But because add_rtes_to_flat_rtable() now (v26~) adds perminfos into finalrtepermlist in the RTE order using lappend(), that wrongness of rewriteRuleAction() would be masked -- no execution-time failure of the test. Anyway, I've also fixed rewriteRuleAction() to be "correct" in v27, so it is the least wrong version AFAIK ;).
Attachment
pgsql-hackers by date: