Date: Mon, 17 Apr 2023 17:38:12 +0900 (JST)
Message-Id: <20230417.173812.452521384661813597.horikyota.ntt@gmail.com>
To: etsuro.fujita@gmail.com
Cc: masao.fujii@oss.nttdata.com, 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: <CAPmGK143nF7BQW0cFv3tpae7FrdYQfKH7eUE+CgwG5c6QGkZ=g@mail.gmail.com>
References: <CAPmGK15YigmZKzVBGYPdmo-QMHBdhNupK8L6kUBdUtMERkbXsQ@mail.gmail.com>
	<67cad2e7-f9c1-5eb0-a0be-99fd78571906@oss.nttdata.com>
	<CAPmGK143nF7BQW0cFv3tpae7FrdYQfKH7eUE+CgwG5c6QGkZ=g@mail.gmail.com>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230417.173812.452521384661813597.horikyota.ntt%40gmail.com>
Precedence: bulk

At Mon, 17 Apr 2023 15:21:02 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in 
> > >> Although the primary message is the same, the supplemental message pro=
> vides additional context that can help distinguish which function is report=
> ing the message.
> > >
> > > If the user is familiar with the PQgetCancel/PQcancel internals, this
> > > is true, but if not, I do not think this is always true.  Consider
> > > this error message, for example:
> > >
> > > 2023-04-14 17:48:55.862 JST [24344] WARNING:  could not send cancel
> > > request: invalid integer value "99999999999" for connection option
> > > "keepalives"
> > >
> > > It would be hard for users without the knowledge about those internals
> > > to distinguish that from this message.  For average users, I think it
> > > would be good to use a more distinguishable error message.
> >
> > In this case, I believe that they should be able to understand that an in=
> valid integer value "99999999999" was specified in the "keepalives" connect=
> ion option, which caused the warning message. Then, they would need to chec=
> k the setting of the "keepalives" option and correct it if necessary.
> 
> Maybe my explanation was not clear.  Let me explain.  Assume that a
> user want to identify the place where the above error was thrown.
> Using grep with =E2=80=9Dcould not send cancel request=E2=80=9D, the user c=
> an find the
> two places emitting the message in pgfdw_cancel_query_begin: one for
> PQgetCancel and one for PQcancel.  If the user are familiar with the
> PQgetCancel/PQcancel internals, the user can determine, from the
> supplemental message, that the error was thrown by the former.  But if
> not, the user cannot do so.  To support the unfamiliar user as well, I
> think it would be a good idea to use a more appropriate message for
> PQgetCancel that is different from "could not send cancel request=E2=80=9D.
> 
> (I agree that most users would not care about the places where errors
> were thrown, but I think some users would, and actually, I do when
> investigating unfamiliar errors.)

If PGgetCancel() fails due to invalid keepliave-related values, It
seems like a bug that needs fixing, regardless of whether we display
an error message when PGgetCacncel() fails.  The only error case of
PGgetCancel() that could occur in pgfdw_cancel_query_begin() is a
malloc() failure, which currently does not set an error message (I'm
not sure we can do that in that case, though..).

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. Additionally, PQgetCancel should set appropriate error messages
for all failure modes.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center