On Mon, Aug 28, 2017 at 6:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> On Mon, Aug 28, 2017 at 6:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Um, what's different about that than before?
>
>> Earlier, we perform the rescan of all the nodes before ExecProcNode,
>> so workers will always start (restart) after the scan descriptor is
>> initialized.
>
> If what you're complaining about is that I put back the "if
> (outerPlan->chgParam == NULL)" test to allow postponement of the
> recursive ExecReScan call, I'm afraid that it's mere wishful
> thinking that omitting that test in nodeGather did anything.
> The nodes underneath the Gather are likely to do the same thing,
> so that the parallel table scan node itself is going to get a
> postponed rescan call anyway. See e.g. ExecReScanNestLoop().
>
Previously outerPlan->chgParam will be NULL, so I think rescan's won't
be postponed. IIRC, I have debugged it during the development of this
code that rescans were not postponed. I don't deny that for some
cases it might get delayed but for simple cases, it was done
immediately. I agree that in general, the proposed fix is better than
having nothing, but not sure if we need it for Gather as well
considering we are not able to demonstrate any case.
> I see your point that there's inadequate interlocking between resetting
> the parallel scan's shared state and starting a fresh set of workers,
> but that's a pre-existing bug. In practice I doubt it makes any
> difference, because according to my testing the leader will generally
> reach the table scan long before any workers do. It'd be nice to do
> better though.
>
Agreed. BTW, I have mentioned above that we can avoid skipping
optimization in rescan path if we are in parallel mode. I think that
will not be as elegant a solution as your patch, but it won't have
this problem.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com