Re: proposal - psql - use pager for \watch command - Mailing list pgsql-hackers

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.

I'll look at the documentation next.