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

2024-04-04 Thread Tom Lane
[ from a week ago ] Alvaro Herrera writes: > Hm, indri failed: > ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new > -Wendif-labels -Wmissing-format-attribute -Wcast-function-type > -Wformat-security

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

2024-04-04 Thread Jelte Fennema-Nio
On Thu, 4 Apr 2024 at 10:45, Alvaro Herrera wrote: > Yeah, this seems to work and I no longer get that complaint from > headerscheck. patch LGTM

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

2024-04-04 Thread Alvaro Herrera
On 2024-Apr-03, Tom Lane wrote: > On my machine, headerscheck does not like this: > > $ src/tools/pginclude/headerscheck --cplusplus > In file included from /tmp/headerscheck.4gTaW5/test.cpp:3: > ./src/include/libpq/libpq-be-fe-helpers.h: In function 'char* > libpqsrv_cancel(PGconn*,

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

2024-04-03 Thread Tom Lane
Alvaro Herrera writes: > Great, thanks for looking. Pushed now, I'll be closing the commitfest > entry shortly. On my machine, headerscheck does not like this: $ src/tools/pginclude/headerscheck --cplusplus In file included from /tmp/headerscheck.4gTaW5/test.cpp:3:

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

2024-03-29 Thread Noah Misch
On Fri, Mar 29, 2024 at 09:17:55AM +0100, Jelte Fennema-Nio wrote: > On Thu, 28 Mar 2024 at 19:03, Tom Lane wrote: > > Could we make this test bulletproof by using an injection point? > > If not, I remain of the opinion that we're better off without it. > > Possibly, and if so, I agree that

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

2024-03-29 Thread Jelte Fennema-Nio
On Thu, 28 Mar 2024 at 19:03, Tom Lane wrote: > > Alvaro Herrera writes: > > It doesn't fail when it's too fast -- it's just that it doesn't cover > > the case we want to cover. > > That's hardly better, because then you think you have test > coverage but maybe you don't. Honestly, that seems

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

2024-03-28 Thread Tom Lane
Alvaro Herrera writes: > On 2024-Mar-28, Tom Lane wrote: >> If the test fails both when the machine is too slow and when it's >> too fast, then there's zero hope of making it stable and we should >> just remove it. > It doesn't fail when it's too fast -- it's just that it doesn't cover > the

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

2024-03-28 Thread Alvaro Herrera
On 2024-Mar-28, Tom Lane wrote: > Jelte Fennema-Nio writes: > > > I think we don't really want to make the timeout too short. Otherwise > > the query might get cancelled before we push any query down to the > > FDW. I guess that means that for some slow machines even 10ms is not > > enough to

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

2024-03-28 Thread Tom Lane
Jelte Fennema-Nio writes: > On Thu, 28 Mar 2024 at 17:43, Alvaro Herrera wrote: >> Hah, you're right, I can reproduce with a smaller timeout, and using SET >> LOCAL works as a fix. If we're doing that, why not reduce the timeout >> to 1ms? We don't need to wait extra 9ms ... > I think we

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

2024-03-28 Thread Jelte Fennema-Nio
On Thu, 28 Mar 2024 at 17:43, Alvaro Herrera wrote: > Hah, you're right, I can reproduce with a smaller timeout, and using SET > LOCAL works as a fix. If we're doing that, why not reduce the timeout > to 1ms? We don't need to wait extra 9ms ... I think we don't really want to make the timeout

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

2024-03-28 Thread Alvaro Herrera
On 2024-Mar-28, Jelte Fennema-Nio wrote: > Ugh that's annoying, the RESET is timing out too I guess. Hah, you're right, I can reproduce with a smaller timeout, and using SET LOCAL works as a fix. If we're doing that, why not reduce the timeout to 1ms? We don't need to wait extra 9ms ... --

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

2024-03-28 Thread Jelte Fennema-Nio
On Thu, 28 Mar 2024 at 17:34, Alvaro Herrera wrote: > > Eh, kestrel has also failed[1], apparently every query after the large > JOIN that this commit added as test fails with a statement timeout error. > > [1] >

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

2024-03-28 Thread Alvaro Herrera
Eh, kestrel has also failed[1], apparently every query after the large JOIN that this commit added as test fails with a statement timeout error. [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=kestrel=2024-03-28%2016%3A01%3A14 -- Álvaro Herrera 48°01'N 7°57'E —

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

2024-03-28 Thread Alvaro Herrera
On 2024-Mar-28, Alvaro Herrera wrote: > Undefined symbols for architecture arm64: > "_libintl_gettext", referenced from: > _libpqsrv_cancel in dblink.o > _libpqsrv_cancel in dblink.o > ld: symbol(s) not found for architecture arm64 > clang: error: linker command failed with exit

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

2024-03-28 Thread Alvaro Herrera
Hm, indri failed: ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument

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

2024-03-28 Thread Alvaro Herrera
On 2024-Mar-28, Jelte Fennema-Nio wrote: > Your changes look good, apart from the prverror stuff indeed. If you > remove the prverror stuff again I think this is ready to commit. Great, thanks for looking. Pushed now, I'll be closing the commitfest entry shortly. -- Álvaro Herrera

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

2024-03-28 Thread Jelte Fennema-Nio
On Wed, 27 Mar 2024 at 19:27, Alvaro Herrera wrote: > I ended up reducing the two PG_TRY blocks to a single one. I see no > reason to split them up, and this way it looks more legible. I definitely agree this looks better. Not sure why I hadn't done that, maybe it wasn't possible in one of the

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

2024-03-28 Thread Jelte Fennema-Nio
On Wed, 27 Mar 2024 at 19:46, Alvaro Herrera wrote: > > On 2024-Mar-27, Alvaro Herrera wrote: > > > I changed it so that the error messages are returned as translated > > phrases, and was bothered by the fact that if errors happen repeatedly, > > the memory for them might be leaked. Maybe this

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

2024-03-27 Thread Alvaro Herrera
On 2024-Mar-27, Alvaro Herrera wrote: > I changed it so that the error messages are returned as translated > phrases, and was bothered by the fact that if errors happen repeatedly, > the memory for them might be leaked. Maybe this is fine depending on > the caller's memory context, but since

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

2024-03-27 Thread Alvaro Herrera
On 2024-Mar-22, Jelte Fennema-Nio wrote: > On Thu, 21 Mar 2024 at 03:54, Noah Misch wrote: > > > > On Mon, Mar 18, 2024 at 07:40:10PM +0100, Alvaro Herrera wrote: > > > I enabled the test again and also pushed the changes to dblink, > > > isolationtester and fe_utils (which AFAICS is used by

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

2024-03-22 Thread Jelte Fennema-Nio
On Thu, 21 Mar 2024 at 03:54, Noah Misch wrote: > > On Mon, Mar 18, 2024 at 07:40:10PM +0100, Alvaro Herrera wrote: > > I enabled the test again and also pushed the changes to dblink, > > isolationtester and fe_utils (which AFAICS is used by pg_dump, > > I recommend adding a libpqsrv_cancel()

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

2024-03-20 Thread Noah Misch
On Mon, Mar 18, 2024 at 07:40:10PM +0100, Alvaro Herrera wrote: > I enabled the test again and also pushed the changes to dblink, > isolationtester and fe_utils (which AFAICS is used by pg_dump, I recommend adding a libpqsrv_cancel() function to libpq-be-fe-helpers.h, to use from dblink and

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

2024-03-18 Thread Alvaro Herrera
I enabled the test again and also pushed the changes to dblink, isolationtester and fe_utils (which AFAICS is used by pg_dump, pg_amcheck, reindexdb and vacuumdb). I chickened out of committing the postgres_fdw changes though, so here they are again. Not sure I'll find courage to get these done

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

2024-03-14 Thread Jelte Fennema-Nio
On Thu, 14 Mar 2024 at 11:33, Alvaro Herrera wrote: > Hmm, isn't this basically saying that we're giving up on reliably > canceling queries altogether? I mean, maybe we'd like to instead fix > the bug about canceling queries in extended query protocol ... > Isn't that something you're worried

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

2024-03-14 Thread Alvaro Herrera
On 2024-Mar-14, Jelte Fennema-Nio wrote: > On Wed, 13 Mar 2024 at 20:08, Jacob Champion > wrote: > > I hit this on my machine. With the attached diff I can reproduce > > constantly (including with the most recent test patch); I think the > > cancel must be arriving between the bind/execute

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

2024-03-14 Thread Jelte Fennema-Nio
On Wed, 13 Mar 2024 at 20:08, Jacob Champion wrote: > I hit this on my machine. With the attached diff I can reproduce > constantly (including with the most recent test patch); I think the > cancel must be arriving between the bind/execute steps? Nice find! Your explanation makes total sense.

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

2024-03-13 Thread Jacob Champion
On Wed, Mar 13, 2024 at 12:01 PM Alvaro Herrera wrote: > On 2024-Mar-13, Jelte Fennema-Nio wrote: > > Sadly I'm having a hard time reliably reproducing this race condition > > locally. So it's hard to be sure what is happening here. Attached is a > > patch with a wild guess as to what the issue

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

2024-03-13 Thread Alvaro Herrera
On 2024-Mar-13, Jelte Fennema-Nio wrote: > I agree it's probably a timing issue. The cancel being received after > the query is done seems very unlikely, since the query takes 180 > seconds (assuming PG_TEST_TIMEOUT_DEFAULT is not lowered for these > animals). I think it's more likely that the

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

2024-03-13 Thread Jelte Fennema-Nio
On Wed, 13 Mar 2024 at 04:53, Tom Lane wrote: > I suspect it's basically just a > timing dependency. Have you thought about the fact that a cancel > request is a no-op if it arrives after the query's done? I agree it's probably a timing issue. The cancel being received after the query is done

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

2024-03-12 Thread Tom Lane
Jelte Fennema-Nio writes: > On Tue, 12 Mar 2024 at 19:28, Alvaro Herrera wrote: >>> Hmm, buildfarm member kestrel (which uses >>> -fsanitize=undefined,alignment) failed: >> Hm, I tried using the same compile flags, couldn't reproduce. > Okay, it passed now it seems so I guess this test is

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

2024-03-12 Thread Jelte Fennema-Nio
On Tue, 12 Mar 2024 at 19:28, Alvaro Herrera wrote: > > On 2024-Mar-12, Alvaro Herrera wrote: > > > Hmm, buildfarm member kestrel (which uses > > -fsanitize=undefined,alignment) failed: > > > > # Running: libpq_pipeline -r 700 cancel port=49975 host=/tmp/dFh46H7YGc > > dbname='postgres' > > test

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

2024-03-12 Thread Alvaro Herrera
On 2024-Mar-12, Alvaro Herrera wrote: > Hmm, buildfarm member kestrel (which uses > -fsanitize=undefined,alignment) failed: > > # Running: libpq_pipeline -r 700 cancel port=49975 host=/tmp/dFh46H7YGc > dbname='postgres' > test cancellations... > libpq_pipeline:260: query did not fail when it

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

2024-03-12 Thread Alvaro Herrera
Hmm, buildfarm member kestrel (which uses -fsanitize=undefined,alignment) failed: # Running: libpq_pipeline -r 700 cancel port=49975 host=/tmp/dFh46H7YGc dbname='postgres' test cancellations... libpq_pipeline:260: query did not fail when it was expected

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

2024-03-12 Thread Alvaro Herrera
On 2024-Mar-12, Jelte Fennema-Nio wrote: > On Tue, 12 Mar 2024 at 10:19, Alvaro Herrera wrote: > > Here's a last one for the cfbot. > > Thanks for committing the first 3 patches btw. Attached a tiny change > to 0001, which adds "(backing struct for PGcancelConn)" to the comment > on

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

2024-03-12 Thread Jelte Fennema-Nio
On Tue, 12 Mar 2024 at 15:04, Jelte Fennema-Nio wrote: > Attached a tiny change to 0001 One more tiny comment change, stating that pg_cancel is used by the deprecated PQcancel function. v36-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch Description: Binary data

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

2024-03-12 Thread Jelte Fennema-Nio
On Tue, 12 Mar 2024 at 10:19, Alvaro Herrera wrote: > Here's a last one for the cfbot. Thanks for committing the first 3 patches btw. Attached a tiny change to 0001, which adds "(backing struct for PGcancelConn)" to the comment on pg_cancel_conn. From d340fde6883a249fd7c1a90033675a3b5edb603e Mon

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

2024-03-12 Thread Jelte Fennema-Nio
On Tue, 12 Mar 2024 at 10:53, Jelte Fennema-Nio wrote: > I'd rather fail as hard as possible when someone is using the API > wrongly. To be clear, this is my way of looking at it. If you feel strongly about that we should not change conn.status, I'm fine with making that change to the patchset.

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

2024-03-12 Thread Jelte Fennema-Nio
On Tue, 12 Mar 2024 at 10:19, Alvaro Herrera wrote: > If we do this and we see conn.status is not ALLOCATED, meaning a cancel > is already ongoing, shouldn't we leave conn.status alone instead of > changing to CONNECTION_BAD? I mean, we shouldn't be juggling the elbow > of whoever's doing that,

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

2024-03-12 Thread Alvaro Herrera
Here's a last one for the cfbot. I have a question about this one int PQcancelStart(PGcancelConn *cancelConn) { [...] if (cancelConn->conn.status != CONNECTION_ALLOCATED) { libpq_append_conn_error(>conn, "cancel request is already being sent on

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

2024-03-07 Thread Jelte Fennema-Nio
Attached is a new patchset with various changes. I created a dedicated 0002 patch to add tests for the already existing cancellation functions, because that seemed useful for another thread where changes to the cancellation protocol are being proposed[1]. [1]:

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

2024-03-06 Thread Jelte Fennema-Nio
On Wed, 6 Mar 2024 at 19:22, Alvaro Herrera wrote: > Docs: one bogus "that that". will fix > Did we consider having PQcancelConn() instead be called > PQcancelCreate()? Fine by me > Also, the comment still says > "Asynchronously cancel a query on the given connection. This requires > polling

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

2024-03-06 Thread Alvaro Herrera
Docs: one bogus "that that". Did we consider having PQcancelConn() instead be called PQcancelCreate()? I think this better conveys that what we're doing is create an object that can be used to do something, and that nothing else is done with it by default. Also, the comment still says

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

2024-03-06 Thread Jelte Fennema-Nio
On Wed, 6 Mar 2024 at 15:03, Denis Laxalde wrote: > > In patch 0004, I noticed a couple of typos in the documentation; please > find attached a fixup patch correcting these. Thanks, applied. > while not actually listing the "statuses". Should we list them? I listed the relevant statuses over

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

2024-03-06 Thread Denis Laxalde
In patch 0004, I noticed a couple of typos in the documentation; please find attached a fixup patch correcting these. Still in the documentation, same patch, the last paragraph documenting PQcancelPoll() ends as: + indicate the current stage of the connection procedure and might be

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

2024-02-14 Thread Jelte Fennema-Nio
On Wed, 14 Feb 2024 at 18:41, Alvaro Herrera wrote: > Hmm, I think the changes to libpq_pipeline in 0005 should be in 0004. Yeah, you're correct. Fixed that now. v32-0005-Start-using-new-libpq-cancel-APIs.patch Description: Binary data

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

2024-02-14 Thread Alvaro Herrera
On 2024-Feb-14, Jelte Fennema-Nio wrote: > Attached is a new version of the final patches, with much improved > docs (imho) and the new function names: PQcancelStart and > PQcancelBlocking. Hmm, I think the changes to libpq_pipeline in 0005 should be in 0004. -- Álvaro Herrera

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

2024-02-14 Thread Jelte Fennema-Nio
On Sun, 4 Feb 2024 at 16:39, Alvaro Herrera wrote: > Maybe this is okay? I'll have a look at the whole final situation more > carefully later; or if somebody else wants to share an opinion, please > do so. Attached is a new version of the final patches, with much improved docs (imho) and the

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

2024-02-04 Thread Alvaro Herrera
On 2024-Feb-02, Jelte Fennema-Nio wrote: > The only reasonable thing I can think of to make that situation better > is to move that part of the function outside of PQcancelPoll and > create a dedicated PQcancelStart function for it. It introduces an > extra function, but it does seem more in line

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

2024-02-02 Thread Jelte Fennema-Nio
On Fri, 2 Feb 2024 at 16:06, Alvaro Herrera wrote: > Now, looking at this list, I think it's surprising that the nonblocking > request for a cancellation is called PQcancelPoll. PQcancelSend() is at > odds with the asynchronous query API, which uses the verb "send" for the > asynchronous

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

2024-02-02 Thread Alvaro Herrera
Hello, The patched docs claim that PQrequestCancel is insecure, but neither the code nor docs explain why. The docs for PQcancel on the other hand do mention that encryption is not used; does that apply to PQrequestCancel as well and is that the reason? If so, I think we should copy the warning

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

2024-02-02 Thread Jelte Fennema-Nio
On Fri, 2 Feb 2024 at 13:19, Alvaro Herrera wrote: > Thank you, looks good. > > I propose the following minor/trivial fixes over your initial 3 patches. All of those seem good like fixes. Attached is an updated patchset where they are all applied. As well as adding a missing word ("been") in a

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

2024-02-02 Thread Alvaro Herrera
On 2024-Jan-29, Jelte Fennema-Nio wrote: > On Mon, 29 Jan 2024 at 12:44, Alvaro Herrera 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!

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

2024-01-29 Thread Jelte Fennema-Nio
On Mon, 29 Jan 2024 at 12:44, Alvaro Herrera 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

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

2024-01-29 Thread Alvaro Herrera
On 2024-Jan-28, Jelte Fennema-Nio wrote: > On Sun, 28 Jan 2024 at 10:51, Jelte Fennema-Nio wrote: > > 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

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

2024-01-28 Thread Jelte Fennema-Nio
On Sun, 28 Jan 2024 at 10:51, Jelte Fennema-Nio wrote: > 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

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

2024-01-28 Thread Jelte Fennema-Nio
On Sun, 28 Jan 2024 at 04:15, vignesh C wrote: > 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

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

2024-01-27 Thread vignesh C
On Fri, 26 Jan 2024 at 22:22, Jelte Fennema-Nio wrote: > > On Fri, 26 Jan 2024 at 13:11, Alvaro Herrera wrote: > > I wonder, would it make sense to put all these new functions in a > > separate file fe-cancel.c? > > > Okay I tried doing that. I think the end result is indeed quite nice, > having

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

2024-01-26 Thread Jelte Fennema-Nio
On Fri, 26 Jan 2024 at 18:19, Alvaro Herrera wrote: > 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. To

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

2024-01-26 Thread Alvaro Herrera
On 2024-Jan-26, Jelte Fennema-Nio wrote: > 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

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

2024-01-26 Thread Jelte Fennema-Nio
On Fri, 26 Jan 2024 at 13:11, Alvaro Herrera wrote: > I wonder, would it make sense to put all these new functions in a > 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

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

2024-01-26 Thread Alvaro Herrera
Pushed 0001. I wonder, would it make sense to put all these new functions in a separate file fe-cancel.c? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "World domination is proceeding according to plan"(Andrew Morton)

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

2024-01-26 Thread Jelte Fennema-Nio
On Fri, 26 Jan 2024 at 02:59, vignesh C wrote: > Please post an updated version for the same. Done. From 5a94d610a4fe138365e2e88c5cec72eba53ed036 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Thu, 14 Dec 2023 13:39:04 +0100 Subject: [PATCH v25 2/3] Add non-blocking version of PQcancel

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

2024-01-25 Thread vignesh C
On Wed, 20 Dec 2023 at 19:17, Jelte Fennema-Nio wrote: > > On Thu, 14 Dec 2023 at 13:57, Jelte Fennema-Nio 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

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

2023-12-20 Thread Jelte Fennema-Nio
On Thu, 14 Dec 2023 at 13:57, Jelte Fennema-Nio 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

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

2023-12-14 Thread Jelte Fennema-Nio
On Mon, 13 Nov 2023 at 03:39, Thomas Munro wrote: > We follow the common en-US usage: "canceled", "canceling" but > "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

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

2023-11-12 Thread Thomas Munro
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

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

2023-07-17 Thread Jelte Fennema
Rebased again to resolve some conflicts v22-0003-Add-non-blocking-version-of-PQcancel.patch Description: Binary data v22-0002-Return-2-from-pqReadData-on-EOF.patch Description: Binary data v22-0004-Start-using-new-libpq-cancel-APIs.patch Description: Binary data

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

2023-06-19 Thread Jelte Fennema
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. v21-0002-Return-2-from-pqReadData-on-EOF.patch Description: Binary data

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

2023-04-21 Thread Jelte Fennema
Okay, I rebased again. Indeed 983ec23007b gave the most problems. On Fri, 7 Apr 2023 at 10:02, Denis Laxalde wrote: > > 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

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

2023-04-07 Thread Denis Laxalde
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,

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

2023-03-30 Thread Jelte Fennema
On Thu, 30 Mar 2023 at 10:07, Denis Laxalde wrote: > Patch 5 is missing respective changes; please find attached a fixup > patch for these. 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

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

2023-03-30 Thread Denis Laxalde
Jelte Fennema a écrit : > On Wed, 29 Mar 2023 at 10:43, Denis Laxalde wrote: > > More importantly, not having PQcancelSend() creating the PGcancelConn > > makes reuse of that value, passing through PQcancelReset(), more > > intuitive. E.g., in the tests: > > You convinced me. Attached is an

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

2023-03-29 Thread Jelte Fennema
On Wed, 29 Mar 2023 at 10:43, Denis Laxalde wrote: > More importantly, not having PQcancelSend() creating the PGcancelConn > makes reuse of that value, passing through PQcancelReset(), more > intuitive. E.g., in the tests: You convinced me. Attached is an updated patch where PQcancelSend takes

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

2023-03-29 Thread Denis Laxalde
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

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

2023-03-28 Thread Jelte Fennema
On Tue, 28 Mar 2023 at 16:54, Denis Laxalde wrote: > I had a look into your patchset (v16), did a quick review and played a > bit with the feature. > > Patch 2 is missing the documentation about PQcancelSocket() and contains > a few typos; please find attached a (fixup) patch to correct these.

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

2023-03-28 Thread Denis Laxalde
Hi Jelte, I had a look into your patchset (v16), did a quick review and played a bit with the feature. Patch 2 is missing the documentation about PQcancelSocket() and contains a few typos; please find attached a (fixup) patch to correct these. --- a/src/interfaces/libpq/libpq-fe.h +++

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

2023-03-22 Thread Jelte Fennema
Rebased after conflicts with bfc9497ece01c7c45437bc36387cb1ebe346f4d2 Also included the fix for feedback from Daniel on patch 2, which he had shared in the load balancing thread. On Wed, 15 Mar 2023 at 09:49, Jelte Fennema wrote: > > The rebase was indeed trivial (git handled everything

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

2023-03-15 Thread Jelte Fennema
The rebase was indeed trivial (git handled everything automatically), because my first patch was doing a superset of the changes that were committed in b6dfee28f. Attached are the new patches. On Tue, 14 Mar 2023 at 19:04, Greg Stark wrote: > > On Tue, 14 Mar 2023 at 13:59, Tom Lane wrote: > >

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

2023-03-14 Thread Greg Stark
On Tue, 14 Mar 2023 at 13:59, Tom Lane wrote: > > "Gregory Stark (as CFM)" writes: > > It looks like this needs a big rebase in fea-uth.c fe-auth-scram.c and > > fe-connect.c. Every hunk is failing which perhaps means the code > > you're patching has been moved or refactored? > > The cfbot is

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

2023-03-14 Thread Tom Lane
"Gregory Stark (as CFM)" writes: > It looks like this needs a big rebase in fea-uth.c fe-auth-scram.c and > fe-connect.c. Every hunk is failing which perhaps means the code > you're patching has been moved or refactored? The cfbot is giving up after

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

2023-03-14 Thread Gregory Stark (as CFM)
It looks like this needs a big rebase in fea-uth.c fe-auth-scram.c and fe-connect.c. Every hunk is failing which perhaps means the code you're patching has been moved or refactored?

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

2023-03-01 Thread Gregory Stark (as CFM)
On Wed, 1 Mar 2023 at 14:48, Jelte Fennema wrote: > > > This looks like it needs a rebase. > > done Great. Please update the CF entry to Needs Review or Ready for Committer as appropriate :) -- Gregory Stark As Commitfest Manager

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

2023-03-01 Thread Greg S
On Tue, 28 Feb 2023 at 15:59, Gregory Stark wrote: > > This looks like it needs a rebase. So I'm updating the patch to Waiting on Author

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

2023-03-01 Thread Gregory Stark
This looks like it needs a rebase. === Applying patches on top of PostgreSQL commit ID 71a75626d5271f2bcdbdc43b8c13065c4634fd9f === === applying patch ./v11-0001-libpq-Run-pgindent-after-a9e9a9f32b3.patch patching file src/interfaces/libpq/fe-auth-scram.c patching file

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

2023-01-19 Thread Jelte Fennema
Is there anything that is currently blocking this patch? I'd quite like it to get into PG16. Especially since I ran into another use case that I would want to use this patch for recently: Adding an async cancel function to Python it's psycopg3 library. This library exposes both a Connection class

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

2022-11-30 Thread Jelte Fennema
> This version of the patch no longer applies, a rebased version is needed. Attached is a patch that applies cleanly again and is also changed to use the recently introduced libpq_append_conn_error. I also attached a patch that runs pgindent after the introduction of libpq_append_conn_error. I

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

2022-09-14 Thread Tom Lane
Jelte Fennema writes: > [ non-blocking PQcancel ] I pushed the 0001 patch (libpq_pipeline documentation) with a bit of further wordsmithing. As for 0002, I'm not sure that's anywhere near ready. I doubt it's a great idea to un-deprecate PQrequestCancel with a major change in its behavior. If

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

2022-06-27 Thread Jelte Fennema
(resent because it was blocked from the mailing-list due to inclusion of a blocked email address in the To line) From: Andres Freund > On 2022-04-04 15:21:54 +, Jelte Fennema wrote: > > 2. Added some extra sleeps to the cancellation test, to remove random > > failures on FreeBSD. > >

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

2022-03-28 Thread Jelte Fennema
Thanks for all the feedback everyone. I'll try to send a new patch later this week that includes user facing docs and a simplified API. For now a few responses: > Yeah. We need to choose a name for these new function(s) that is > sufficiently different from "PQcancel" that people won't expect