Re: proposal - psql - use pager for \watch command

so 20. 3. 2021 v 23:45 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Thu, Mar 4, 2021 at 11:28 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > čt 4. 3. 2021 v 7:37 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> napsal:
> >> Here is a little bit updated patch - detection of end of any child
> process cannot be used on WIN32.
>
> Yeah, it's OK for me if this feature only works on Unix until the
> right person for the job shows up with a patch.  If there is no pspg
> on Windows, how would we even know if it works?
>
> > second version - after some thinking, I think the pager for \watch
> command should be controlled by option "pager" too.  When the pager is
> disabled on psql level, then the pager will not be used for \watch too.
>
> Makes sense.
>
> +            long        s = Min(i, 100L);
> +
> +            pg_usleep(1000L * s);
> +
> +            /*
> +             * in this moment an pager process can be only one child of
> +             * psql process. There cannot be other processes. So we can
> +             * detect end of any child process for fast detection of
> +             * pager process.
> +             *
> +             * This simple detection doesn't work on WIN32, because we
> +             * don't know handle of process created by _popen function.
> +             * Own implementation of _popen function based on
> CreateProcess
> +             * looks like overkill in this moment.
> +             */
> +            if (pagerpipe)
> +            {
> +
> +                int        status;
> +                pid_t    pid;
> +
> +                pid = waitpid(-1, &status, WNOHANG);
> +                if (pid)
> +                    break;
> +            }
> +
> +#endif
> +
>              if (cancel_pressed)
>                  break;
>
> I thought a bit about what we're really trying to achieve here.  We
> want to go to sleep until someone presses ^C, the pager exits, or a
> certain time is reached.  Here, we're waking up 10 times per second to
> check for exited child processes.  It works, but it does not spark
> joy.
>
> I thought about treating SIGCHLD the same way as we treat SIGINT: it
> could use the siglongjmp() trick for a non-local exit from the signal
> handler.  (Hmm... I wonder why that pre-existing code bothers to check
> cancel_pressed, considering it is running with
> sigint_interrupt_enabled = true so it won't even set the flag.)  It
> feels clever, but then you'd still have the repeating short
> pg_usleep() calls, for reasons described by commit 8c1a71d36f5.  I do
> not like sleep/poll loops.  Think of the polar bears.  I need to fix
> all of these, as a carbon emission offset for cfbot.
>
> Although there are probably several ways to do this efficiently, my
> first thought was: let's try sigtimedwait()!  If you block the signals
> first, you have a race-free way to wait for SIGINT (^C), SIGCHLD
> (pager exited) or a timeout you can specify.  I coded that up and it
> worked pretty nicely, but then I tried it on macOS too and it didn't
> compile -- Apple didn't implement that.  Blah.
>
> Next I tried sigwait().  That's already used in our tree, so it should
> be OK.  At first I thought that using SIGALRM to wake it up would be a
> bit too ugly and I was going to give up, but then I realised that an
> interval timer (one that automatically fires every X seconds) is
> exactly what we want here, and we can set it up just once at the start
> of do_watch() and cancel it at the end of do_watch().  With the
> attached patch you get exactly one sigwait() syscall of the correct
> duration per \watch cycle.
>
> Thoughts?  I put my changes into a separate patch for clarity, but
> they need some more tidying up.
>

yes, your solution is much better.

Pavel


> I'll look at the documentation next.
>