public inbox for [email protected]  

help / color / mirror / Atom feed

From: Kyotaro Horiguchi <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
Date: Wed, 12 Apr 2023 12:00:53 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

At Wed, 12 Apr 2023 03:36:01 +0900, Fujii Masao <[email protected]> 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

view thread (3+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox