Re: Shouldn't postgres_fdw report warning when it gives up getting result from foreign server?

On Fri, Nov 19, 2021 at 3:31 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> Hi,
>
> postgres_fdw reports no log message when it sends "ABORT TRANSACTION" etc
> and gives up getting a reply from a foreign server because of timeout or
> connection trouble. This makes the troubleshooting a bit harder when
> using postgres_fdw.
>
> So how about making postgres_fdw report a warning in that case?
> Specifically I'm thinking to change pgfdw_get_cleanup_result()
> in postgres_fdw/connection.c so that it reports a warning in case of
> a timeout or connection failure (error of PQconsumeInput()).

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.

/*
* If a command has been submitted to the remote server by using an
* asynchronous execution function, the command might not have yet
* completed. Check to see if a command is still being processed by the
* remote server, and if so, request cancellation of the command.
*/
if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE &&
!pgfdw_cancel_query(entry->conn))
return; /* Unable to cancel running query */

if (!pgfdw_exec_cleanup_query(entry->conn, sql, false))
return;

> BTW, pgfdw_get_cleanup_result() does almost the same things as
> what pgfdw_get_result() does. So it might be good idea to refactor
> those function to reduce the code duplication.

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.

Regards,
Bharath Rupireddy.