public inbox for [email protected]
help / color / mirror / Atom feedFrom: Fujii Masao <[email protected]>
To: Jelte Fennema <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
Date: Wed, 26 Apr 2023 01:49:28 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAGECzQThFfOSf_J87pXO1oLY5=45A0deC=x0LsrpWMhO7GSs4w@mail.gmail.com>
References: <CAPmGK15YigmZKzVBGYPdmo-QMHBdhNupK8L6kUBdUtMERkbXsQ@mail.gmail.com>
<[email protected]>
<CAPmGK143nF7BQW0cFv3tpae7FrdYQfKH7eUE+CgwG5c6QGkZ=g@mail.gmail.com>
<[email protected]>
<CAGECzQThFfOSf_J87pXO1oLY5=45A0deC=x0LsrpWMhO7GSs4w@mail.gmail.com>
On 2023/04/21 16:39, Jelte Fennema wrote:
>> In my opinion, PQconnectPoll and PQgetCancel should use the same
>> parsing function or PQconnectPoll should set parsed values, making
>> unnecessary for PQgetCancel to parse the same parameter
>> again.
>
> Yes, I totally agree. So I think patch 0002 looks fine.
It seems like we have reached a consensus to push the 0002 patch.
As for back-patching, although the issue it fixes is trivial,
it may be a good idea to back-patch to v12 where parse_int_param()
was added, for easier back-patching in the future. Therefore
I'm thinking to push the 0002 patch at first and back-patch to v12.
>> Additionally, PQgetCancel should set appropriate error messages
>> for all failure modes.
>
> I don't think that PQgetCancel should ever set error messages on the
> provided conn object though. It's not part of the documented API and
> it's quite confusing since there's actually no error on the connection
> itself. That this happens for the keepalive parameter was an
> unintended sideeffect of 5987feb70b combined with the fact that the
> parsing is different. All those parsing functions should never error,
> because setting up the connection should already have checked them.
>
> So I think the newly added libpq_append_conn_error calls in patch 0001
> should be removed. The AF_UNIX check and the new WARNING in pg_fdw
> seem fine though.
Sounds reasonable to me.
Regarding the WARNING message, another idea is to pass the return value
of PQgetCancel() directly to PQcancel() as follows. If NULL is passed,
PQcancel() will detect it and set the proper error message to errbuf.
Then the warning message "WARNING: could not send cancel request:
PQcancel() -- no cancel object supplied" is output. This approach is
similar to how dblink_cancel_query() does. Thought?
----------------
cancel = PQgetCancel(conn);
if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
{
ereport(WARNING,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not send cancel request: %s",
errbuf)));
PQfreeCancel(cancel);
return false;
}
----------------
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
view thread (7+ 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], [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