On 2023/04/13 11:00, Kyotaro Horiguchi wrote:
Agreed, it seems to be a leftover when we moved to parse_int_param()
in that area.
It looks like there was an oversight in commit e7a2217978. I've attached a
patch (0002) that updates PQconnectPoll() to use parse_int_param() for parsing
the keepalives parameter.
As this change is not directly related to the bug fix, it may not be necessary
to back-patch it to the stable versions, I think. Thought?
To clarify, are you suggesting that PQgetCancel() should
only parse the parameters for TCP connections
if cancel->raddr.addr.ss_family != AF_UNIX?
If so, I think that's a good idea.
You're right. I used connip in the diff because I thought it provided
the same condition, but in a simpler way.
I made a modification to the 0001 patch. It will now allow PQgetCancel() to
parse and interpret TCP connection parameters only when the connection is not
made through a Unix-domain socket.
However, I notcied that PQgetCancel() doesn't set errbuf.. So, I'm
fine with your proposal.
Ok.
I think it is important to inform the user when an error
occurs and a cancel request cannot be sent, as this information
can help them identify the cause of the problem (such as
setting an overly large value for the keepalives parameter).
Although I view it as an internal error, I agree with emitting some
error messages in that situation.
Ok.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From c46e1b06e68cf761bacbfe5b6d35ccf5fcbbb39d Mon Sep 17 00:00:00 2001
From: Fujii Masao <fu...@postgresql.org>
Date: Wed, 12 Apr 2023 02:17:56 +0900
Subject: [PATCH v2 1/2] postgres_fdw: Fix bug causing unnecessary wait for
cancel request reply.
In the event of a local transaction being aborted while a query is
running on a remote server in postgres_fdw, the extension sends
a cancel request to the remote server. However, if PQgetCancel()
returned NULL and no cancel request was issued, postgres_fdw
could still wait for the reply to the cancel request, causing unnecessary
wait time with a 30 second timeout.
To fix this issue, this commit ensures that postgres_fdw only waits for
a reply if a cancel request was actually issued. Additionally,
this commit improves PQgetCancel() to set error messages in certain
error cases, such as when out of memory happens and malloc() fails.
Moreover, this commit enhances postgres_fdw to report a warning
message when PQgetCancel() returns NULL, explaining the reason for
the NULL value.
---
contrib/postgres_fdw/connection.c | 8 +++
src/interfaces/libpq/fe-connect.c | 81 ++++++++++++++++++-------------
2 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 75d93d6ead..c8bebe7b14 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1384,6 +1384,14 @@ pgfdw_cancel_query_begin(PGconn *conn)
}
PQfreeCancel(cancel);
}
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not send cancel request: %s",
+ pchomp(PQerrorMessage(conn)))));
+ return false;
+ }
return true;
}
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index fcd3d0d9a3..6558d098b5 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4709,11 +4709,17 @@ PQgetCancel(PGconn *conn)
return NULL;
if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(conn, "connection is not open");
return NULL;
+ }
cancel = malloc(sizeof(PGcancel));
if (cancel == NULL)
+ {
+ libpq_append_conn_error(conn, "out of memory");
return NULL;
+ }
memcpy(&cancel->raddr, &conn->raddr, sizeof(SockAddr));
cancel->be_pid = conn->be_pid;
@@ -4724,40 +4730,49 @@ PQgetCancel(PGconn *conn)
cancel->keepalives_idle = -1;
cancel->keepalives_interval = -1;
cancel->keepalives_count = -1;
- if (conn->pgtcp_user_timeout != NULL)
- {
- if (!parse_int_param(conn->pgtcp_user_timeout,
-
&cancel->pgtcp_user_timeout,
- conn,
"tcp_user_timeout"))
- goto fail;
- }
- if (conn->keepalives != NULL)
- {
- if (!parse_int_param(conn->keepalives,
- &cancel->keepalives,
- conn, "keepalives"))
- goto fail;
- }
- if (conn->keepalives_idle != NULL)
- {
- if (!parse_int_param(conn->keepalives_idle,
-
&cancel->keepalives_idle,
- conn,
"keepalives_idle"))
- goto fail;
- }
- if (conn->keepalives_interval != NULL)
- {
- if (!parse_int_param(conn->keepalives_interval,
-
&cancel->keepalives_interval,
- conn,
"keepalives_interval"))
- goto fail;
- }
- if (conn->keepalives_count != NULL)
+
+ /*
+ * Parse and interpret the TCP connection parameters, but only
+ * if the connection is not made through a Unix-domain socket.
+ * This is because Unix-domain sockets do not require these parameters.
+ */
+ if (cancel->raddr.addr.ss_family != AF_UNIX)
{
- if (!parse_int_param(conn->keepalives_count,
-
&cancel->keepalives_count,
- conn,
"keepalives_count"))
- goto fail;
+ if (conn->pgtcp_user_timeout != NULL)
+ {
+ if (!parse_int_param(conn->pgtcp_user_timeout,
+
&cancel->pgtcp_user_timeout,
+ conn,
"tcp_user_timeout"))
+ goto fail;
+ }
+ if (conn->keepalives != NULL)
+ {
+ if (!parse_int_param(conn->keepalives,
+
&cancel->keepalives,
+ conn,
"keepalives"))
+ goto fail;
+ }
+ if (conn->keepalives_idle != NULL)
+ {
+ if (!parse_int_param(conn->keepalives_idle,
+
&cancel->keepalives_idle,
+ conn,
"keepalives_idle"))
+ goto fail;
+ }
+ if (conn->keepalives_interval != NULL)
+ {
+ if (!parse_int_param(conn->keepalives_interval,
+
&cancel->keepalives_interval,
+ conn,
"keepalives_interval"))
+ goto fail;
+ }
+ if (conn->keepalives_count != NULL)
+ {
+ if (!parse_int_param(conn->keepalives_count,
+
&cancel->keepalives_count,
+ conn,
"keepalives_count"))
+ goto fail;
+ }
}
return cancel;
--
2.40.0
From be067eb3c7c87e70e6a3604d936a5c688d5bb1e7 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fu...@postgresql.org>
Date: Fri, 14 Apr 2023 02:46:26 +0900
Subject: [PATCH v2 2/2] Improve PQconnectPoll by strictly parsing the
keepalives parameter.
Commit e7a2217978 introduced the parse_int_param() function to parse
libpq connection parameters more strictly. However, PQconnectPoll()
was not updated to use parse_int_param() when parsing the keepalives
parameter. This caused inconsistency in the handling of the same
parameter between PQconnectPoll() and PQgetCancel().
This commit resolves the issue by using parse_int_param() to parse
the keepalives parameter in useKeepalives() called in PQconnectPoll().
By parsing the keepalives parameter more strictly, the behavior of
PQconnectPoll() is now consistent with PQgetCancel().
---
src/interfaces/libpq/fe-connect.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 6558d098b5..e626d50366 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2067,14 +2067,14 @@ connectFailureMessage(PGconn *conn, int errorno)
static int
useKeepalives(PGconn *conn)
{
- char *ep;
int val;
if (conn->keepalives == NULL)
return 1;
- val = strtol(conn->keepalives, &ep, 10);
- if (*ep)
+
+ if (!parse_int_param(conn->keepalives, &val, conn, "keepalives"))
return -1;
+
return val != 0 ? 1 : 0;
}
--
2.40.0
