ExecRTCheckPerms() and many prunable partitions
Amit Langote <amitlangote09@gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-06-30T13:33:44Z
Lists: pgsql-hackers
Attachments
- 0001-Explicitly-track-RT-indexes-of-relations-to-check-pe.patch (application/octet-stream) patch 0001
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+HiwqG7ZruBmmih3wPsBZ4s0H2EhywrnXEduckY5Hr3fWzPWA@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAApHDvqPzsMcKLRpmNpUW97PmaQDTmD7b2BayEPS5AN4LY-0bA%40mail.gmail.com
Commits
-
Add a test case for a316a3bc
- 054ff3b33a85 17.0 landed
- 0a14bca662a4 16.0 landed
-
Correctly set userid of subquery relations' child rels
- a316a3bc6d3f 16.0 landed
-
Fix buggy recursion in flatten_rtes_walker().
- c7468c73f7b6 16.0 landed
-
Remove some dead code in selfuncs.c
- 438e6b724090 16.0 landed
-
Update outdated comment in ApplyRetrieveRule
- 29861e228a47 16.0 landed
-
Rework query relation permission checking
- a61b1f74823c 16.0 landed
-
Generalize ri_RootToPartitionMap to use for non-partition children
- fb958b5da86d 16.0 landed
-
Stop accessing checkAsUser via RTE in some cases
- 599b33b9492d 16.0 landed
-
Add 'missing_ok' argument to build_attrmap_by_name
- ad86d159b6ab 16.0 landed