Date: Wed, 12 Apr 2023 12:00:53 +0900 (JST)
Message-Id: <20230412.120053.1851827772375524248.horikyota.ntt@gmail.com>
To: masao.fujii@oss.nttdata.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Issue in postgres_fdw causing unnecessary wait for cancel
 request reply
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <e587761c-eef3-1f83-bf16-6dadba0acaab@oss.nttdata.com>
References: <e587761c-eef3-1f83-bf16-6dadba0acaab@oss.nttdata.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230412.120053.1851827772375524248.horikyota.ntt%40gmail.com>
Precedence: bulk

At Wed, 12 Apr 2023 03:36:01 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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