On Thu, Aug 2, 2018 at 8:38 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2018-08-02 08:21:58 +0530, Amit Kapila wrote:
>> 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.
>
> I think this is almost a guarantee to introduce bugs in the future. And
> besides that, as Robert points out, it's essentially an exiting bug for
> custom scans. Given that EXEC_FLAG_BACKWARD already exists, why not do
> the right thing here?
>
Sure, if we want we can do it in this patch, but this is not the
problem of this patch. It is also related to existing usage of
shutdown callbacks. I think we can use es_top_eflags in Estate to
detect it and then call shutdown only if EXEC_FLAG_BACKWARD is not
set. I think we should do that as a separate patch either before or
after this patch rather than conflating that change into this patch.
IIUC, then Robert also said that we should fix that separately. I
will do as per whatever consensus we reach here.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
