Re: ExecRTCheckPerms() and many prunable partitions

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(at)alvh(dot)no-ip(dot)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 ;).