On Fri, Nov 19, 2021 at 9:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/11/19 22:13, Bharath Rupireddy wrote:
> > How about adding the warning message in pgfdw_abort_cleanup instead of
> > pgfdw_get_cleanup_result?
> >
> > Just before this in pgfdw_abort_cleanup seems better to me.
>
> I was thinking pgfdw_get_cleanup_result() is better because it can
> easily report different warning messages based on cases of a timeout
> or connection failure, respectively. Since pgfdw_get_cleanup_result()
> returns true in both those cases, ISTM that it's not easy to
> distinguish them in pgfdw_abort_cleanup().
>
> Anyway, attached is the patch (pgfdw_get_cleanup_result_v1.patch)
> that makes pgfdw_get_cleanup_result() report a warning message.
It reports "remote SQL command: (cancel request)" which isn't a sql
query, but it looks okay to me as we report (cancel request). The
pgfdw_get_cleanup_result_v1 patch LGTM.
> > Yeah, this seems to be an opportunity. But, the function should deal
> > with the timeout separately, I'm concerned that the function will
> > eventually be having if (timeout_param_specified) { } else { } sort
> > of code. We can see how much duplicate code we save here vs the
> > readability or complexity that comes with the single function.
>
> Please see the attached patch (refactor_pgfdw_get_result_v1.patch).
> This is still WIP, but you can check how much the refactoring can
> simplify the code.
I think we can discuss this refactoring patch separately. Thoughts?
Regards,
Bharath Rupireddy.
