Hi, Last year in [1], I had briefly mentioned $subject. I'm starting this thread to propose a small patch to alleviate the inefficiency of that case.
As also mentioned in [1], when running -Mprepared benchmarks
(plan_cache_mode = force_generic_plan) using partitioned tables,
ExecRTCheckPerms() tends to show up in the profile, especially with
large partition counts. Granted it's lurking behind
AcquireExecutorLocks(), LockReleaseAll() et al, but still seems like a
problem we should do something about.
The problem is that it loops over the entire range table even though
only one or handful of those entries actually need their permissions
checked. Most entries, especially those of partition child tables
have their requiredPerms set to 0, which David pointed out to me in
[2], so what ExecCheckRTPerms() does in their case is pure overhead.
An idea to fix that is to store the RT indexes of the entries that
have non-0 requiredPerms into a separate list or a bitmapset in
PlannedStmt. I thought of two implementation ideas for how to set
that:
1. Put add_rtes_to_flat_rtable() in the charge of populating it:
@@ -324,12 +324,18 @@ add_rtes_to_flat_rtable(PlannerInfo *root, bool recursing)
* flattened rangetable match up with their original indexes. When
* recursing, we only care about extracting relation RTEs.
*/
+ rti = 1;
foreach(lc, root->parse->rtable)
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
if (!recursing || rte->rtekind == RTE_RELATION)
+ {
add_rte_to_flat_rtable(glob, rte);
+ if (rte->requiredPerms != 0)
+ glob->checkPermRels = bms_add_member(glob->checkPermRels, rti);
+ }
+ rti++
}
2. Start populating checkPermRels in ParseState (parse_relation.c),
passing it along in Query through the rewriter and finally the
planner.
1 seems very simple, but appears to add overhead to what is likely a
oft-taken path. Also, the newly added code would have to run as many
times as there are partitions, which sounds like a dealbreaker to me.
2 can seem a bit complex. Given that the set is tracked in Query,
special care is needed to handle views and subqueries correctly,
because those features involve intricate manipulation of Query nodes
and their range tables. However, most of that special care code
remains out of the busy paths. Also, none of that code touches
partition/child RTEs, so unaffected by how many of them there are.
For now, I have implemented the idea 2 as the attached patch. While
it passes make check-world, I am not fully confident yet that it
correctly handles all the cases involving views and subqueries.
So while still kind of PoC, will add this to July CF for keeping track.
--
Amit Langote
EDB: http://www.enterprisedb.com
[1]
https://www.postgresql.org/message-id/ca+hiwqg7zrubmmih3wpsbz4s0h2ehywrnxeducky5hr3fwz...@mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAApHDvqPzsMcKLRpmNpUW97PmaQDTmD7b2BayEPS5AN4LY-0bA%40mail.gmail.com
0001-Explicitly-track-RT-indexes-of-relations-to-check-pe.patch
Description: Binary data
