proposal - psql - use pager for \watch command

Pavel Stehule < > writes:
> I propose new psql environment variable PSQL_WATCH_PAGER. When this
> variable is not empty, then \watch command starts specified pager, and
> redirect output to related pipe. When pipe is closed - by pager, then
> \watch cycle is leaved.

I dunno, this just seems really strange.  With any normal pager,
you'd get completely unusable behavior (per the comments that you
didn't bother to change).  Also, how would the pager know where
the boundaries between successive query outputs are?  If it does
not know, seems like that's another decrement in usability.

                        regards, tom lane

Hi,

last week I finished pspg 3.0 https://github.com/okbob/pspg . pspg now supports pipes, named pipes very well. Today the pspg can be used as pager for output of \watch command. Sure, psql needs attached patch.

I propose new psql environment variable PSQL_WATCH_PAGER. When this variable is not empty, then \watch command starts specified pager, and redirect output to related pipe. When pipe is closed - by pager, then \watch cycle is leaved.

If you want to test proposed feature, you need a pspg with cb4114f98318344d162a84b895a3b7f8badec241 commit.

Then you can set your env

export PSQL_WATCH_PAGER="pspg --stream"

psql

 

SELECT * FROM pg_stat_database;

\watch 1

Comments, notes?

Regards

Pavel

On Fri, Jan 8, 2021 at 10:36 PM Pavel Stehule < > wrote:
> ne 19. 4. 2020 v 19:27 odesílatel Pavel Stehule < > napsal:
>> last week I finished pspg 3.0 https://github.com/okbob/pspg . pspg now supports pipes, named pipes very well. Today the pspg can be used as pager for output of \watch command. Sure, psql needs attached patch.
>>
>> I propose new psql environment variable PSQL_WATCH_PAGER. When this variable is not empty, then \watch command starts specified pager, and redirect output to related pipe. When pipe is closed - by pager, then \watch cycle is leaved.
>>
>> If you want to test proposed feature, you need a pspg with cb4114f98318344d162a84b895a3b7f8badec241 commit.
>>
>> Then you can set your env
>>
>> export PSQL_WATCH_PAGER="pspg --stream"
>> psql
>>
>> SELECT * FROM pg_stat_database;
>> \watch 1
>>
>> Comments, notes?

I tried this out with pspg 4.1 from my package manager.  It seems
really useful, especially for demos.  I like it!

         * Set up rendering options, in particular, disable the pager, because
         * nobody wants to be prompted while watching the output of 'watch'.
         */
-       myopt.topt.pager = 0;
+       if (!pagerpipe)
+               myopt.topt.pager = 0;

Obsolete comment.

+static bool sigpipe_received = false;

This should be "static volatile sig_atomic_t", and I suppose our
convention name for that variable would be got_SIGPIPE.  Would it be
possible to ignore SIGPIPE instead, and then rely on another way of
knowing that the pager has quit?  But... hmm:

-                       long            s = Min(i, 1000L);
+                       long            s = Min(i, pagerpipe ? 100L : 1000L);

I haven't studied this (preexisting) polling loop, but I don't like
it.  I understand that it's there because on some systems, pg_usleep()
won't wake up for SIGINT (^C), but now it's being used for a secondary
purpose, that I haven't fully understood.  After I quit pspg (by
pressing q) while running \watch 10, I have to wait until the end of a
10 second cycle before it tries to write to the pipe again, unless I
also press ^C.  I feel like it has to be possible to achieve "precise"
behaviour somehow when you quit; maybe something like waiting for
readiness on the pager's stderr, or something like that -- I haven't
thought hard about this and I admit that I have no idea how this works
on Windows.

Sometimes I see a message like this after I quit pspg:

postgres=# \watch 10
input stream was closed

Hi

Here is a little bit updated patch - detection of end of any child process cannot be used on WIN32. I am not an expert on this platform, but from what I read about it, there is no easy solution. The problem is in _popen function. We lost the handle of the created process, and it is not possible to find it. Writing a new implementation of _popen function looks like a big overkill to me. We can disable this functionality there completely (on win32) or we can accept the waiting time after pager has ended until we detect pipe error. I hope so this is acceptable, in this moment, because a) there are not pspg for windows (and there was only few requests for porting there in last 4 years), b) usage of psql on mswin platform is not too wide, c) in near future, there will be an possibility to use Unix psql on this platform.

Regards

Pavel

On Sun, Mar 21, 2021 at 11:44 AM Thomas Munro < > wrote:
> [review]

Oh, just BTW, to save confusion for others who might try this:  It
seems there is something wrong with pspg --stream on macOS, at least
when using MacPorts.  I assumed it might be just pspg 3.1.5 being too
old (that's what MacPorts has currently), so I didn't mention it
before, but I just built pspg from your github master branch and it
has the same symptom.  It doesn't seem to repaint the screen until you
press a key.  I can see that psql is doing its job, but pspg is
sitting in select() reached from ncurses wgetch():

  * frame #0: 0x000000019b4af0e8 libsystem_kernel.dylib`__select + 8
    frame #1: 0x0000000100ca0620 libncurses.6.dylib`_nc_timed_wait + 332
    frame #2: 0x0000000100c85444 libncurses.6.dylib`_nc_wgetch + 296
    frame #3: 0x0000000100c85b24 libncurses.6.dylib`wgetch + 52
    frame #4: 0x0000000100a815e4 pspg`get_event + 624
    frame #5: 0x0000000100a7899c pspg`main + 9640
    frame #6: 0x000000019b4f9f34 libdyld.dylib`start + 4

That's using MacPorts' libncurses.  I couldn't get it to build against
Apple's libcurses (some missing functions).  It's the same for both
your V2 and the fixup I posted.  When you press a key, it suddenly
catches up and repaints all the \watch updates that were buffered.

It works fine on Linux and FreeBSD though (I tried pspg 4.1.0 from
Debian's package manager, and pspg 4.3.1 from FreeBSD's).

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

On Sun, Mar 21, 2021 at 10:38 PM Pavel Stehule < > wrote:
> Can somebody help me (with access on macos0 with debugging this issue?

I'll try to figure it out, but maybe after the code freeze.  I started
my programming career writing curses software a million years ago on a
couple of extinct Unixes... I might even be able to remember how it
works.  This is not a problem for committing the PSQL_WATCH_PAGER
patch, I just mentioned it here because I thought that others might
try it out on a Mac and be confused.

On Mon, Mar 22, 2021 at 5:10 PM Pavel Stehule < > wrote:
> probably there will not be an issue inside ncurses - the most complex part of get_event is polling of input sources - tty and some other. The pspg should not to stop there on tty reading.

The problem is that Apple's /dev/tty device is defective, and doesn't
work in poll().  It always returns immediately with revents=POLLNVAL,
but pspg assumes that data is ready and tries to read the keyboard and
then blocks until I press a key.  This seems to fix it:

+#ifndef __APPLE__
+               /* macOS can't use poll() on /dev/tty */
                state.tty = fopen("/dev/tty", "r+");
+#endif
                if (!state.tty)
                        state.tty = fopen(ttyname(fileno(stdout)), "r");

A minor problem is that on macOS, _GNU_SOURCE doesn't seem to imply
NCURSES_WIDECHAR, so I suspect Unicode will be broken unless you
manually add -DNCURSES_WIDECHAR=1, though I didn't check.

On Tue, Mar 23, 2021 at 1:53 AM Pavel Stehule < > wrote:
> po 22. 3. 2021 v 13:13 odesílatel Thomas Munro < > napsal:
>> The problem is that Apple's /dev/tty device is defective, and doesn't
>> work in poll().  It always returns immediately with revents=POLLNVAL,
>> but pspg assumes that data is ready and tries to read the keyboard and
>> then blocks until I press a key.  This seems to fix it:
>>
>> +#ifndef __APPLE__
>> +               /* macOS can't use poll() on /dev/tty */
>>                 state.tty = fopen("/dev/tty", "r+");
>> +#endif
>>                 if (!state.tty)
>>                         state.tty = fopen(ttyname(fileno(stdout)), "r");
>
>
> it is hell.

Heh.  I've recently spent many, many hours trying to make AIO work on
macOS, and nothing surprises me anymore.  BTW I found something from
years ago on the 'net that fits with my observation about /dev/tty:



Curious, which other OS did you put that fallback case in for?  I'm a
little confused about why it works, so I'm not sure if it's the best
possible change, but I'm not planning to dig any further now, too many
patches, not enough time :-)

> Please, can you verify this fix?

It works perfectly for me on a macOS 11.2 system with that change,
repainting the screen exactly when it should.  I'm happy about that
because (1) it means I can confirm that the proposed change to psql is
working correctly on the 3 Unixes I have access to, and (2) I am sure
that a lot of Mac users will appreciate being able to use super-duper
\watch mode when this ships (a high percentage of PostgreSQL users I
know use a Mac as their client machine).

>> A minor problem is that on macOS, _GNU_SOURCE doesn't seem to imply
>> NCURSES_WIDECHAR, so I suspect Unicode will be broken unless you
>> manually add -DNCURSES_WIDECHAR=1, though I didn't check.
>
> It is possible -
>
> can you run "pspg --version"

Looks like I misunderstood: it is showing "with wide char support",
it's just that the "num" is 0 rather than 1.  I'm not planning to
investigate that any further now, but I checked that it can show the
output of SELECT 'špeĉiäl chârãçtérs' correctly.