Message-ID: <53f03117-087f-5583-2c97-e82a3e9dc43e@oss.nttdata.com>
Date: Wed, 26 Apr 2023 01:49:28 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.0
Subject: Re: Issue in postgres_fdw causing unnecessary wait for cancel request
 reply
To: Jelte Fennema <postgres@jeltef.nl>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: etsuro.fujita@gmail.com, pgsql-hackers@lists.postgresql.org
References: <CAPmGK15YigmZKzVBGYPdmo-QMHBdhNupK8L6kUBdUtMERkbXsQ@mail.gmail.com>
 <67cad2e7-f9c1-5eb0-a0be-99fd78571906@oss.nttdata.com>
 <CAPmGK143nF7BQW0cFv3tpae7FrdYQfKH7eUE+CgwG5c6QGkZ=g@mail.gmail.com>
 <20230417.173812.452521384661813597.horikyota.ntt@gmail.com>
 <CAGECzQThFfOSf_J87pXO1oLY5=45A0deC=x0LsrpWMhO7GSs4w@mail.gmail.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <CAGECzQThFfOSf_J87pXO1oLY5=45A0deC=x0LsrpWMhO7GSs4w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/53f03117-087f-5583-2c97-e82a3e9dc43e%40oss.nttdata.com>
Precedence: bulk


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