On Fri, Nov 19, 2021 at 9:14 PM Fujii Masao <masao.fu...@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.