Re: Remove unnecessary code from psql's watch command
On Fri, 08 Mar 2024 12:09:12 -0500 Tom Lane wrote: > Yugo NAGATA writes: > > On Wed, 06 Mar 2024 13:03:39 -0500 > > Tom Lane wrote: > >> I don't have Windows here to test on, but does the WIN32 code > >> path work at all? > > > The outer loop is eventually exited even because PSQLexecWatch returns 0 > > when cancel_pressed = 0. However, it happens after executing an extra > > query in this function not just after exit of the inner loop. Therefore, > > it would be better to adding set and check of "done" in WIN32, too. > > Ah, I see now. Agreed, that could stand improvement. > > > I've attached the updated patch > > (v2_remove_unnecessary_code_in_psql_watch.patch). > > Pushed with minor tidying. Thanks! -- Yugo NAGATA
Re: Remove unnecessary code from psql's watch command
Yugo NAGATA writes: > On Wed, 06 Mar 2024 13:03:39 -0500 > Tom Lane wrote: >> I don't have Windows here to test on, but does the WIN32 code >> path work at all? > The outer loop is eventually exited even because PSQLexecWatch returns 0 > when cancel_pressed = 0. However, it happens after executing an extra > query in this function not just after exit of the inner loop. Therefore, > it would be better to adding set and check of "done" in WIN32, too. Ah, I see now. Agreed, that could stand improvement. > I've attached the updated patch > (v2_remove_unnecessary_code_in_psql_watch.patch). Pushed with minor tidying. regards, tom lane
Re: Remove unnecessary code from psql's watch command
On Wed, 06 Mar 2024 13:03:39 -0500
Tom Lane wrote:
> Michael Paquier writes:
> > On Tue, Mar 05, 2024 at 10:05:52PM +0900, Yugo NAGATA wrote:
> >> In the current code of do_watch(), sigsetjmp is called if WIN32
> >> is defined, but siglongjmp is not called in the signal handler
> >> in this condition. On Windows, currently, cancellation is checked
> >> only by cancel_pressed, and calling sigsetjmp in do_watch() is
> >> unnecessary. Therefore, we can remove code around sigsetjmp in
> >> do_watch(). I've attached the patch for this fix.
>
> > Re-reading the top comment of sigint_interrupt_enabled, it looks like
> > you're right here. As long as we check for cancel_pressed there
> > should be no need for any special cancellation handling here.
>
> I don't have Windows here to test on, but does the WIN32 code
> path work at all? It looks to me like cancel_pressed becoming
> true doesn't get us to exit the outer loop, only the inner delay
> one, meaning that trying to control-C out of a \watch will just
> cause it to repeat the command even faster. That path isn't
> setting the "done" variable, and it isn't checking it either,
> because all of that only happens in the other #ifdef arm.
The outer loop is eventually exited even because PSQLexecWatch returns 0
when cancel_pressed = 0. However, it happens after executing an extra
query in this function not just after exit of the inner loop. Therefore,
it would be better to adding set and check of "done" in WIN32, too.
I've attached the updated patch
(v2_remove_unnecessary_code_in_psql_watch.patch).
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5c906e4806..6827d8b8d0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5312,30 +5312,22 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
continue;
#ifdef WIN32
-
- /*
- * Set up cancellation of 'watch' via SIGINT. We redo this each time
- * through the loop since it's conceivable something inside
- * PSQLexecWatch could change sigint_interrupt_jmp.
- */
- if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
- break;
-
/*
- * Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s).
+ * Wait a while before running the query again. Break the sleep into
+ * short intervals (at most 1s).
*/
- sigint_interrupt_enabled = true;
for (long i = sleep_ms; i > 0;)
{
long s = Min(i, 1000L);
pg_usleep(s * 1000L);
if (cancel_pressed)
+ {
+done = true;
break;
+ }
i -= s;
}
- sigint_interrupt_enabled = false;
#else
/* sigwait() will handle SIGINT. */
sigprocmask(SIG_BLOCK, , NULL);
@@ -5369,9 +5361,9 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
/* Unblock SIGINT so that slow queries can be interrupted. */
sigprocmask(SIG_UNBLOCK, , NULL);
+#endif
if (done)
break;
-#endif
}
if (pagerpipe)
Re: Remove unnecessary code from psql's watch command
Michael Paquier writes: > On Tue, Mar 05, 2024 at 10:05:52PM +0900, Yugo NAGATA wrote: >> In the current code of do_watch(), sigsetjmp is called if WIN32 >> is defined, but siglongjmp is not called in the signal handler >> in this condition. On Windows, currently, cancellation is checked >> only by cancel_pressed, and calling sigsetjmp in do_watch() is >> unnecessary. Therefore, we can remove code around sigsetjmp in >> do_watch(). I've attached the patch for this fix. > Re-reading the top comment of sigint_interrupt_enabled, it looks like > you're right here. As long as we check for cancel_pressed there > should be no need for any special cancellation handling here. I don't have Windows here to test on, but does the WIN32 code path work at all? It looks to me like cancel_pressed becoming true doesn't get us to exit the outer loop, only the inner delay one, meaning that trying to control-C out of a \watch will just cause it to repeat the command even faster. That path isn't setting the "done" variable, and it isn't checking it either, because all of that only happens in the other #ifdef arm. regards, tom lane
Re: Remove unnecessary code from psql's watch command
On Tue, Mar 05, 2024 at 10:05:52PM +0900, Yugo NAGATA wrote: > In the current code of do_watch(), sigsetjmp is called if WIN32 > is defined, but siglongjmp is not called in the signal handler > in this condition. On Windows, currently, cancellation is checked > only by cancel_pressed, and calling sigsetjmp in do_watch() is > unnecessary. Therefore, we can remove code around sigsetjmp in > do_watch(). I've attached the patch for this fix. Re-reading the top comment of sigint_interrupt_enabled, it looks like you're right here. As long as we check for cancel_pressed there should be no need for any special cancellation handling here. -- Michael signature.asc Description: PGP signature
Remove unnecessary code from psql's watch command
Hi,
In the current code of do_watch(), sigsetjmp is called if WIN32
is defined, but siglongjmp is not called in the signal handler
in this condition. On Windows, currently, cancellation is checked
only by cancel_pressed, and calling sigsetjmp in do_watch() is
unnecessary. Therefore, we can remove code around sigsetjmp in
do_watch(). I've attached the patch for this fix.
Regards,
Yugo Ngata
--
Yugo NAGATA
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5c906e4806..c03e47744e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5312,20 +5312,10 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
continue;
#ifdef WIN32
-
- /*
- * Set up cancellation of 'watch' via SIGINT. We redo this each time
- * through the loop since it's conceivable something inside
- * PSQLexecWatch could change sigint_interrupt_jmp.
- */
- if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
- break;
-
/*
- * Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s).
+ * Wait a while before running the query again. Break the sleep into
+ * short intervals (at most 1s).
*/
- sigint_interrupt_enabled = true;
for (long i = sleep_ms; i > 0;)
{
long s = Min(i, 1000L);
@@ -5335,7 +5325,6 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
break;
i -= s;
}
- sigint_interrupt_enabled = false;
#else
/* sigwait() will handle SIGINT. */
sigprocmask(SIG_BLOCK, , NULL);
