Feedback-ID: ia2694551:Fastmail
Date: Fri, 2 Feb 2024 13:19:38 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: vignesh C <vignesh21@gmail.com>, Thomas Munro <thomas.munro@gmail.com>,
	Denis Laxalde <denis.laxalde@dalibo.com>, Greg Stark <stark@mit.edu>,
 Tom Lane <tgl@sss.pgh.pa.us>,
	"Gregory Stark (as CFM)" <stark.cfm@gmail.com>,
 Jelte Fennema <Jelte.Fennema@microsoft.com>,
	Daniel Gustafsson <daniel@yesql.se>, Peter Eisentraut <peter@eisentraut.org>,
	Andres Freund <andres@anarazel.de>, Justin Pryzby <pryzby@telsasoft.com>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
Message-ID: <202402021219.mrrtsrwikgzc@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nynvjh35s62iujzg"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAGECzQRLpsk31bKa-iq6+nY0QYpYg_or7UK9n-z-DH0B07haOg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202402021219.mrrtsrwikgzc%40alvherre.pgsql>
Precedence: bulk


--nynvjh35s62iujzg
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2024-Jan-29, Jelte Fennema-Nio wrote:

> On Mon, 29 Jan 2024 at 12:44, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Thanks!  I committed 0001 now.  I also renamed the new
> > pq_parse_int_param to pqParseIntParam, for consistency with other
> > routines there.  Please rebase the other patches.
> 
> Awesome! Rebased, and renamed pq_release_conn_hosts to
> pqReleaseConnHosts for the same consistency reasons.

Thank you, looks good.

I propose the following minor/trivial fixes over your initial 3 patches.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu.  Five minutes later I realize that it's also talking
about food" (Donald Knuth)

--nynvjh35s62iujzg
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment; filename="0001-pgindent.patch.txt"

From 92ca8dc2739a777ff5a0df990d6e9818c5729ac5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 2 Feb 2024 10:57:02 +0100
Subject: [PATCH 1/4] pgindent

---
 src/interfaces/libpq/fe-cancel.c | 14 +++++++-------
 src/interfaces/libpq/libpq-fe.h  | 17 +++++++++--------
 src/tools/pgindent/typedefs.list |  1 +
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index 7416791d9f..d75c9628e7 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -137,7 +137,7 @@ oom_error:
  * Returns 1 if successful 0 if not.
  */
 int
-PQcancelSend(PGcancelConn * cancelConn)
+PQcancelSend(PGcancelConn *cancelConn)
 {
 	if (!cancelConn || cancelConn->conn.status == CONNECTION_BAD)
 		return 1;
@@ -157,7 +157,7 @@ PQcancelSend(PGcancelConn * cancelConn)
  * Poll a cancel connection. For usage details see PQconnectPoll.
  */
 PostgresPollingStatusType
-PQcancelPoll(PGcancelConn * cancelConn)
+PQcancelPoll(PGcancelConn *cancelConn)
 {
 	PGconn	   *conn = (PGconn *) cancelConn;
 	int			n;
@@ -249,7 +249,7 @@ PQcancelPoll(PGcancelConn * cancelConn)
  * Get the status of a cancel connection.
  */
 ConnStatusType
-PQcancelStatus(const PGcancelConn * cancelConn)
+PQcancelStatus(const PGcancelConn *cancelConn)
 {
 	return PQstatus((const PGconn *) cancelConn);
 }
@@ -260,7 +260,7 @@ PQcancelStatus(const PGcancelConn * cancelConn)
  * Get the socket of the cancel connection.
  */
 int
-PQcancelSocket(const PGcancelConn * cancelConn)
+PQcancelSocket(const PGcancelConn *cancelConn)
 {
 	return PQsocket((const PGconn *) cancelConn);
 }
@@ -271,7 +271,7 @@ PQcancelSocket(const PGcancelConn * cancelConn)
  * Get the socket of the cancel connection.
  */
 char *
-PQcancelErrorMessage(const PGcancelConn * cancelConn)
+PQcancelErrorMessage(const PGcancelConn *cancelConn)
 {
 	return PQerrorMessage((const PGconn *) cancelConn);
 }
@@ -283,7 +283,7 @@ PQcancelErrorMessage(const PGcancelConn * cancelConn)
  * request.
  */
 void
-PQcancelReset(PGcancelConn * cancelConn)
+PQcancelReset(PGcancelConn *cancelConn)
 {
 	pqClosePGconn((PGconn *) cancelConn);
 	cancelConn->conn.status = CONNECTION_STARTING;
@@ -299,7 +299,7 @@ PQcancelReset(PGcancelConn * cancelConn)
  * Closes and frees the cancel connection.
  */
 void
-PQcancelFinish(PGcancelConn * cancelConn)
+PQcancelFinish(PGcancelConn *cancelConn)
 {
 	PQfinish((PGconn *) cancelConn);
 }
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 857ba54d94..851e549355 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -329,17 +329,18 @@ extern PostgresPollingStatusType PQresetPoll(PGconn *conn);
 extern void PQreset(PGconn *conn);
 
 /* Create a PGcancelConn that's used to cancel a query on the given PGconn */
-extern PGcancelConn * PQcancelConn(PGconn *conn);
+extern PGcancelConn *PQcancelConn(PGconn *conn);
+
 /* issue a blocking cancel request */
-extern int	PQcancelSend(PGcancelConn * conn);
+extern int	PQcancelSend(PGcancelConn *conn);
 
 /* issue or poll a non-blocking cancel request */
-extern PostgresPollingStatusType PQcancelPoll(PGcancelConn * cancelConn);
-extern ConnStatusType PQcancelStatus(const PGcancelConn * cancelConn);
-extern int	PQcancelSocket(const PGcancelConn * cancelConn);
-extern char *PQcancelErrorMessage(const PGcancelConn * cancelConn);
-extern void PQcancelReset(PGcancelConn * cancelConn);
-extern void PQcancelFinish(PGcancelConn * cancelConn);
+extern PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+extern ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+extern int	PQcancelSocket(const PGcancelConn *cancelConn);
+extern char *PQcancelErrorMessage(const PGcancelConn *cancelConn);
+extern void PQcancelReset(PGcancelConn *cancelConn);
+extern void PQcancelFinish(PGcancelConn *cancelConn);
 
 
 /* request a cancel structure */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 91433d439b..9ffb169e9d 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1760,6 +1760,7 @@ PG_Locale_Strategy
 PG_Lock_Status
 PG_init_t
 PGcancel
+PGcancelConn
 PGcmdQueueEntry
 PGconn
 PGdataValue
-- 
2.39.2


--nynvjh35s62iujzg
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment;
	filename="0002-Add-missing-period.patch.txt"

From 2fb777f47288815a370a15c7c17f39496b64c4f5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 2 Feb 2024 13:16:51 +0100
Subject: [PATCH 2/4] Add missing period

---
 doc/src/sgml/libpq.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9808e67865..67f6378ba8 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -6093,7 +6093,7 @@ int PQcancelSend(PGcancelConn *conn);
 
       <para>
        The request is made over the given <structname>PGcancelConn</structname>,
-       which needs to be created with <xref linkend="libpq-PQcancelConn"/>
+       which needs to be created with <xref linkend="libpq-PQcancelConn"/>.
        The return value of <xref linkend="libpq-PQcancelSend"/>
        is 1 if the cancel request was successfully
        dispatched and 0 if not. If it was unsuccessful, the error message can be
-- 
2.39.2


--nynvjh35s62iujzg
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment;
	filename="0003-Add-missing-const-decorator-in-documentation.patch.txt"

From c2a6bc14174a1e7a8a4f1508a5c30ee0d30d5f47 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 2 Feb 2024 13:17:10 +0100
Subject: [PATCH 3/4] Add missing 'const' decorator in documentation

---
 doc/src/sgml/libpq.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 67f6378ba8..8648379f8d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -6166,7 +6166,7 @@ ConnStatusType PQcancelStatus(const PGcancelConn *conn);
        A version of <xref linkend="libpq-PQsocket"/> that can be used for
        cancellation connections.
 <synopsis>
-int PQcancelSocket(PGcancelConn *conn);
+int PQcancelSocket(const PGcancelConn *conn);
 </synopsis>
       </para>
      </listitem>
-- 
2.39.2


--nynvjh35s62iujzg
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment;
	filename="0004-Wording-whitespace-changes.patch.txt"

From ba6cc0ba4d387015d64a98a37eb72188191c3672 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 2 Feb 2024 13:17:25 +0100
Subject: [PATCH 4/4] Wording, whitespace changes

---
 doc/src/sgml/libpq.sgml           |  8 ++++----
 src/interfaces/libpq/fe-cancel.c  |  9 +++------
 src/interfaces/libpq/fe-connect.c | 12 ++++++------
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 8648379f8d..81b4028381 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -6052,7 +6052,7 @@ PGcancelConn *PQcancelConn(PGconn *conn);
        connection. A cancel request can be sent over this connection in a
        blocking manner using <xref linkend="libpq-PQcancelSend"/> and in a
        non-blocking manner using <xref linkend="libpq-PQcancelPoll"/>.
-       The return value should can be passed to <xref linkend="libpq-PQcancelStatus"/>,
+       The return value can be passed to <xref linkend="libpq-PQcancelStatus"/>
        to check if the <structname>PGcancelConn</structname> object was
        created successfully. The <structname>PGcancelConn</structname> object
        is an opaque structure that is not meant to be accessed directly by the
@@ -6150,9 +6150,9 @@ ConnStatusType PQcancelStatus(const PGcancelConn *conn);
        returns <symbol>CONNECTION_OK</symbol> for a <structname>PGcancelConn</structname>
        it means that that the dispatch of the cancel request has completed (although
        this is no promise that the query was actually canceled) and that the
-       connection is now closed. While a <symbol>CONNECTION_OK</symbol> result
-       for <structname>PGconn</structname> means that queries can be sent over
-       the connection.
+       cancel connection is now closed, while a <symbol>CONNECTION_OK</symbol>
+       result for <structname>PGconn</structname> means that queries can be
+       sent over the connection.
       </para>
 
      </listitem>
diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index d75c9628e7..6420384be7 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -51,7 +51,6 @@ PQcancelConn(PGconn *conn)
 		return (PGcancelConn *) cancelConn;
 	}
 
-
 	/*
 	 * Indicate that this connection is used to send a cancellation
 	 */
@@ -223,20 +222,19 @@ PQcancelPoll(PGcancelConn *cancelConn)
 #endif
 
 	/*
-	 * We don't expect any data, only connection closure. So if we strangly do
+	 * We don't expect any data, only connection closure. So if we strangely do
 	 * receive some data we consider that an error.
 	 */
 	if (n > 0)
 	{
-
 		libpq_append_conn_error(conn, "received unexpected response from server");
 		conn->status = CONNECTION_BAD;
 		return PGRES_POLLING_FAILED;
 	}
 
 	/*
-	 * Getting here means that we received an EOF. Which is what we were
-	 * expecting. The cancel request has completed.
+	 * Getting here means that we received an EOF, which is what we were
+	 * expecting -- the cancel request has completed.
 	 */
 	cancelConn->conn.status = CONNECTION_OK;
 	resetPQExpBuffer(&conn->errorMessage);
@@ -304,7 +302,6 @@ PQcancelFinish(PGcancelConn *cancelConn)
 	PQfinish((PGconn *) cancelConn);
 }
 
-
 /*
  * PQgetCancel: get a PGcancel structure corresponding to a connection.
  *
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 4add35ec5c..4f9b2182db 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -618,9 +618,9 @@ pqDropServerData(PGconn *conn)
 	conn->write_err_msg = NULL;
 
 	/*
-	 * Cancel connections should save their be_pid and be_key across
-	 * PQcancelReset invocations. Otherwise they would not have access to the
-	 * secret token of the connection they are supposed to cancel anymore.
+	 * Cancel connections need to retain their be_pid and be_key across
+	 * PQcancelReset invocations, otherwise they would not have access to the
+	 * secret token of the connection they are supposed to cancel.
 	 */
 	if (!conn->cancelRequest)
 	{
@@ -648,8 +648,8 @@ pqDropServerData(PGconn *conn)
  * PQconnectStart or PQconnectStartParams (which differ in the same way as
  * PQconnectdb and PQconnectdbParams) and PQconnectPoll.
  *
- * Internally, the static functions pqConnectDBStart, pqConnectDBComplete
- * are part of the connection procedure.
+ * The non-exported functions pqConnectDBStart, pqConnectDBComplete are
+ * part of the connection procedure implementation.
  */
 
 /*
@@ -2358,7 +2358,7 @@ pqConnectDBStart(PGconn *conn)
 	 * anything else looks at it.)
 	 *
 	 * Cancel requests are special though, they should only try one host and
-	 * address. These fields have already set up in PQcancelConn. So leave
+	 * address, and these fields have already set up in PQcancelConn, so leave
 	 * these fields alone for cancel requests.
 	 */
 	if (!conn->cancelRequest)
-- 
2.39.2


--nynvjh35s62iujzg--