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

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

On Wed, 20 Dec 2023 at 19:17, Jelte Fennema-Nio <[email-masked]> wrote:
>
> On Thu, 14 Dec 2023 at 13:57, Jelte Fennema-Nio <[email-masked]> wrote:
> > 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.

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.