Subject: Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 12 Apr 2023 12:00:53 +0900 (JST)

At Wed, 12 Apr 2023 03:36:01 +0900, Fujii Masao <[email-masked]> wrote in 
> Attached patch fixes this issue. It ensures that postgres_fdw only
> waits
> for a reply if a cancel request is actually issued. Additionally,
> it improves PQgetCancel() to set error messages in certain error
> cases,
> such as when out of memory occurs and malloc() fails. Moreover,
> it enhances postgres_fdw to report a warning message when
> PQgetCancel()
> returns NULL, explaining the reason for the NULL value.
> 
> Thought?

I wondered why the connection didn't fail in the first place. After
digging into it, I found (or remembered) that local (or AF_UNIX)
connections ignore the timeout value at making a connection. I think
the real issue here is that PGgetCancel is unnecessarily checking its
value and failing as a result. Otherwise there would be no room for
failure in the call to PQgetCancel() at that point in the example
case.

PQconnectPoll should remove the ignored parameters at connection or
PQgetCancel should ingore the unreferenced (or unchecked)
parameters. For example, the below diff takes the latter way and seems
working (for at least AF_UNIX connections)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 40fef0e2c8..30e2ab54ba 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4718,6 +4718,10 @@ PQgetCancel(PGconn *conn)
        cancel->keepalives_idle = -1;
        cancel->keepalives_interval = -1;
        cancel->keepalives_count = -1;
+
+       if (conn->connip == 0)
+               return cancel;
+
        if (conn->pgtcp_user_timeout != NULL)
        {
                if (!parse_int_param(conn->pgtcp_user_timeout,

Of course, it's not great that pgfdw_cancel_query_begin() ignores the
result from PQgetCancel(), but I think we don't need another ereport.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Fujii Masao <[email-masked]>
Date: Wed, 12 Apr 2023 23:39:29 +0900

> connections ignore the timeout value at making a connection. I think

BTW, you can reproduce the issue even when using a TCP connection
instead of a Unix domain socket by specifying a very large number
in the "keepalives" connection parameter for the foreign server.
Here is an example:

-----------------
create server loopback foreign data wrapper postgres_fdw options (host '127.0.0.1', port '5432', keepalives '99999999999');
-----------------

The reason behind this issue is that PQconnectPoll() parses
the "keepalives" parameter value by simply using strtol(),
while PQgetCancel() uses parse_int_param(). To fix this issue,
it might be better to update PQconnectPoll() so that it uses
parse_int_param() for parsing the "keepalives" parameter.

> working (for at least AF_UNIX connections)

To clarify, are you suggesting that PQgetCancel() should
only parse the parameters for TCP connections
if cancel->raddr.addr.ss_family != AF_UNIX?
If so, I think that's a good idea.

> result from PQgetCancel(), but I think we don't need another ereport.

Can you please clarify why you suggest avoiding outputting
the warning message when PQgetCancel() returns NULL?
I think it is important to inform the user when an error
occurs and a cancel request cannot be sent, as this information
can help them identify the cause of the problem (such as
setting an overly large value for the keepalives parameter).

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION