Обсуждение: Make psql's qeury canceling test simple by using signal() routine of IPC::Run

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>

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.

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

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.

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>

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>

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

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

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>