Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)

On Tue, Jan 17, 2023 at 7:33 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2022-Dec-12, Amit Langote wrote:
> > On Sun, Dec 11, 2022 at 6:25 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > I've attached 0001 to remove those extraneous code blocks and add a
> > > comment mentioning that userid need not be recomputed.
> > >
> > > While staring at the build_simple_rel() bit mentioned above, I
> > > realized that this code fails to set userid correctly in the
> > > inheritance parent rels that are child relations of subquery parent
> > > relations, such as UNION ALL subqueries. In that case, instead of
> > > copying the userid (= 0) of the parent rel, the child should look up
> > > its own RTEPermissionInfo, which should be there, and use the
> > > checkAsUser value from there. I've attached 0002 to fix this hole. I
> > > am not sure whether there's a way to add a test case for this in the
> > > core suite.
> >
> > Ah, I realized we could just expand the test added by 553d2ec27 with a
> > wrapper view (to test checkAsUser functionality) and a UNION ALL query
> > over the view (to test this change).
>
> Hmm, but if I run this test without the code change in 0002, the test
> also passes (to wit: the plan still has two hash joins), so I understand
> that either you're testing something that's not changed by the patch,
> or the test case itself isn't really what you wanted.

Yeah, the test case is bogus. :-(.

It seems that, with the test as written, it's not the partitioned
table referenced in the view's query that becomes a child of the UNION
ALL parent subquery, but the subquery itself. The bug being fixed in
0002 doesn't affect the planning of this query at all, because child
subquery is planned independently of the main query involving UNION
ALL because of it being unable to be pushed up into the latter. We
want the partitioned table referenced in the child subquery to become
a child of the UNION ALL parent subquery for the bug to be relevant.

I tried rewriting the test such that the view's subquery does get
pulled up such that the partitioned table becomes a child of the UNION
ALL subquery. By attaching a debugger, I do see the bug affecting the
planning of this query, but still not in a way that changes the plan.
I will keep trying but in the meantime I'm attaching 0001 to show the
rewritten query and the plan.

> As for 0001, it seems simpler to me to put the 'userid' variable in the
> same scope as 'onerel', and then compute it just once and don't bother
> with it at all afterwards, as in the attached.

That sounds better. Attached as 0002.

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