Re: Explain buffers wrong counter with parallel plans

On Wed, Aug 1, 2018 at 7:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jul 28, 2018 at 2:14 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> We have done verification that the approach works and fixes the bug in
>> all known cases. Do you see any problem with this approach?
>
> Regarding the change to execParallel.c, I think if you're going to
> move that code you should add a comment explaining the justification
> for the new positioning, like:
>
> /*
> * Prepare to track buffer usage during query execution.
> *
> * We do this after starting up the executor to match what happens in the
> * leader, which also doesn't count buffer accesses that occur during
> * executor startup.
> */
>
> If you just make a change and move the code without adding a comment
> explaining why you moved it, the next person will wonder why they
> shouldn't just move it back when they hit some other problem. I also
> agree with Andres that this could be committed separately.
>
> Regarding the change to execProcnode.c, "atleast" should be "at
> least". But more broadly, I don't think the comment is very clear
> about the justification for what we're doing, and Andres is right that
> duplication the comment isn't helpful. I've attempted to improve this
> in the attached version.
>
> Regarding Andres's comments about the changes to nodeLimit.c, I think
> he's right, but as you say, this isn't the first place to have this
> problem. The issue is that someone might be using cursor operations
> to fetch forward through the plan one tuple at a time, and then after
> triggering the shutdown, they might start fetching backward. That
> doesn't matter so long as shutdown callbacks are only used for
> shutting down stuff related to parallel query, because you can't fetch
> backwards from a Gather node or anything under it.
>

Yeah and at the first place, we won't allow parallel query for
cursors. The CURSOR_OPT_PARALLEL_OK won't be set for them and
standard_planner will prohibit generating parallel plans.

> But if we want to
> make broader use of shutdown callbacks -- and I think that would be a
> good idea -- then it's a problem. There's already a latent bug here,
> because custom scans and foreign scans are allowed to have shutdown
> callbacks (but postgres_fdw and file_fdw don't).
>
> Fixing that seems like a separate issue, and I think it would probably
> be OK to proceed with the change as you have it for now, but we ought
> to do something about it. I'm not sure there's a problem with
> rewinding, as Andres suggests: if the node is entirely rescanned, I
> believe it's allowed to regenerate the output, and Gather (Merge) can
> do that by firing up a new set of workers. But scanning backwards is
> a problem. I'm not exactly sure what the best way of handling that
> is, but one thing I think might work is to save ExecutePlan's
> execute_once flag in the EState and then make the call in nodeLimit.c
> and the one in ExecutePlan itself conditional on that flag. If we
> know that the plan is only going to be executed once, then there can
> never be any backward fetches and it's fine to shut down as soon as we
> finish going forward.
>

I think something on the lines what Tom and you are suggesting can be
done with the help of EXEC_FLAG_BACKWARD, but I don't see the need to
do anything for this patch. The change in nodeLimit.c is any way for
forward scans, so there shouldn't be any need for any other check.

If we agree that the current patch is sufficient for this bug fix,
then I can work on splitting the patch and preparing the patch for
back-branches. As mentioned above, I think we should backpatch this
till 9.6. Any objections on backpatching this fix?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com