Re: fe-utils - share query cancellation code

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
Cc: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: fe-utils - share query cancellation code
Date: 2019-11-28 07:52:08
Message-ID: 20191128075208.GB259486@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Nov 06, 2019 at 10:41:39AM +0100, Fabien COELHO wrote:
> Indeed, I put it on the wrong side of a "#ifndef WIN32".
>
> Basically it is a false constant under WIN32, which it seems does not have
> sigint handler, but the code checks whether the non existent handler is
> enabled anyway.
>
> Patch v5 attached fixes that, hopefully.

I have looked at this one, and found a couple of issues, most of them
small-ish.

s/cancelation/cancellation/ in fe_utils/cancel.h.

Then, the format of the new file headers was not really consistent
with the rest, and the order of the headers included in most of the
files was incorrect. That would break the recent flow of commits done
by Amit K.

The query cancellation added to pgbench is different than the actual
refactoring, and it is a result of the refactoring, so I would rather
split that into two different commits for clarity. The split is easy
enough, so that's fine not to send two different patches.

Compilation of the patch fails for me on Windows for psql:
unresolved external symbol sigint_interrupt_jmp
Please note that Mr Robot complains as well at build time:
http://commitfest.cputube.org/fabien-coelho.html

Visibly the problem here is that sigint_interrupt_jmp is declared in
common.h, but you have moved it to a non-WIN32 section of the code in
psql/common.c. And actually, note that copy.c and mainloop.c make use
of it...

I would not worry much about SIGTERM as you mentioned in the comments,
query cancellations are associated to SIGINT now. There could be an
argument possible later to allow passing down a custom signal though.

Attached is an updated patch with a couple of edits I have done,
including the removal of some noise diffs and the previous edits. I
am switching the patch as waiting on author, bumping it to next CF at
the same time. Could you please fix the Windows issue?
--
Michael

Attachment Content-Type Size
fe-cancel-6.patch text/x-diff 21.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2019-11-28 08:01:21 Re: [HACKERS] Block level parallel vacuum
Previous Message Pavel Stehule 2019-11-28 07:45:33 Re: missing estimation for coalesce function