Обсуждение: 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 <nagata@sraoss.co.jp>

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

Michael Paquier <michael@paquier.xyz> 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

On Wed, 06 Mar 2024 13:03:39 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael@paquier.xyz> 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 <nagata@sraoss.co.jp>

Yugo NAGATA <nagata@sraoss.co.jp> writes:
> On Wed, 06 Mar 2024 13:03:39 -0500
> Tom Lane <tgl@sss.pgh.pa.us> 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

On Fri, 08 Mar 2024 12:09:12 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo NAGATA <nagata@sraoss.co.jp> writes:
> > On Wed, 06 Mar 2024 13:03:39 -0500
> > Tom Lane <tgl@sss.pgh.pa.us> 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 <nagata@sraoss.co.jp>