public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH 6/6] 0003 review
7+ messages / 6 participants
[nested] [flat]

* [PATCH 6/6] 0003 review
@ 2021-02-15 15:36  Tomas Vondra <[email protected]>
  0 siblings, 0 replies; 7+ messages in thread

From: Tomas Vondra @ 2021-02-15 15:36 UTC (permalink / raw)

---
 src/test/regress/input/copy.source  | 4 ++--
 src/test/regress/output/copy.source | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source
index ddde33e7cc..4ee1b26326 100644
--- a/src/test/regress/input/copy.source
+++ b/src/test/regress/input/copy.source
@@ -204,7 +204,7 @@ drop table parted_copytest;
 
 --
 -- progress reporting
--- 
+--
 
 -- setup
 -- reuse employer datatype, that has a small sized data set
@@ -221,7 +221,7 @@ create function notice_after_progress_reporting() returns trigger AS
 $$
 declare report record;
 begin
-	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to 
+	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to
 	-- variance in system process ids, and concurrency in runs of tests.
 	-- Additionally, due to the usage of this test in pg_regress, the 'datid'
 	-- also is not consistent between runs.
diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source
index 60f4206aa1..8ebfb7ae81 100644
--- a/src/test/regress/output/copy.source
+++ b/src/test/regress/output/copy.source
@@ -167,7 +167,7 @@ select * from parted_copytest where b = 2;
 drop table parted_copytest;
 --
 -- progress reporting
--- 
+--
 -- setup
 -- reuse employer datatype, that has a small sized data set
 create table progress_reporting (
@@ -181,7 +181,7 @@ create function notice_after_progress_reporting() returns trigger AS
 $$
 declare report record;
 begin
-	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to 
+	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to
 	-- variance in system process ids, and concurrency in runs of tests.
 	-- Additionally, due to the usage of this test in pg_regress, the 'datid'
 	-- also is not consistent between runs.
-- 
2.26.2


--------------DA8D60BF026F7ACE69A9AEE5--





^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
@ 2023-04-14 14:28  Fujii Masao <[email protected]>
  0 siblings, 1 reply; 7+ messages in thread

From: Fujii Masao @ 2023-04-14 14:28 UTC (permalink / raw)
  To: Etsuro Fujita <[email protected]>; +Cc: [email protected]



On 2023/04/14 18:59, Etsuro Fujita wrote:
>> The primary message basically should avoid reference to implementation details such as specific structure names like PGcancel, shouldn't it, as per the error message style guide?
> 
> I do not think that PGcancel is that specific, as it is described in
> the user-facing documentation [1].  (In addition, the error message I
> proposed was created by copying the existing error message "could not
> create OpenSSL BIO structure" in contrib/sslinfo.c.)

I think that mentioning PGcancel in the error message could be confusing for average users who are just running a query on a foreign table and encounter the error message after pressing Ctrl-C. They may not understand why the PGcancel struct is referenced in the error message while accessing foreign tables. It could be viewed as an internal detail that is not necessary for the user to know.


>> Although the primary message is the same, the supplemental message provides additional context that can help distinguish which function is reporting 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 invalid integer value "99999999999" was specified in the "keepalives" connection option, which caused the warning message. Then, they would need to check the setting of the "keepalives" option and correct it if necessary.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION






^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
@ 2023-04-17 06:21  Etsuro Fujita <[email protected]>
  parent: Fujii Masao <[email protected]>
  0 siblings, 1 reply; 7+ messages in thread

From: Etsuro Fujita @ 2023-04-17 06:21 UTC (permalink / raw)
  To: Fujii Masao <[email protected]>; +Cc: [email protected]

On Fri, Apr 14, 2023 at 11:28 PM Fujii Masao
<[email protected]> wrote:
> On 2023/04/14 18:59, Etsuro Fujita wrote:
> >> The primary message basically should avoid reference to implementation details such as specific structure names like PGcancel, shouldn't it, as per the error message style guide?
> >
> > I do not think that PGcancel is that specific, as it is described in
> > the user-facing documentation [1].  (In addition, the error message I
> > proposed was created by copying the existing error message "could not
> > create OpenSSL BIO structure" in contrib/sslinfo.c.)
>
> I think that mentioning PGcancel in the error message could be confusing for average users who are just running a query on a foreign table and encounter the error message after pressing Ctrl-C. They may not understand why the PGcancel struct is referenced in the error message while accessing foreign tables. It could be viewed as an internal detail that is not necessary for the user to know.

Ok, understood.  I do not think it is wrong to use "could not send
cancel request” for PQgetCancel as well, but I feel that that is not
perfect for PQgetCancel, because that function never sends a cancel
request; that function just initializes the request.  So how about
"could not initialize cancel request”, instead?

> >> Although the primary message is the same, the supplemental message provides additional context that can help distinguish which function is reporting 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 invalid integer value "99999999999" was specified in the "keepalives" connection option, which caused the warning message. Then, they would need to check 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 ”could not send cancel request”, the user can 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”.

(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.)

Best regards,
Etsuro Fujita






^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
@ 2023-04-17 08:38  Kyotaro Horiguchi <[email protected]>
  parent: Etsuro Fujita <[email protected]>
  0 siblings, 1 reply; 7+ messages in thread

From: Kyotaro Horiguchi @ 2023-04-17 08:38 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; [email protected]

At Mon, 17 Apr 2023 15:21:02 +0900, Etsuro Fujita <[email protected]> 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






^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
@ 2023-04-21 07:39  Jelte Fennema <[email protected]>
  parent: Kyotaro Horiguchi <[email protected]>
  0 siblings, 1 reply; 7+ messages in thread

From: Jelte Fennema @ 2023-04-21 07:39 UTC (permalink / raw)
  To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]

> 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.

> 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. It would probably make sense to have them be
separate patches though, because they are pretty unrelated.






^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
@ 2023-04-25 16:49  Fujii Masao <[email protected]>
  parent: Jelte Fennema <[email protected]>
  0 siblings, 1 reply; 7+ messages in thread

From: Fujii Masao @ 2023-04-25 16:49 UTC (permalink / raw)
  To: Jelte Fennema <[email protected]>; Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]



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






^ permalink  raw  reply  [nested|flat] 7+ messages in thread

* Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
@ 2023-07-27 11:01  Kuwamura Masaki <[email protected]>
  parent: Fujii Masao <[email protected]>
  0 siblings, 0 replies; 7+ messages in thread

From: Kuwamura Masaki @ 2023-07-27 11:01 UTC (permalink / raw)
  To: Fujii Masao <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]

Hi, Fujii-san

> 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.

I agree to go with this.

With this approach, the information behind the error (e.g., "out of
memory") will disappear, I guess.
I think we have to deal with it eventually. (I'm sorry, I don't have a good
idea right now)
However, the original issue is unnecessary waiting, and this should be
fixed soon.
So it is better to fix the problem this way and discuss retaining
information in another patch IMO.

I'm afraid I'm new to reviewing.
If I'm misunderstanding something, please let me know.

Masaki Kuwamura


^ permalink  raw  reply  [nested|flat] 7+ messages in thread

end of thread, other threads:[~2023-07-27 11:01 UTC | newest]

Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-15 15:36 [PATCH 6/6] 0003 review Tomas Vondra <[email protected]>
2023-04-14 14:28 Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply Fujii Masao <[email protected]>
2023-04-17 06:21 ` Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply Etsuro Fujita <[email protected]>
2023-04-17 08:38   ` Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply Kyotaro Horiguchi <[email protected]>
2023-04-21 07:39     ` Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply Jelte Fennema <[email protected]>
2023-04-25 16:49       ` Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply Fujii Masao <[email protected]>
2023-07-27 11:01         ` Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply Kuwamura Masaki <[email protected]>

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