fe-utils - share query cancellation code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

fe-utils - share query cancellation code

Fabien COELHO-3

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.

fe-cancel-1.patch (29K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Ibrar Ahmed-4


On Thu, Oct 31, 2019 at 11:43 PM Fabien COELHO <[hidden email]> wrote:

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.

I give a quick look and I think we can 

void
psql_setup_cancel_handler(void)
{
#ifndef WIN32
        setup_cancel_handler(psql_sigint_callback);
#else
        setup_cancel_handler();
#endif /* WIN32 */
}

to

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.

Hmm, need to remove the assert in the function
"setup_cancel_handler"

-- Ibrar Ahmed
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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.

fe-cancel-2.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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

fe-cancel-3.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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

fe-cancel-4.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Thomas Munro-5
On Sat, Nov 2, 2019 at 10:38 AM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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

fe-cancel-5.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Michael Paquier-2
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

fe-cancel-6.patch (22K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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.

fe-cancel-7.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Michael Paquier-2
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

cancel-fixes.patch (2K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Fabien COELHO-3

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.
Reply | Threaded
Open this post in threaded view
|

Re: fe-utils - share query cancellation code

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment