Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Wed, 13 Sep 2023 10:20:23 +0900 Michael Paquier 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
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
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 signature.asc Description: PGP signature
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
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 signature.asc Description: PGP signature
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Mon, 14 Aug 2023 23:37:25 +0900
Yugo NAGATA wrote:
> On Mon, 14 Aug 2023 08:29:25 +0900
> Michael Paquier 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 test 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
>
>
--
Yugo NAGATA
diff --git a/src/bin/psql/t/020_cancel.pl b/src/bin/psql/t/020_cancel.pl
index 0765d82b92..cabbf0210a 100644
--- a/src/bin/psql/t/020_cancel.pl
+++ b/src/bin/psql/t/020_cancel.pl
@@ -9,72 +9,39 @@ use PostgreSQL::Test::Utils;
use Test::More;
use Time::HiRes qw(usleep);
-my $tempdir = PostgreSQL::Test::Utils::tempdir;
+# Test query canceling by sending SIGINT to a running psql
+
+if ($windows_os)
+{
+ plan skip_all => 'sending SIGINT on Windows terminates the test itself';
+}
my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
-# Test query canceling by sending SIGINT to a running psql
-#
-# 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.
-SKIP:
-{
- skip "cancel test requires a Unix shell", 2 if $windows_os;
+local %ENV = $node->_get_env();
- local %ENV = $node->_get_env();
+my ($stdin, $stdout, $stderr);
+my $h = IPC::Run::start([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
+ \$stdin, \$stdout, \$stderr);
- my ($stdin, $stdout, $stderr);
+# Send sleep command and wait until the server has registered it
+$stdin = "select pg_sleep($PostgreSQL::Test::Utils::timeout_default);\n";
+pump $h while length $stdin;
+$node->poll_query_until('postgres',
+ q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep') > 0;}
+) or die "timed out";
- # Test whether shell supports $PPID. It's part of POSIX, but some
- # pre-/non-POSIX shells don't support it (e.g., NetBSD).
- $stdin = "\\! echo \$PPID";
- IPC::Run::run([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
- '<', \$stdin, '>', \$stdout, '2>', \$stderr);
- $stdout =~ /^\d+$/ or skip "shell apparently does not support \$PPID", 2;
+# Send cancel request
+$h->signal('INT');
- # Now start the real test
- my $h = IPC::Run::start([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
- \$stdin, \$stdout, \$stderr);
+my $result = finish $h;
- # Get the PID
- $stdout = '';
- $stderr = '';
- $stdin = "\\! echo \$PPID >$tempdir/psql.pid\n";
- pump $h while length $stdin;
- my $count;
- my $psql_pid;
- until (
- -s "$tempdir/psql.pid"
- and ($psql_pid =
- PostgreSQL::Test::Utils::slurp_file("$tempdir/psql.pid")) =~
- /^\d+\n/s)
- {
- ($count++ < 100 * $PostgreSQL::Test::Utils::timeout_default)
- or die "pid file did not appear";
- usleep(10_000);
- }
-
- # Send sleep command and wait until the server has registered it
- $stdin = "select pg_sleep($PostgreSQL::Test::Utils::timeout_default);\n";
- pump $h while length $stdin;
- $node->poll_query_until('postgres',
- q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep') > 0;}
- ) or die "timed out";
-
- # Send cancel request
- kill 'INT', $psql_pid;
-
- my $result =
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Mon, 14 Aug 2023 08:29:25 +0900
Michael Paquier 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 test 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.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
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
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 test 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.
--
Michael
signature.asc
Description: PGP signature
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
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.
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
diff --git a/src/bin/psql/t/020_cancel.pl b/src/bin/psql/t/020_cancel.pl
index 0765d82b92..081a1468d9 100644
--- a/src/bin/psql/t/020_cancel.pl
+++ b/src/bin/psql/t/020_cancel.pl
@@ -29,35 +29,10 @@ SKIP:
my ($stdin, $stdout, $stderr);
- # Test whether shell supports $PPID. It's part of POSIX, but some
- # pre-/non-POSIX shells don't support it (e.g., NetBSD).
- $stdin = "\\! echo \$PPID";
- IPC::Run::run([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
- '<', \$stdin, '>', \$stdout, '2>', \$stderr);
- $stdout =~ /^\d+$/ or skip "shell apparently does not support \$PPID", 2;
-
# Now start the real test
my $h = IPC::Run::start([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
\$stdin, \$stdout, \$stderr);
- # Get the PID
- $stdout = '';
- $stderr = '';
- $stdin = "\\! echo \$PPID >$tempdir/psql.pid\n";
- pump $h while length $stdin;
- my $count;
- my $psql_pid;
- until (
- -s "$tempdir/psql.pid"
- and ($psql_pid =
- PostgreSQL::Test::Utils::slurp_file("$tempdir/psql.pid")) =~
- /^\d+\n/s)
- {
- ($count++ < 100 * $PostgreSQL::Test::Utils::timeout_default)
- or die "pid file did not appear";
- usleep(10_000);
- }
-
# Send sleep command and wait until the server has registered it
$stdin = "select pg_sleep($PostgreSQL::Test::Utils::timeout_default);\n";
pump $h while length $stdin;
@@ -66,7 +41,7 @@ SKIP:
) or die "timed out";
# Send cancel request
- kill 'INT', $psql_pid;
+ $h->signal('INT');
my $result = finish $h;
