Thread: fe-utils - share query cancellation code

Hello Devs,

This patch moves duplicated query cancellation code code from psql & 
scripts to fe-utils, so that it is shared and may be used by other 
commands.

This is because Masao-san suggested to add a query cancellation feature to 
pgbench for long queries (server-side data generation being discussed, but 
possibly pk and fk could use that as well).

-- 
Fabien.

Hello,

> I give a quick look and I think we can
>
> void
> psql_setup_cancel_handler(void)
> {
>        setup_cancel_handler(psql_sigint_callback);
> }
>
> Because it does not matter for setup_cancel_handler what we passed
> because it is ignoring that in case of windows.

The "psql_sigint_callback" function is not defined under WIN32.

I've fixed a missing NULL argument in the section you pointed out, though.

I've used the shared infrastructure in pgbench.

I've noticed yet another instance of the cancelation stuff in 
"src/bin/pg_dump/parallel.c", but it seems somehow different from the two 
others, so I have not tried to used the shared version.

-- 
Fabien.

On 2019-Nov-01, Fabien COELHO wrote:

> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index 03bcd22996..389b4d7bcd 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -59,9 +59,10 @@
>  
>  #include "common/int.h"
>  #include "common/logging.h"
> -#include "fe_utils/conditional.h"
>  #include "getopt_long.h"
>  #include "libpq-fe.h"
> +#include "fe_utils/conditional.h"
> +#include "fe_utils/cancel.h"
>  #include "pgbench.h"
>  #include "portability/instr_time.h"

wtf?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Alvaro,

>>  #include "common/int.h"
>>  #include "common/logging.h"
>> -#include "fe_utils/conditional.h"
>>  #include "getopt_long.h"
>>  #include "libpq-fe.h"
>> +#include "fe_utils/conditional.h"
>> +#include "fe_utils/cancel.h"
>>  #include "pgbench.h"
>>  #include "portability/instr_time.h"
>
> wtf?

I understand that you are unhappy about something, but where the issue is 
fails me, the "wtf" 3 characters are not enough to point me in the right 
direction. Feel free to elaborate a little bit more:-)

-- 
Fabien.

On 2019-Nov-01, Fabien COELHO wrote:

> > >  #include "common/int.h"
> > >  #include "common/logging.h"
> > > -#include "fe_utils/conditional.h"
> > >  #include "getopt_long.h"
> > >  #include "libpq-fe.h"
> > > +#include "fe_utils/conditional.h"
> > > +#include "fe_utils/cancel.h"
> > >  #include "pgbench.h"
> > >  #include "portability/instr_time.h"
> > 
> > wtf?
> 
> I understand that you are unhappy about something, but where the issue is
> fails me, the "wtf" 3 characters are not enough to point me in the right
> direction. Feel free to elaborate a little bit more:-)

I don't see why you move the "conditional.h" line out of its correct
alphabetical position (where it is now), and then add "cancel.h" next to
it also out of its correct alphabetical position.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>> I understand that you are unhappy about something, but where the issue is
>> fails me, the "wtf" 3 characters are not enough to point me in the right
>> direction. Feel free to elaborate a little bit more:-)
>
> I don't see why you move the "conditional.h" line out of its correct
> alphabetical position (where it is now), and then add "cancel.h" next to
> it also out of its correct alphabetical position.

Because "cancel.h" requires PGconn declaration, thus must appear after 
"libpq-fe.h", and once I put it after that letting "conditional.h" above & 
alone looked a little bit silly. I put cancel after conditional because it 
was the new addition, which is somehow logical, although not alphabetical.

Now I can put cancel before conditional, sure.

Patch v3 attached does that.

-- 
Fabien.

On 2019-Nov-01, Fabien COELHO wrote:

> Because "cancel.h" requires PGconn declaration, thus must appear after
> "libpq-fe.h",

Then you need to add #include libpq-fe.h in cancel.h.  Our policy is
that headers compile standalone (c.h / postgres_fe.h / postgres.h
excluded).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> Then you need to add #include libpq-fe.h in cancel.h.  Our policy is
> that headers compile standalone (c.h / postgres_fe.h / postgres.h
> excluded).

Ok. I do not make a habit of adding headers in postgres, so I did not 
notice there was an alphabetical logic to that.

Attached patch v4 does it.

-- 
Fabien.

On Sat, Nov 2, 2019 at 10:38 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> Attached patch v4 does it.

Hi Fabien,

It looks like don't define sigint_interrupt_jmp and
sigint_interrupt_enabled on Windows, yet they are still declared and
referenced?

command.obj : error LNK2001: unresolved external symbol
sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj]
copy.obj : error LNK2001: unresolved external symbol
sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj]
input.obj : error LNK2001: unresolved external symbol
sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj]
mainloop.obj : error LNK2001: unresolved external symbol
sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj]
command.obj : error LNK2001: unresolved external symbol
sigint_interrupt_jmp [C:\projects\postgresql\psql.vcxproj]
copy.obj : error LNK2001: unresolved external symbol
sigint_interrupt_jmp [C:\projects\postgresql\psql.vcxproj]
mainloop.obj : error LNK2001: unresolved external symbol
sigint_interrupt_jmp [C:\projects\postgresql\psql.vcxproj]
.\Release\psql\psql.exe : fatal error LNK1120: 2 unresolved externals
[C:\projects\postgresql\psql.vcxproj]
0 Warning(s)

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64074

> It looks like don't define sigint_interrupt_jmp and 
> sigint_interrupt_enabled on Windows, yet they are still declared and 
> referenced?

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.

-- 
Fabien.

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

Bonjour Michaël,

> 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.

Yep, different file set.

> 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...

Indeed.

> 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.

Ok.

> Attached is an updated patch with a couple of edits I have done,
> including the removal of some noise diffs and the previous edits.

Thanks!

> 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?

I do not have a Windows host, so I can only do blind tests. I just moved 
the declaration out of !WIN32 scope in attached v7, which might solve the 
resolution error. All other issues pointed out above seem fixed in the v6 
you sent.

-- 
Fabien.

On Fri, Nov 29, 2019 at 08:44:25AM +0100, Fabien COELHO wrote:
> I do not have a Windows host, so I can only do blind tests. I just moved the
> declaration out of !WIN32 scope in attached v7, which might solve the
> resolution error. All other issues pointed out above seem fixed in the v6
> you sent.

Committed the patch after splitting things into two commits and after
testing things from Linux and from a Windows console: the actual
refactoring and the pgbench changes.  While polishing the code, I have
found the upthread argument of Ibrar quite appealing because there are
use cases where a callback can be interesting on Windows, like simply
being able to log the cancel event to a different source.  So I have
removed the callback restriction and the assertion, making the
callback of psql a no-op on Windows.  A second thing is that two large
comments originally in psql had better be moved to cancel.c because
the logic with libpq cancel routines applies only there.
--
Michael

On Mon, Dec 02, 2019 at 11:54:02AM +0900, Michael Paquier wrote:
> Committed the patch after splitting things into two commits and after
> testing things from Linux and from a Windows console: the actual
> refactoring and the pgbench changes.

I have found that we have a useless declaration of CancelRequested in
common.h, which is already part of cancel.h.  On top of that I think
that we need to rework a bit the header inclusions of bin/scripts/, as
per the attached.  A small set of issues, still these are issues.
Sorry for having missed these.
--
Michael

Bonjour Michaël,

>> Committed the patch after splitting things into two commits and after 
>> testing things from Linux and from a Windows console: the actual 
>> refactoring and the pgbench changes.
>
> I have found that we have a useless declaration of CancelRequested in 
> common.h, which is already part of cancel.h.

Ok.

> On top of that I think that we need to rework a bit the header 
> inclusions of bin/scripts/, as per the attached.

Looks fine to me: patch applies, compiles, runs.

-- 
Fabien.

On Tue, Dec 03, 2019 at 01:11:27PM +0100, Fabien COELHO wrote:
> Looks fine to me: patch applies, compiles, runs.

Thanks for double-checking.  Done.
--
Michael