On Tue, Feb 23, 2021 at 12:33 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Feb 22, 2021 at 8:41 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > On Fri, Feb 19, 2021 at 9:38 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Thu, Feb 18, 2021 at 11:05 AM Amit Langote <amitlangot...@gmail.com> > > > wrote: > > > > > > > > > > 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. > > > > > > > > > > Greg has point here but I feel something on previous lines (having a > > > test of glob->parallelModeNeeded) is better. We only want to > > > invalidate the plan if the prepared plan is unsafe to execute next > > > time. It is quite possible that there are unsafe triggers on different > > > partitions and only one of them is dropped, so next time planning will > > > again yield to the same non-parallel plan. If we agree with that I > > > think it is better to add this dependency in set_plan_refs (along with > > > Gather node handling). > > > > > > > I think we should try to be consistent with current plan-cache > > functionality, rather than possibly inventing new rules for > > partitions. > > I'm finding that with current Postgres functionality (master branch), > > if there are two parallel-unsafe triggers defined on a normal table > > and one is dropped, it results in replanning and it yields the same > > (non-parallel) plan. > > > > Does such a plan have partitions access in it? Can you share the plan? >
Er, no (it's just a regular table), but that was exactly my point: aren't you suggesting functionality for partitions that doesn't seem to work the same way for non-partitions? Regards, Greg Nancarrow Fujitsu Australia
