On Fri, Feb 19, 2021 at 7: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).
Are you saying that partitions shouldn't be added to the dependency list if a parallel plan was not chosen for insert into a partitioned table for whatever reason (parallel unsafe expressions or beaten by other paths in terms of cost)? If so, I am inclined to agree with that. I may be wrong but it doesn't seem to me that the possibility of constructing a better plan due to a given change is enough reason for plancache.c to invalidate plans that depend on that change. AIUI, plancache.c only considers a change interesting if it would *break* a Query or a plan. So in this case, a non-parallel plan may be slower, but it isn't exactly rendered *wrong* by changes that make a parallel plan possible. > Also, if we agree that we don't have any cheap way to determine > parallel-safety of partitioned relations then shall we consider the > patch being discussed [1] to be combined here? Yes, I think it does make sense to consider the GUC patch with the patches on this thread. > I feel we should focus on getting the first patch of Greg > (v18-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT, along with > a test case patch) and Hou-San's patch discussed at [1] ready. Then we > can focus on the > v18-0003-Enable-parallel-INSERT-and-or-SELECT-for-INSERT-INTO. Because > even if we get the first patch that is good enough for some users. +1. -- Amit Langote EDB: http://www.enterprisedb.com
