Horton

Thread

Commits

  1. Fix \watch's interaction with libedit on ^C.

  2. Add PSQL_WATCH_PAGER for psql's \watch command.

  3. psql: Show all query results by default

  4. Add a comment warning against use of pg_usleep() for long sleeps.

  1. proposal - psql - use pager for \watch command

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-04-19T17:27:31Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-01T20:41:42Z

    Pavel Stehule <pavel.stehule@gmail.com> 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

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-07-01T21:03:30Z

    st 1. 7. 2020 v 22:41 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Pavel Stehule <pavel.stehule@gmail.com> 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.
>

This feature is designed for specialized pagers - now only pspg can work in
this mode. But pspg is part of RH, Fedora, Debian, and it is available on
almost Unix platforms.

https://github.com/okbob/pspg

the pspg knows the psql output format of \watch statement.

The usability of this combination - psql \watch and pspg is really good.

Regards

Pavel


>                         regards, tom lane
>

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-01-08T09:35:28Z

    ne 19. 4. 2020 v 19:27 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

> 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
>

rebase

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-02-16T01:49:11Z

    On Fri, Jan 8, 2021 at 10:36 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> ne 19. 4. 2020 v 19:27 odesílatel Pavel Stehule <pavel.stehule@gmail.com> 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

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-02-18T08:16:35Z

    Hi

út 16. 2. 2021 v 2:49 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Fri, Jan 8, 2021 at 10:36 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > ne 19. 4. 2020 v 19:27 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> 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!
>

Thank you :)


>          * 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.
>

I'll look there.


> Sometimes I see a message like this after I quit pspg:
>
> postgres=# \watch 10
> input stream was closed
>

This is a pspg's message. It's a little bit strange, because this message
comes from event reading, and in the end, the pspg doesn't read events. So
it looks like the pspg issue, and I have to check it.

I have one question - now, the pspg has to do complex heuristics to detect
an start and an end of data in an stream. Can we, in this case (when
PSQL_WATCH_PAGER is active), use invisible chars STX and ETX or maybe ETB?
It can be a special \pset option. Surely, the detection of these chars
should be much more robust than current pspg's heuristics.

Regards

Pavel

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-03T18:16:47Z

    Hi

út 16. 2. 2021 v 2:49 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Fri, Jan 8, 2021 at 10:36 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > ne 19. 4. 2020 v 19:27 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> 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.
>

fixed


> +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.
>
>
I rewrote this mechanism (it was broken, because the timing of SIGPIPE is
different, then I expected). An implementation can be significantly simpler
- just detect with waitpid any closed child and react. You proposed it.

Sometimes I see a message like this after I quit pspg:
>
> postgres=# \watch 10
> input stream was closed
>

I don't see this message. But I use fresh 4.3 pspg

please, see attached patch

Regards

Pavel

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-04T06:37:04Z

    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

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-04T10:28:16Z

    Hi

čt 4. 3. 2021 v 7:37 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

> 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.
>
>
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.

Regards

Pavel


> Regards
>
> Pavel
>
>

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-20T22:44:46Z

    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.

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-20T23:41:54Z

    On Sun, Mar 21, 2021 at 11:44 AM Thomas Munro <thomas.munro@gmail.com> 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).

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-21T09:37:36Z

    ne 21. 3. 2021 v 0:42 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Sun, Mar 21, 2021 at 11:44 AM Thomas Munro <thomas.munro@gmail.com>
> 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.
>

I  do not have a Mac, so I never tested these features there. Surelly,
something is wrong, but I have no idea what.

1. pspg call timeout function with value 1000. So maximal waiting time
anywhere should be 1 sec

2. For this case, the pool function should be called, and timeout is
detected from the result value of the pool function.

So it looks like the pool function has a little bit different behavior than
I expect.

Can somebody help me (with access on macos0 with debugging this issue?

Regards

Pavel




> 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).
>

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-21T10:43:10Z

    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.
>

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-22T03:55:16Z

    On Sun, Mar 21, 2021 at 10:38 PM Pavel Stehule <pavel.stehule@gmail.com> 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.

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-22T04:09:43Z

    po 22. 3. 2021 v 4:55 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Sun, Mar 21, 2021 at 10:38 PM Pavel Stehule <pavel.stehule@gmail.com>
> 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.
>

Thank you.

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.

Regards

Pavel

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-22T12:13:11Z

    On Mon, Mar 22, 2021 at 5:10 PM Pavel Stehule <pavel.stehule@gmail.com> 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.

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-22T12:52:47Z

    po 22. 3. 2021 v 13:13 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Mon, Mar 22, 2021 at 5:10 PM Pavel Stehule <pavel.stehule@gmail.com>
> 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");
>

it is hell.

Please, can you verify this fix?


> 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"

[pavel@localhost pspg-master]$ ./pspg --version
pspg-4.4.0
with readline (version: 0x0801)
with integrated menu
ncurses version: 6.2, patch: 20200222
ncurses with wide char support
ncurses widechar num: 1
wchar_t width: 4, max: 2147483647
with inotify support

This is not too critical for pspg, because all commands are basic ascii
chars. Strings are taken by readline library or by wgetnstr function

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-22T21:07:07Z

    On Tue, Mar 23, 2021 at 1:53 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> po 22. 3. 2021 v 13:13 odesílatel Thomas Munro <thomas.munro@gmail.com> 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:

https://www.mail-archive.com/bug-gnulib@gnu.org/msg00296.html

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.

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-22T23:35:09Z

    On Sun, Mar 21, 2021 at 11:43 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> so 20. 3. 2021 v 23:45 odesílatel Thomas Munro <thomas.munro@gmail.com> napsal:
>> Thoughts?  I put my changes into a separate patch for clarity, but
>> they need some more tidying up.
>
> yes, your solution is much better.

Hmm, there was a problem with it though: it blocked ^C while running
the query, which is bad.  I fixed that.   I did some polishing of the
code and some editing on the documentation and comments.  I disabled
the feature completely on Windows, because it seems unlikely that
we'll be able to know if it even works, in this cycle.

-       output = PageOutput(158, pager ? &(pset.popt.topt) : NULL);
+       output = PageOutput(160, pager ? &(pset.popt.topt) : NULL);

What is that change for?

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-23T04:25:57Z

    po 22. 3. 2021 v 22:07 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Tue, Mar 23, 2021 at 1:53 AM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > po 22. 3. 2021 v 13:13 odesílatel Thomas Munro <thomas.munro@gmail.com>
> 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:
>
> https://www.mail-archive.com/bug-gnulib@gnu.org/msg00296.html
>
> 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 :-)
>

Unfortunately, I have no exact evidence. My original implementation was
very primitive

if (freopen("/dev/tty", "rw", stdin) == NULL)
{
fprintf(stderr, "cannot to reopen stdin: %s\n", strerror(errno));
exit(1);
}

Some people reported problems, but I don't know if these issues was related
to tty or to freopen

In some discussion I found a workaround with reusing stdout and stderr -
and then this works well, but I have no feedback about these fallback
cases. And because this strategy is used by "less" pager too, I expect so
this is a common and widely used workaround.

I remember so there was a problems with cygwin and some unix platforms (but
maybe very old) there was problem in deeper nesting - some like

screen -> psql -> pspg.

Directly pspg was working, but it didn't work from psql. Probably somewhere
the implementation of pty was not fully correct.



>
> > 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).
>

Thank you for verification. I fixed it in master branch


> >> 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.
>

It is the job of ncursesw -  pspg sends data to ncurses in original format
- it does only some game with attributes.

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-23T05:30:49Z

    út 23. 3. 2021 v 0:35 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Sun, Mar 21, 2021 at 11:43 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > so 20. 3. 2021 v 23:45 odesílatel Thomas Munro <thomas.munro@gmail.com>
> napsal:
> >> Thoughts?  I put my changes into a separate patch for clarity, but
> >> they need some more tidying up.
> >
> > yes, your solution is much better.
>
> Hmm, there was a problem with it though: it blocked ^C while running
> the query, which is bad.  I fixed that.   I did some polishing of the
> code and some editing on the documentation and comments.  I disabled
> the feature completely on Windows, because it seems unlikely that
> we'll be able to know if it even works, in this cycle.
>
> -       output = PageOutput(158, pager ? &(pset.popt.topt) : NULL);
> +       output = PageOutput(160, pager ? &(pset.popt.topt) : NULL);
>
> What is that change for?
>

This is correct - this is the number of printed rows - it is used for
decisions about using a pager for help. There are two new rows, and the
number is correctly +2

Pavel

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-03-23T17:09:48Z

    po 22. 3. 2021 v 13:13 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Mon, Mar 22, 2021 at 5:10 PM Pavel Stehule <pavel.stehule@gmail.com>
> 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.
>

For record, this issue is fixed in pspg 4.5.0.

Regards

Pavel

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-07T23:37:36Z

    Here's a rebase, due to a conflict with 3a513067 "psql: Show all query
results by default" which moved a few things around making it harder
to use the pager for the right scope.  Lacking time, I came up with
this change to PSQLexecWatch():

+       if (printQueryFout)
+       {
+               restoreQueryFout = pset.queryFout;
+               pset.queryFout = printQueryFout;
+       }
+
        SetCancelConn(pset.db);
        res = SendQueryAndProcessResults(query, &elapsed_msec, true);
        ResetCancelConn();

        fflush(pset.queryFout);

+       if (restoreQueryFout)
+               pset.queryFout = restoreQueryFout;
+

If someone has a tidier way to factor this, I'm keen to hear it.  I'd
like to push this today.

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-04-21T06:32:48Z

    Hi

čt 8. 4. 2021 v 1:38 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> Here's a rebase, due to a conflict with 3a513067 "psql: Show all query
> results by default" which moved a few things around making it harder
> to use the pager for the right scope.  Lacking time, I came up with
> this change to PSQLexecWatch():
>
> +       if (printQueryFout)
> +       {
> +               restoreQueryFout = pset.queryFout;
> +               pset.queryFout = printQueryFout;
> +       }
> +
>         SetCancelConn(pset.db);
>         res = SendQueryAndProcessResults(query, &elapsed_msec, true);
>         ResetCancelConn();
>
>         fflush(pset.queryFout);
>
> +       if (restoreQueryFout)
> +               pset.queryFout = restoreQueryFout;
> +
>
> If someone has a tidier way to factor this, I'm keen to hear it.  I'd
> like to push this today.
>

here is an rebase of Thomas's implementation

Regards

Pavel

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-21T06:48:52Z

    On Wed, Apr 21, 2021 at 6:33 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> here is an rebase of Thomas's implementation

Thanks.  I finished up not committing that one for 14 because I wasn't
sure about the way to rebase it on top of 3a513067 (now reverted);
that "restore" stuff seemed a bit weird.  Let's try again in v15 CF1!

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-04-21T06:52:05Z

    st 21. 4. 2021 v 8:49 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Wed, Apr 21, 2021 at 6:33 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > here is an rebase of Thomas's implementation
>
> Thanks.  I finished up not committing that one for 14 because I wasn't
> sure about the way to rebase it on top of 3a513067 (now reverted);
> that "restore" stuff seemed a bit weird.  Let's try again in v15 CF1!
>

Understand. Thank you

Pavel

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-05-12T10:25:00Z

    Hi

st 21. 4. 2021 v 8:52 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

>
>
> st 21. 4. 2021 v 8:49 odesílatel Thomas Munro <thomas.munro@gmail.com>
> napsal:
>
>> On Wed, Apr 21, 2021 at 6:33 PM Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> > here is an rebase of Thomas's implementation
>>
>> Thanks.  I finished up not committing that one for 14 because I wasn't
>> sure about the way to rebase it on top of 3a513067 (now reverted);
>> that "restore" stuff seemed a bit weird.  Let's try again in v15 CF1!
>>
>
> Understand. Thank you
>

rebase

Regards

Pavel


> Pavel
>
>

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-05-12T12:14:29Z

    st 12. 5. 2021 v 12:25 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

> Hi
>
> st 21. 4. 2021 v 8:52 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> napsal:
>
>>
>>
>> st 21. 4. 2021 v 8:49 odesílatel Thomas Munro <thomas.munro@gmail.com>
>> napsal:
>>
>>> On Wed, Apr 21, 2021 at 6:33 PM Pavel Stehule <pavel.stehule@gmail.com>
>>> wrote:
>>> > here is an rebase of Thomas's implementation
>>>
>>> Thanks.  I finished up not committing that one for 14 because I wasn't
>>> sure about the way to rebase it on top of 3a513067 (now reverted);
>>> that "restore" stuff seemed a bit weird.  Let's try again in v15 CF1!
>>>
>>
>> Understand. Thank you
>>
>
> rebase
>

looks so with your patch psql doesn't work on ms

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.134648

Regards

Pavel


> Regards
>
> Pavel
>
>
>> Pavel
>>
>>

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

    vignesh C <vignesh21@gmail.com> — 2021-07-10T13:18:28Z

    On Wed, May 12, 2021 at 5:45 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> st 12. 5. 2021 v 12:25 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
>>
>> Hi
>>
>> st 21. 4. 2021 v 8:52 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
>>>
>>>
>>>
>>> st 21. 4. 2021 v 8:49 odesílatel Thomas Munro <thomas.munro@gmail.com> napsal:
>>>>
>>>> On Wed, Apr 21, 2021 at 6:33 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>>> > here is an rebase of Thomas's implementation
>>>>
>>>> Thanks.  I finished up not committing that one for 14 because I wasn't
>>>> sure about the way to rebase it on top of 3a513067 (now reverted);
>>>> that "restore" stuff seemed a bit weird.  Let's try again in v15 CF1!
>>>
>>>
>>> Understand. Thank you
>>
>>
>> rebase
>
>
> looks so with your patch psql doesn't work on ms
>
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.134648

I am changing the status to "Waiting on Author" as Pavel's comments
are not addressed.

Regards,
Vignesh

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-07-11T22:58:53Z

    On Sun, Jul 11, 2021 at 1:18 AM vignesh C <vignesh21@gmail.com> wrote:
> On Wed, May 12, 2021 at 5:45 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > looks so with your patch psql doesn't work on ms

Here's a fix for Windows.  The pqsignal() calls are #ifdef'd out.  I
also removed a few lines that were added after commit 3a513067 but
aren't needed anymore after fae65629.

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

    vignesh C <vignesh21@gmail.com> — 2021-07-12T16:11:51Z

    On Mon, Jul 12, 2021 at 4:29 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 1:18 AM vignesh C <vignesh21@gmail.com> wrote:
> > On Wed, May 12, 2021 at 5:45 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > > looks so with your patch psql doesn't work on ms
>
> Here's a fix for Windows.  The pqsignal() calls are #ifdef'd out.  I
> also removed a few lines that were added after commit 3a513067 but
> aren't needed anymore after fae65629.

Thanks for fixing the comments, CFbot also passes for the same. I have
changed the status back to "Ready for Committer".

Regards,
Vignesh

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-07-12T20:20:20Z

    po 12. 7. 2021 v 18:12 odesílatel vignesh C <vignesh21@gmail.com> napsal:

> On Mon, Jul 12, 2021 at 4:29 AM Thomas Munro <thomas.munro@gmail.com>
> wrote:
> >
> > On Sun, Jul 11, 2021 at 1:18 AM vignesh C <vignesh21@gmail.com> wrote:
> > > On Wed, May 12, 2021 at 5:45 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > > > looks so with your patch psql doesn't work on ms
> >
> > Here's a fix for Windows.  The pqsignal() calls are #ifdef'd out.  I
> > also removed a few lines that were added after commit 3a513067 but
> > aren't needed anymore after fae65629.
>
> Thanks for fixing the comments, CFbot also passes for the same. I have
> changed the status back to "Ready for Committer".
>

I tested this version with the last release and with a developing version
of pspg, and it works very well.

Regards

Pavel


> Regards,
> Vignesh
>

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-07-13T00:01:15Z

    On Tue, Jul 13, 2021 at 8:20 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> po 12. 7. 2021 v 18:12 odesílatel vignesh C <vignesh21@gmail.com> napsal:
>> Thanks for fixing the comments, CFbot also passes for the same. I have
>> changed the status back to "Ready for Committer".
>
> I tested this version with the last release and with a developing version of pspg, and it works very well.

Pushed, after retesting on macOS (with the fixed pspg that has by now
arrived in MacPorts), FreeBSD and Linux.  Thanks!  I'm using this to
monitor system views when demoing new features in development, it's
nice.  Of course, I don't like the default theme, it's a bit too
MS-DOS/Norton for my taste, but the quieter themes are good :-)

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-07-13T04:34:33Z

    út 13. 7. 2021 v 2:01 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Tue, Jul 13, 2021 at 8:20 AM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > po 12. 7. 2021 v 18:12 odesílatel vignesh C <vignesh21@gmail.com>
> napsal:
> >> Thanks for fixing the comments, CFbot also passes for the same. I have
> >> changed the status back to "Ready for Committer".
> >
> > I tested this version with the last release and with a developing
> version of pspg, and it works very well.
>
> Pushed, after retesting on macOS (with the fixed pspg that has by now
> arrived in MacPorts), FreeBSD and Linux.  Thanks!  I'm using this to
> monitor system views when demoing new features in development, it's
> nice.  Of course, I don't like the default theme, it's a bit too
> MS-DOS/Norton for my taste, but the quieter themes are good :-)
>

I have a different feeling - I cannot write with an editor without a blue
background from my beginning with Turbo Pascal 5.5 :-), and although I
spent a lot of hours on creating and tuning themes for the pspg, I still
prefer the mc theme, and some themes look really nice. But I cannot work
with it.

Thank you very much for your work - I hope so this will be an interesting
feature of psql

Pavel

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-13T17:50:38Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> Pushed, after retesting on macOS (with the fixed pspg that has by now
> arrived in MacPorts), FreeBSD and Linux.  Thanks!

After playing with this along the way to fixing the sigwait issues,
I have a gripe/suggestion.  If I hit control-C while the thing
is waiting between queries, eg

regression=# select now() \watch
Tue Jul 13 13:44:44 2021 (every 2s)

              now              
-------------------------------
 2021-07-13 13:44:44.396565-04
(1 row)

Tue Jul 13 13:44:46 2021 (every 2s)

              now              
-------------------------------
 2021-07-13 13:44:46.396572-04
(1 row)

^Cregression=# 

then as you can see I get nothing but the "^C" echo before the next
psql prompt.  The problem with this is that now libreadline is
misinformed about the cursor position, messing up any editing I
might try to do on the next line of input.  So I think it would
be a good idea to have some explicit final output when the \watch
command terminates, along the line of

...
Tue Jul 13 13:44:46 2021 (every 2s)

              now              
-------------------------------
 2021-07-13 13:44:46.396572-04
(1 row)

^C\watch cancelled
regression=# 

This strikes me as a usability improvement even without the
readline-confusion angle.

			regards, tom lane

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2021-07-13T18:05:32Z

    út 13. 7. 2021 v 19:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Thomas Munro <thomas.munro@gmail.com> writes:
> > Pushed, after retesting on macOS (with the fixed pspg that has by now
> > arrived in MacPorts), FreeBSD and Linux.  Thanks!
>
> After playing with this along the way to fixing the sigwait issues,
> I have a gripe/suggestion.  If I hit control-C while the thing
> is waiting between queries, eg
>
> regression=# select now() \watch
> Tue Jul 13 13:44:44 2021 (every 2s)
>
>               now
> -------------------------------
>  2021-07-13 13:44:44.396565-04
> (1 row)
>
> Tue Jul 13 13:44:46 2021 (every 2s)
>
>               now
> -------------------------------
>  2021-07-13 13:44:46.396572-04
> (1 row)
>
> ^Cregression=#
>
> then as you can see I get nothing but the "^C" echo before the next
> psql prompt.  The problem with this is that now libreadline is
> misinformed about the cursor position, messing up any editing I
> might try to do on the next line of input.  So I think it would
> be a good idea to have some explicit final output when the \watch
> command terminates, along the line of
>
> ...
> Tue Jul 13 13:44:46 2021 (every 2s)
>
>               now
> -------------------------------
>  2021-07-13 13:44:46.396572-04
> (1 row)
>
> ^C\watch cancelled
> regression=#
>
> This strikes me as a usability improvement even without the
> readline-confusion angle.
>
>
I'll look at this issue.

Pavel




>                         regards, tom lane
>

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

    Thomas Munro <thomas.munro@gmail.com> — 2022-03-24T10:04:37Z

    On Wed, Jul 14, 2021 at 6:06 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> út 13. 7. 2021 v 19:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> After playing with this along the way to fixing the sigwait issues,
>> I have a gripe/suggestion.  If I hit control-C while the thing
>> is waiting between queries, eg
>>
>> regression=# select now() \watch
>> Tue Jul 13 13:44:44 2021 (every 2s)
>>
>>               now
>> -------------------------------
>>  2021-07-13 13:44:44.396565-04
>> (1 row)
>>
>> Tue Jul 13 13:44:46 2021 (every 2s)
>>
>>               now
>> -------------------------------
>>  2021-07-13 13:44:46.396572-04
>> (1 row)
>>
>> ^Cregression=#
>>
>> then as you can see I get nothing but the "^C" echo before the next
>> psql prompt.  The problem with this is that now libreadline is
>> misinformed about the cursor position, messing up any editing I
>> might try to do on the next line of input.  So I think it would
>> be a good idea to have some explicit final output when the \watch
>> command terminates, along the line of
>>
>> ...
>> Tue Jul 13 13:44:46 2021 (every 2s)
>>
>>               now
>> -------------------------------
>>  2021-07-13 13:44:46.396572-04
>> (1 row)
>>
>> ^C\watch cancelled
>> regression=#
>>
>> This strikes me as a usability improvement even without the
>> readline-confusion angle.
>
> I'll look at this issue.

Hi Pavel,

Do you have a patch for this?

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2022-03-24T10:21:04Z

    čt 24. 3. 2022 v 11:05 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Wed, Jul 14, 2021 at 6:06 AM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > út 13. 7. 2021 v 19:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
> >> After playing with this along the way to fixing the sigwait issues,
> >> I have a gripe/suggestion.  If I hit control-C while the thing
> >> is waiting between queries, eg
> >>
> >> regression=# select now() \watch
> >> Tue Jul 13 13:44:44 2021 (every 2s)
> >>
> >>               now
> >> -------------------------------
> >>  2021-07-13 13:44:44.396565-04
> >> (1 row)
> >>
> >> Tue Jul 13 13:44:46 2021 (every 2s)
> >>
> >>               now
> >> -------------------------------
> >>  2021-07-13 13:44:46.396572-04
> >> (1 row)
> >>
> >> ^Cregression=#
> >>
> >> then as you can see I get nothing but the "^C" echo before the next
> >> psql prompt.  The problem with this is that now libreadline is
> >> misinformed about the cursor position, messing up any editing I
> >> might try to do on the next line of input.  So I think it would
> >> be a good idea to have some explicit final output when the \watch
> >> command terminates, along the line of
> >>
> >> ...
> >> Tue Jul 13 13:44:46 2021 (every 2s)
> >>
> >>               now
> >> -------------------------------
> >>  2021-07-13 13:44:46.396572-04
> >> (1 row)
> >>
> >> ^C\watch cancelled
> >> regression=#
> >>
> >> This strikes me as a usability improvement even without the
> >> readline-confusion angle.
> >
> > I'll look at this issue.
>
> Hi Pavel,
>
> Do you have a patch for this?
>

Not yet. I forgot about this issue.

Regards

Pavel

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2022-05-09T07:07:06Z

    Hi

út 13. 7. 2021 v 19:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Thomas Munro <thomas.munro@gmail.com> writes:
> > Pushed, after retesting on macOS (with the fixed pspg that has by now
> > arrived in MacPorts), FreeBSD and Linux.  Thanks!
>
> After playing with this along the way to fixing the sigwait issues,
> I have a gripe/suggestion.  If I hit control-C while the thing
> is waiting between queries, eg
>
> regression=# select now() \watch
> Tue Jul 13 13:44:44 2021 (every 2s)
>
>               now
> -------------------------------
>  2021-07-13 13:44:44.396565-04
> (1 row)
>
> Tue Jul 13 13:44:46 2021 (every 2s)
>
>               now
> -------------------------------
>  2021-07-13 13:44:46.396572-04
> (1 row)
>
> ^Cregression=#
>
> then as you can see I get nothing but the "^C" echo before the next
> psql prompt.  The problem with this is that now libreadline is
> misinformed about the cursor position, messing up any editing I
> might try to do on the next line of input.  So I think it would
> be a good idea to have some explicit final output when the \watch
> command terminates, along the line of
>
> ...
> Tue Jul 13 13:44:46 2021 (every 2s)
>
>               now
> -------------------------------
>  2021-07-13 13:44:46.396572-04
> (1 row)
>
> ^C\watch cancelled
> regression=#
>
> This strikes me as a usability improvement even without the
> readline-confusion angle.
>

here is an patch

Regards

Pavel



>                         regards, tom lane
>

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

    Thomas Munro <thomas.munro@gmail.com> — 2022-06-07T03:12:49Z

    On Mon, May 9, 2022 at 7:07 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> út 13. 7. 2021 v 19:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> ^Cregression=#
>>
>> then as you can see I get nothing but the "^C" echo before the next
>> psql prompt.  The problem with this is that now libreadline is
>> misinformed about the cursor position, messing up any editing I
>> might try to do on the next line of input.  So I think it would
>> be a good idea to have some explicit final output when the \watch
>> command terminates, along the line of
>>
>> ...
>> Tue Jul 13 13:44:46 2021 (every 2s)
>>
>>               now
>> -------------------------------
>>  2021-07-13 13:44:46.396572-04
>> (1 row)
>>
>> ^C\watch cancelled
>> regression=#
>>
>> This strikes me as a usability improvement even without the
>> readline-confusion angle.
>
> here is an patch

I played with this.  On a libedit build (tested on my Mac), an easy
way to see corruption is to run eg SELECT;, then \watch 1, then ^C,
then up-arrow to see the previous command clobber the wrong columns.
On a libreadline build (tested on my Debian box), that simple test
doesn't fail in the same way.  Though there may be some other way to
make it misbehave that would take me longer to find, it's enough for
me that libedit is befuddled by what we're doing.

Do we really need the extra text?  What about just \n, so you get:

postgres=# \watch 1
...blah blah...
^C
postgres=#

This affects all release branches too.  Should we bother to fix this
there?  For them, I think the fix is just:

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index d1ee795cb6..3a88d5d6c4 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4992,6 +4992,9 @@ do_watch(PQExpBuffer query_buf, double sleep)
        sigint_interrupt_enabled = false;
    }

+   fprintf(pset.queryFout, "\n");
+   fflush(pset.queryFout);
+
    pg_free(title);
    return (res >= 0);
 }

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-06-07T03:23:00Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, May 9, 2022 at 7:07 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> út 13. 7. 2021 v 19:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>>> ^C\watch cancelled
>>> regression=#

> Do we really need the extra text?  What about just \n, so you get:

> postgres=# \watch 1
> ...blah blah...
> ^C
> postgres=#

Fine by me.

> This affects all release branches too.  Should we bother to fix this
> there?  For them, I think the fix is just:

If we're doing something as nonintrusive as just adding a newline,
it'd probably be OK to backpatch.

The code needs a comment about why it's emitting a newline, though.
In particular, it had better explain why that should be conditional
on !pagerpipe, because that makes no sense to me.

			regards, tom lane

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

    Thomas Munro <thomas.munro@gmail.com> — 2022-06-07T04:50:06Z

    On Tue, Jun 7, 2022 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The code needs a comment about why it's emitting a newline, though.
> In particular, it had better explain why that should be conditional
> on !pagerpipe, because that makes no sense to me.

Yeah.  OK, here's my take:

+               /*
+                * If the terminal driver echoed "^C",
libedit/libreadline might be
+                * confused about the cursor position.  Therefore,
inject a newline
+                * before the next prompt is displayed.  We only do
this when not using
+                * a pager, because pagers are expected to restore the
screen to a sane
+                * state on exit.
+                */

AFAIK pagers conventionally use something like termcap ti/te[1] to
restore the screen, or equivalents in tinfo etc (likely via curses).
If we were to inject an extra newline we'd just have a blank line for
nothing.  I suppose there could be a hypothetical pager that doesn't
follow that convention, and in fact both less and pspg have a -X
option to preserve last output, but in any case I expect that pagers
disable echoing, so I don't think the ^C will make it to the screen,
and furthermore ^C isn't used for exit anyway.  Rather than speculate
about the precise details, I just said "... sane state on exit".
Pavel, do you agree?

Here's how it looks after I enter and then exit Pavel's streaming pager:

$ PSQL_WATCH_PAGER='pspg --stream' ~/install/bin/psql postgres
psql (15beta1)
Type "help" for help.

postgres=# select;
--
(1 row)

postgres=# \watch 1
postgres=#

FWIW it's the same with PSQL_WATCH_PAGER='less'.

[1] https://www.gnu.org/software/termutils/manual/termcap-1.3/html_node/termcap_39.html

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2022-06-07T05:26:17Z

    út 7. 6. 2022 v 6:50 odesílatel Thomas Munro <thomas.munro@gmail.com>
napsal:

> On Tue, Jun 7, 2022 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > The code needs a comment about why it's emitting a newline, though.
> > In particular, it had better explain why that should be conditional
> > on !pagerpipe, because that makes no sense to me.
>
> Yeah.  OK, here's my take:
>
> +               /*
> +                * If the terminal driver echoed "^C",
> libedit/libreadline might be
> +                * confused about the cursor position.  Therefore,
> inject a newline
> +                * before the next prompt is displayed.  We only do
> this when not using
> +                * a pager, because pagers are expected to restore the
> screen to a sane
> +                * state on exit.
> +                */
>
> AFAIK pagers conventionally use something like termcap ti/te[1] to
> restore the screen, or equivalents in tinfo etc (likely via curses).
> If we were to inject an extra newline we'd just have a blank line for
> nothing.  I suppose there could be a hypothetical pager that doesn't
> follow that convention, and in fact both less and pspg have a -X
> option to preserve last output, but in any case I expect that pagers
> disable echoing, so I don't think the ^C will make it to the screen,
> and furthermore ^C isn't used for exit anyway.  Rather than speculate
> about the precise details, I just said "... sane state on exit".
> Pavel, do you agree?
>

Applications designed to be used as pager are usually careful about the
final cursor position. Without it, there can be no wanted artefacts. pspg
should work in pgcli, which is a more sensitive environment than psql.

I think modern pagers like less or pspg will work in all modes correctly.
There can be some legacy pagers like "pg" or very old implementations of
"more". But we don't consider it probably (more just in comment).

Regards

Pavel



> Here's how it looks after I enter and then exit Pavel's streaming pager:
>
> $ PSQL_WATCH_PAGER='pspg --stream' ~/install/bin/psql postgres
> psql (15beta1)
> Type "help" for help.
>
> postgres=# select;
> --
> (1 row)
>
> postgres=# \watch 1
> postgres=#
>
> FWIW it's the same with PSQL_WATCH_PAGER='less'.
>
> [1]
> https://www.gnu.org/software/termutils/manual/termcap-1.3/html_node/termcap_39.html
>