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

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

> On Thu, Mar 4, 2021 at 11:28 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > čt 4. 3. 2021 v 7:37 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>