On Thu, Aug 2, 2018 at 5:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I have created three patches (a) move InstrStartParallelQuery from its
> original location so that we perform it just before ExecutorRun (b)
> patch to fix the gather stats by calling shutdown at appropriate place
> and allow stats collection in ExecShutdownNode (c) Probit calling
> ExecShutdownNode if there is a possibility of backward scans (I have
> done some basic tests with this patch, if we decide to proceed with
> it, then some more verification and testing would be required).
>
> I think we should commit first two patches as that fixes the problem
> being discussed in this thread and then do some additional
> verification for the third patch (mentioned in option c). I can
> understand if people want to commit the third patch before the second
> patch, so let me know what you guys think.
I'm happy with the first two patches. In the third one, I don't think
"See ExecLimit" is a good thing to put a comment like this, because
it's too hard to find the comment to which it refers, and because
future commits are likely to edit or remove that comment without
noticing the references to it from elsewhere. Instead I would just
write, in all three places, /* If we know we won't need to back up, we
can release resources at this point. */ or something like that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company