On Thu, Feb 18, 2021 at 10:03 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > On Thu, Feb 18, 2021 at 12:34 AM Amit Langote <amitlangot...@gmail.com> wrote: > > All that is to say that we should move our code to add partition OIDs > > as plan dependencies to somewhere in set_plan_references(), which > > otherwise populates PlannedStmt.relationOids. I updated the patch to > > do that. > > OK, understood. Thanks for the detailed explanation. > > > It also occurred to me that we can avoid pointless adding of > > partitions if the final plan won't use parallelism. For that, the > > patch adds checking glob->parallelModeNeeded, which seems to do the > > trick though I don't know if that's the correct way of doing that. > > > > I'm not sure if's pointless adding partitions even in the case of NOT > using parallelism, because we may be relying on the result of > parallel-safety checks on partitions in both cases. > For example, insert_parallel.sql currently includes a test (that you > originally provided in a previous post) that checks a non-parallel > plan is generated after a parallel-unsafe trigger is created on a > partition involved in the INSERT. > If I further add to that test by then dropping that trigger and then > again using EXPLAIN to see what plan is generated, then I'd expect a > parallel-plan to be generated, but with the setrefs-v3.patch it still > generates a non-parallel plan. So I think the "&& > glob->parallelModeNeeded" part of test needs to be removed.
Ah, okay, I didn't retest my case after making that change. Looking at this again, I am a bit concerned about going over the whole partition tree *twice* when making a parallel plan for insert into partitioned tables. Maybe we should do what you did in your first attempt a slightly differently -- add partition OIDs during the max_parallel_hazard() initiated scan of the partition tree as you did. Instead of adding them directly to PlannerGlobal.relationOids, add to, say, PlannerInfo.targetPartitionOids and have set_plan_references() do list_concat(glob->relationOids, list_copy(root->targetPartitionOids) in the same place as setrefs-v3 does add_target_partition_oids_recurse(). Thoughts? -- Amit Langote EDB: http://www.enterprisedb.com
