Thread: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Yugo NAGATA
Date:
Hello, Currently, the psql's test of query cancelling (src/bin/psql/t/020_cancel.pl) gets the PPID of a running psql by using "\!" meta command, and sends SIGINT to the process by using "kill". However, IPC::Run provides signal() routine that sends a signal to a running process, so I think we can rewrite the test using this routine to more simple fashion as attached patch. What do you think about it? Use of signal() is suggested by Fabien COELHO during the discussion of query cancelling of pgbench [1]. [1] https://www.postgresql.org/message-id/7691ade8-78-dd4-e26-135ccfbf0e9%40mines-paristech.fr Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Fabien COELHO
Date:
Hello Yugo-san, > Currently, the psql's test of query cancelling (src/bin/psql/t/020_cancel.pl) > gets the PPID of a running psql by using "\!" meta command, and sends SIGINT to > the process by using "kill". However, IPC::Run provides signal() routine that > sends a signal to a running process, so I think we can rewrite the test using > this routine to more simple fashion as attached patch. > > What do you think about it? I'm the one who pointed out signal(), so I'm a little biaised, nevertheless, regarding the patch: Patch applies with "patch". Test code is definitely much simpler. Test run is ok on my Ubuntu laptop. Let's drop 25 lines of perl! -- Fabien.
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Michael Paquier
Date:
On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote:
> Test run is ok on my Ubuntu laptop.
I have a few comments about this patch.
On HEAD and even after this patch, we still have the following:
SKIP:
{
skip "cancel
testrequires a Unix shell", 2 if $windows_os;
Could the SKIP be removed for $windows_os? If not, this had better be
documented because the reason for the skip becomes incorrect.
The comment at the top of the SKIP block still states the following:
# There is, as of this writing, no documented way to get the PID of
# the process from IPC::Run. As a workaround, we have psql print its
# own PID (which is the parent of the shell launched by psql) to a
# file.
This is also incorrect.
--
Michael
Attachment
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Fabien COELHO
Date:
Bonjour Michaël, > On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote: >> Test run is ok on my Ubuntu laptop. > > I have a few comments about this patch. Argh, sorry! I looked at what was removed (a lot) from the previous version, not what was remaining and should also have been removed. -- Fabien.
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Yugo NAGATA
Date:
On Mon, 14 Aug 2023 08:29:25 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote:
> > Test run is ok on my Ubuntu laptop.
>
> I have a few comments about this patch.
>
> On HEAD and even after this patch, we still have the following:
> SKIP:
{
skip "cancel
testrequires a Unix shell", 2 if $windows_os;
>
> Could the SKIP be removed for $windows_os? If not, this had better be
> documented because the reason for the skip becomes incorrect.
>
> The comment at the top of the SKIP block still states the following:
> # There is, as of this writing, no documented way to get the PID of
> # the process from IPC::Run. As a workaround, we have psql print its
> # own PID (which is the parent of the shell launched by psql) to a
> # file.
>
> This is also incorrect.
Thank you for your comments
I will check whether the test works in Windows and remove SKIP if possible.
Also, I'll fix the comment in either case.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Yugo NAGATA
Date:
On Mon, 14 Aug 2023 23:37:25 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> On Mon, 14 Aug 2023 08:29:25 +0900
> Michael Paquier <michael@paquier.xyz> wrote:
>
> > On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote:
> > > Test run is ok on my Ubuntu laptop.
> >
> > I have a few comments about this patch.
> >
> > On HEAD and even after this patch, we still have the following:
> > SKIP:
{
skip
"canceltest requires a Unix shell", 2 if $windows_os;
> >
> > Could the SKIP be removed for $windows_os? If not, this had better be
> > documented because the reason for the skip becomes incorrect.
> >
> > The comment at the top of the SKIP block still states the following:
> > # There is, as of this writing, no documented way to get the PID of
> > # the process from IPC::Run. As a workaround, we have psql print its
> > # own PID (which is the parent of the shell launched by psql) to a
> > # file.
> >
> > This is also incorrect.
>
> Thank you for your comments
>
> I will check whether the test works in Windows and remove SKIP if possible.
> Also, I'll fix the comment in either case.
I checked if the test using IPC::Run::signal can work on Windows, and
confirmed that this didn't work because sending SIGINT caused to
terminate the test itself. Here is the results;
t/001_basic.pl ........... ok
t/010_tab_completion.pl .. skipped: readline is not supported by this build
t/020_cancel.pl .......... Terminating on signal SIGINT(2)
Therefore, this test should be skipped on Windows.
I attached the update patch. I removed the incorrect comments and
unnecessary lines. Also, I rewrote the test to use "skip_all" instead
of SKIP because we skip the whole test rather than a part of it.
Regards,
Yugo Nagata
>
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA <nagata@sraoss.co.jp>
>
>
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Michael Paquier
Date:
On Wed, Sep 06, 2023 at 12:45:24AM +0900, Yugo NAGATA wrote: > I attached the update patch. I removed the incorrect comments and > unnecessary lines. Also, I rewrote the test to use "skip_all" instead > of SKIP because we skip the whole test rather than a part of it. Thanks for checking how IPC::Run behaves in this case on Windows! Right. This test is currently setting up a node for nothing, so let's skip this test entirely under $windows_os and move on. I'll backpatch that down to 15 once the embargo on REL_16_STABLE is lifted with the 16.0 tag. -- Michael
Attachment
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Michael Paquier
Date:
On Tue, Sep 12, 2023 at 03:18:05PM +0900, Michael Paquier wrote: > On Wed, Sep 06, 2023 at 12:45:24AM +0900, Yugo NAGATA wrote: > > I attached the update patch. I removed the incorrect comments and > > unnecessary lines. Also, I rewrote the test to use "skip_all" instead > > of SKIP because we skip the whole test rather than a part of it. > > Thanks for checking how IPC::Run behaves in this case on Windows! > > Right. This test is currently setting up a node for nothing, so let's > skip this test entirely under $windows_os and move on. I'll backpatch > that down to 15 once the embargo on REL_16_STABLE is lifted with the > 16.0 tag. At the end, I have split this change into two: - One to disable the test to run on Windows, skipping the wasted node initialization, and applied that down to 15. - One to switch to signal(), only for HEAD to see what happens in the buildfarm once the test is able to run on platforms that do not support PPID. I am wondering as well how IPC::Run::signal is stable, as it is the first time we would use it, AFAIK. -- Michael
Attachment
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
From
Yugo NAGATA
Date:
On Wed, 13 Sep 2023 10:20:23 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Sep 12, 2023 at 03:18:05PM +0900, Michael Paquier wrote: > > On Wed, Sep 06, 2023 at 12:45:24AM +0900, Yugo NAGATA wrote: > > > I attached the update patch. I removed the incorrect comments and > > > unnecessary lines. Also, I rewrote the test to use "skip_all" instead > > > of SKIP because we skip the whole test rather than a part of it. > > > > Thanks for checking how IPC::Run behaves in this case on Windows! > > > > Right. This test is currently setting up a node for nothing, so let's > > skip this test entirely under $windows_os and move on. I'll backpatch > > that down to 15 once the embargo on REL_16_STABLE is lifted with the > > 16.0 tag. > > At the end, I have split this change into two: > - One to disable the test to run on Windows, skipping the wasted node > initialization, and applied that down to 15. > - One to switch to signal(), only for HEAD to see what happens in the > buildfarm once the test is able to run on platforms that do not > support PPID. I am wondering as well how IPC::Run::signal is stable, > as it is the first time we would use it, AFAIK. Thank you for pushing them. I agree with the suspection about IPC::Run::singnal, so I'll also check the buildfarm result. Regards, Yugo Nagata > -- > Michael -- Yugo NAGATA <nagata@sraoss.co.jp>