Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

From: Denis Laxalde <[email-masked]>
Date: Wed, 29 Mar 2023 10:43:14 +0200

Jelte Fennema a écrit :
> > Namely, I wonder why it returns a PGcancelConn and what's the
> > point of requiring the user to call PQcancelStatus() to see if something
> > got wrong. Maybe it could be defined as:
> >
> >   int PQcancelSend(PGcancelConn *cancelConn);
> >
> > where the return value would be status? And the user would only need to
> > call PQcancelErrorMessage() in case of error. This would leave only one
> > single way to create a PGcancelConn value (i.e. PQcancelConn()), which
> > seems less confusing to me.
> 
> To clarify what you mean, the API would then be like this:
> PGcancelConn cancelConn = PQcancelConn(conn);
> if (PQcancelSend(cancelConn) == CONNECTION_BAD) {
>    printf("ERROR %s\n", PQcancelErrorMessage(cancelConn))
>    exit(1)
> }

I'm not sure it's worth returning the connection status, maybe just an
int value (the return value of connectDBComplete() for instance).

More importantly, not having PQcancelSend() creating the PGcancelConn
makes reuse of that value, passing through PQcancelReset(), more
intuitive. E.g., in the tests:

diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 6764ab513b..91363451af 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -217,17 +217,18 @@ test_cancel(PGconn *conn, const char *conninfo)
 		pg_fatal("failed to run PQrequestCancel: %s", PQerrorMessage(conn));
 	confirm_query_cancelled(conn);
 
+	cancelConn = PQcancelConn(conn);
+
 	/* test PQcancelSend */
 	send_cancellable_query(conn, monitorConn);
-	cancelConn = PQcancelSend(conn);
-	if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
+	if (PQcancelSend(cancelConn) == CONNECTION_BAD)
 		pg_fatal("failed to run PQcancelSend: %s", PQcancelErrorMessage(cancelConn));
 	confirm_query_cancelled(conn);
-	PQcancelFinish(cancelConn);
+
+	PQcancelReset(cancelConn);
 
 	/* test PQcancelConn and then polling with PQcancelPoll */
 	send_cancellable_query(conn, monitorConn);
-	cancelConn = PQcancelConn(conn);
 	if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
 		pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
 	while (true)

Otherwise, it's not clear if the PGcancelConn created by PQcancelSend()
should be reused or not. But maybe that's a matter of documentation?

> > As part of my testing, I've implemented non-blocking cancellation in
> > Psycopg, based on v16 on this patchset. Overall this worked fine and
> > seems useful; if you want to try it:
> >
> >   https://github.com/dlax/psycopg3/tree/pg16/non-blocking-pqcancel
> 
> That's great to hear! I'll try to take a closer look at that change tomorrow.

See also https://github.com/psycopg/psycopg/issues/534 if you want to
discuss about this.

> > (The only thing I found slightly inconvenient is the need to convey the
> > connection encoding (from PGconn) when handling error message from the
> > PGcancelConn.)
> 
> Could you expand a bit more on this? And if you have any idea on how
> to improve the API with regards to this?

The thing is that we need the connection encoding (client_encoding) when
eventually forwarding the result of PQcancelErrorMessage(), decoded, to
the user. More specifically, it seems to me that we'd the encoding of
the *cancel connection*, but since PQparameterStatus() cannot be used
with a PGcancelConn, I use that of the PGconn. Roughly, in Python:

    encoding = conn.parameter_status(b"client_encoding")
    # i.e, in C: char *encoding PQparameterStatus(conn, "client_encoding");
    cancel_conn = conn.cancel_conn()
    # i.e., in C: PGcancelConn *cancelConn = PQcancelConn(conn);
    # [... then work with with cancel_conn ...]
    if cancel_conn.status == ConnStatus.BAD:
        raise OperationalError(cancel_conn.error_message().decode(encoding))

This feels a bit non-atomic to me; isn't there a risk that
client_encoding be changed between PQparameterStatus(conn) and
PQcancelConn(conn) calls?

So maybe PQcancelParameterStatus(PGcancelConn *cancelConn, char *name)
is needed?

---

From: Jelte Fennema <[email-masked]>
Date: Wed, 29 Mar 2023 17:58:51 +0200

> intuitive. E.g., in the tests:

You convinced me. Attached is an updated patch where PQcancelSend
takes the PGcancelConn and returns 1 or 0.

> the user.

Cancel connections don't have an encoding specified. They never
receive an error from the server. All errors come from the machine
that libpq is on. So I think you're making the decoding more
complicated than it needs to be.

---

From: Denis Laxalde <[email-masked]>
Date: Thu, 30 Mar 2023 10:07:28 +0200

> takes the PGcancelConn and returns 1 or 0.

Patch 5 is missing respective changes; please find attached a fixup
patch for these.
>From c9e59fb3e30db1bfab75be9fdd4afbc227a5270e Mon Sep 17 00:00:00 2001
From: Denis Laxalde <[email-masked]>
Date: Thu, 30 Mar 2023 09:19:18 +0200
Subject: [PATCH] fixup! Start using new libpq cancel APIs

---
 contrib/dblink/dblink.c              | 4 ++--
 src/fe_utils/connect_utils.c         | 4 +++-
 src/test/isolation/isolationtester.c | 4 ++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index e139f66e11..073795f088 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1332,11 +1332,11 @@ dblink_cancel_query(PG_FUNCTION_ARGS)
 
 	dblink_init();
 	conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
-	cancelConn = PQcancelSend(conn);
+	cancelConn = PQcancelConn(conn);
 
 	PG_TRY();
 	{
-		if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
+		if (!PQcancelSend(cancelConn))
 		{
 			msg = pchomp(PQcancelErrorMessage(cancelConn));
 		}
diff --git a/src/fe_utils/connect_utils.c b/src/fe_utils/connect_utils.c
index b32448c010..1cfd717217 100644
--- a/src/fe_utils/connect_utils.c
+++ b/src/fe_utils/connect_utils.c
@@ -161,7 +161,9 @@ disconnectDatabase(PGconn *conn)
 
 	if (PQtransactionStatus(conn) == PQTRANS_ACTIVE)
 	{
-		PQcancelFinish(PQcancelSend(conn));
+		PGcancelConn *cancelConn = PQcancelConn(conn);
+		PQcancelSend(cancelConn);
+		PQcancelFinish(cancelConn);
 	}
 
 	PQfinish(conn);
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 3781f7982b..de31a87571 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -946,9 +946,9 @@ try_complete_step(TestSpec *testspec, PermutationStep *pstep, int flags)
 			 */
 			if (td > max_step_wait && !canceled)
 			{
-				PGcancelConn *cancel_conn = PQcancelSend(conn);
+				PGcancelConn *cancel_conn = PQcancelConn(conn);
 
-				if (PQcancelStatus(cancel_conn) == CONNECTION_OK)
+				if (PQcancelSend(cancel_conn))
 				{
 					/*
 					 * print to stdout not stderr, as this should appear in
-- 
2.30.2

---

From: Jelte Fennema <[email-masked]>
Date: Thu, 30 Mar 2023 12:17:21 +0200

> Patch 5 is missing respective changes; please find attached a fixup

Thanks, attached are newly rebased patches that include this change. I
also cast the result of PQcancelSend to to void in the one case where
it's ignored on purpose. Note that the patchset shrunk by one, since
the original patch 0002 has been committed now.

---

From: Denis Laxalde <[email-masked]>
Date: Fri, 7 Apr 2023 10:01:01 +0200

The patch set does not apply any more.

I tried to rebase locally; even leaving out 1 ("libpq: Run pgindent 
after a9e9a9f32b3"), patch 4 ("Start using new libpq cancel APIs") is 
harder to resolve following 983ec23007b (I suppose).

Appart from that, the implementation in v19 sounds good to me, and seems 
worthwhile. FWIW, as said before, I also implemented it in Psycopg in a 
sort of an end-to-end validation.

---

From: Jelte Fennema <[email-masked]>
Date: Fri, 21 Apr 2023 10:20:35 +0200

Okay, I rebased again. Indeed 983ec23007b gave the most problems.

---

From: Jelte Fennema <[email-masked]>
Date: Mon, 19 Jun 2023 12:52:48 +0200

I noticed that cfbot was unable to run tests due to some rebase
conflict. It seems the pgindent changes from patch 1 have now been
made.
So adding the rebased patches without patch 1 now to unblock cfbot.

---

From: Jelte Fennema <[email-masked]>
Date: Mon, 17 Jul 2023 15:00:50 +0200

Rebased again to resolve some conflicts

---

From: Thomas Munro <[email-masked]>
Date: Mon, 13 Nov 2023 15:38:34 +1300

Trivial observation: these patches obviously introduce many instances
of words derived from "cancel", but they don't all conform to
established project decisions (cf 21f1e15a) about how to spell them.
We follow the common en-US usage: "canceled", "canceling" but
"cancellation".  Blame Webstah et al.

https://english.stackexchange.com/questions/176957/cancellation-canceled-canceling-us-usage

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 14 Dec 2023 13:57:47 +0100

> "cancellation".  Blame Webstah et al.

I changed all the places that were not adhering to those spellings.
There were also a few of such places in parts of the codebase that
these changes didn't touch. I included a new 0001 patch to fix those.

I do feel like this patchset is pretty much in a committable state. So
it would be very much appreciated if any comitter could help push it
over the finish line.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 20 Dec 2023 14:46:52 +0100

> I changed all the places that were not adhering to those spellings.

It seems I forgot a /g on my sed command to do this so it turned out I
missed one that caused the test to fail to compile... Attached is a
fixed version.

I also updated the patchset to use the EOF detection provided by
0a5c46a7a488f2f4260a90843bb9de6c584c7f4e instead of introducing a new
way of EOF detection using a -2 return value.

---

From: vignesh C <[email-masked]>
Date: Fri, 26 Jan 2024 07:29:07 +0530

> way of EOF detection using a -2 return value.

CFBot shows that the patch does not apply anymore as in [1]:
patching file doc/src/sgml/libpq.sgml
...
patching file src/interfaces/libpq/exports.txt
Hunk #1 FAILED at 191.
1 out of 1 hunk FAILED -- saving rejects to file
src/interfaces/libpq/exports.txt.rej
patching file src/interfaces/libpq/fe-connect.c

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_3511.log

Regards,
Vignesh

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 26 Jan 2024 11:44:24 +0100

> Please post an updated version for the same.

Done.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 26 Jan 2024 13:11:02 +0100

Pushed 0001.

I wonder, would it make sense to put all these new functions in a
separate file fe-cancel.c?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"World domination is proceeding according to plan"        (Andrew Morton)

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 26 Jan 2024 17:52:45 +0100

> separate file fe-cancel.c?

Okay I tried doing that. I think the end result is indeed quite nice,
having all the cancellation related functions together in a file. But
it did require making a bunch of static functions in fe-connect
extern, and adding them to libpq-int.h. On one hand that seems fine to
me, on the other maybe that indicates that this cancellation logic
makes sense to be in the same file as the other connection functions
(in a sense, connecting is all that a cancel request does).

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 26 Jan 2024 18:19:41 +0100

> (in a sense, connecting is all that a cancel request does).

Yeah, I see that point of view as well.  I like the end result; the
additional protos in libpq-int.h don't bother me.  Does anybody else
wants to share their opinion on it?  If none, then I'd consider going
ahead with this version.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"We’ve narrowed the problem down to the customer’s pants being in a situation
 of vigorous combustion" (Robert Haas, Postgres expert extraordinaire)

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Sat, 27 Jan 2024 00:14:58 +0100

> wants to share their opinion on it?  If none, then I'd consider going

To be clear, I'm +1 on the new file structure (although if people feel
strongly against it, I don't care enough to make a big deal out of
it).

@Alvaro did you have any other comments on the contents of the patch btw?

---

From: vignesh C <[email-masked]>
Date: Sun, 28 Jan 2024 08:45:08 +0530

> (in a sense, connecting is all that a cancel request does).

CFBot shows that the patch has few compilation errors as in [1]:
[17:07:07.621] /usr/bin/ld:
../../../src/fe_utils/libpgfeutils.a(cancel.o): in function
`handle_sigint':
[17:07:07.621] cancel.c:(.text+0x50): undefined reference to `PQcancel'
[17:07:07.621] /usr/bin/ld:
../../../src/fe_utils/libpgfeutils.a(cancel.o): in function
`SetCancelConn':
[17:07:07.621] cancel.c:(.text+0x10c): undefined reference to `PQfreeCancel'
[17:07:07.621] /usr/bin/ld: cancel.c:(.text+0x114): undefined
reference to `PQgetCancel'
[17:07:07.621] /usr/bin/ld:
../../../src/fe_utils/libpgfeutils.a(cancel.o): in function
`ResetCancelConn':
[17:07:07.621] cancel.c:(.text+0x148): undefined reference to `PQfreeCancel'
[17:07:07.621] /usr/bin/ld:
../../../src/fe_utils/libpgfeutils.a(connect_utils.o): in function
`disconnectDatabase':
[17:07:07.621] connect_utils.c:(.text+0x2fc): undefined reference to
`PQcancelConn'
[17:07:07.621] /usr/bin/ld: connect_utils.c:(.text+0x307): undefined
reference to `PQcancelSend'
[17:07:07.621] /usr/bin/ld: connect_utils.c:(.text+0x30f): undefined
reference to `PQcancelFinish'
[17:07:07.623] /usr/bin/ld: ../../../src/interfaces/libpq/libpq.so:
undefined reference to `PQcancelPoll'
[17:07:07.626] collect2: error: ld returned 1 exit status
[17:07:07.626] make[3]: *** [Makefile:31: pg_amcheck] Error 1
[17:07:07.626] make[2]: *** [Makefile:45: all-pg_amcheck-recurse] Error 2
[17:07:07.626] make[2]: *** Waiting for unfinished jobs....
[17:07:08.126] /usr/bin/ld: ../../../src/interfaces/libpq/libpq.so:
undefined reference to `PQcancelPoll'
[17:07:08.130] collect2: error: ld returned 1 exit status
[17:07:08.131] make[3]: *** [Makefile:42: initdb] Error 1
[17:07:08.131] make[2]: *** [Makefile:45: all-initdb-recurse] Error 2
[17:07:08.492] /usr/bin/ld: ../../../src/interfaces/libpq/libpq.so:
undefined reference to `PQcancelPoll'
[17:07:08.495] collect2: error: ld returned 1 exit status
[17:07:08.496] make[3]: *** [Makefile:50: pg_basebackup] Error 1
[17:07:08.496] make[2]: *** [Makefile:45: all-pg_basebackup-recurse] Error 2
[17:07:09.060] /usr/bin/ld: parallel.o: in function `sigTermHandler':
[17:07:09.060] parallel.c:(.text+0x1aa): undefined reference to `PQcancel'

[1] - https://cirrus-ci.com/task/6210637211107328

Regards,
Vignesh

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Sun, 28 Jan 2024 10:51:48 +0100

> [17:07:07.621] cancel.c:(.text+0x50): undefined reference to `PQcancel'

I forgot to update ./configure based builds with the new file, only
meson was working. Also it seems I trimmed the header list fe-cancel.c
a bit too much for OSX, so I added unistd.h back.

Both of those are fixed now.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Sun, 28 Jan 2024 13:39:42 +0100

> Both of those are fixed now.

Okay, there turned out to also be an issue on Windows with
setKeepalivesWin32 not being available in fe-cancel.c. That's fixed
now too (as well as some minor formatting issues).

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 29 Jan 2024 12:44:44 +0100

> now too (as well as some minor formatting issues).

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.

Thanks,

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Mon, 29 Jan 2024 13:28:22 +0100

> routines there.  Please rebase the other patches.

Awesome! Rebased, and renamed pq_release_conn_hosts to
pqReleaseConnHosts for the same consistency reasons.