Re: patch to add \watch to psql

Daniel Farina <daniel(at)heroku(dot)com> writes:
> The name \repeat has grown on me, but I haven't bothered renaming it
> for the time being. I think sameness with the familiar 'watch'
> program may not be such a big deal as I thought originally, but
> 'repeat' sounds a lot more like a kind of flow control for scripts,
> whereas \watch is more clearly for humans, which is the idea.

I think that is a very good argument not to use \repeat --- we might
conceivably want that name for something else someday.

>>> I think to make it cooperate better with psql syntax, put the \watch at
>>> the end, as a replacement for \g, like

> I have implemented some kind of multi-line support. The rough part is
> in this part of the patch:

> + if (query_buf && query_buf->len > 0)
> + {
> + /*
> + * Check that the query in query_buf has been terminated. This
> + * is mostly consistent with behavior from commands like \g.
> + * The reason this is here is to prevent terrible things from
> + * occuring from submitting incomplete input of statements
> + * like:
> + *
> + * DELETE FROM foo
> + * \watch
> + * WHERE....
> + *
> + * Wherein \watch will go ahead and send whatever has been
> + * submitted so far. So instead, insist that the user
> + * terminate the query with a semicolon to be safe.
> + */
> + if (query_buf->data[query_buf->len - 1] == ';')

I think this is simply gratuitous inconsistency. We have had \g for
many years and the number of complaints about it prematurely sending
input is nil. I see no reason to think that \watch is more dangerous
than \g. Frankly I'd remove the *entire* chunk testing the state of the
query buffer. You don't even need the emptiness test, because the check
later for PGRES_EMPTY_QUERY handles that issue more completely.

But speaking of PGRES_EMPTY_QUERY, surely the loop also ought to exit on
PGRES_FATAL_ERROR, or indeed any result other than PGRES_TUPLES_OK?

pg_usleep isn't interruptable on some platforms, so I'm not sure this
is a good idea:

+ pg_usleep(1000000 * sleep);

Probably better is something like

for (i = 0; i < sleep; i++)
{
pg_usleep(1000000L);
if (cancel_pressed)
break;
}

Also, this avoids pg_usleep's limit of 2000 sec on 32-bit platforms.
(Speaking of which, that code to limit the sleep to 1 day seems
completely pointless.)

I also concur with the complaint here
http://www.postgresql.org/message-id/CAAZKuFZxyj-RT1aeC6s0g7zM68tDLfbBM1R6HGrbbxnz80KcoA@mail.gmail.com
that allowing a minimum sleep of 0 is rather dangerous (especially so in
view of the fact that that's what strtol will probably return for bad
input, and there's no code at all here to notice a bogus option value).
Perhaps it would be better to tweak the above loop so that the sleep
quantum is 100ms or so, and force it to iterate at least once even if
"zero seconds" are requested.

Another minor question is whether we really need the time-of-day in the
banner, and if we do, whether it shouldn't be captured after receiving
the result instead of before sending the query. Also, please make the
banner translatable (just add _(), I think).

One other thought is that it might be best to redo the sigsetjmp call
in each loop, immediately before "sigint_interrupt_enabled = true;".
This avoids the risky assumption that nothing down inside PSQLExec would
change sigint_interrupt_jmp.

regards, tom lane