Re: Allow default \watch interval in psql to be configured

On Fri, Mar 21, 2025 at 2:15 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 17 Mar 2025, at 13:37, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> > 0 is an accepted value for interval, even though it might look insensible.
> >
> > The behaviour should be same in both cases \watch i=<some value> and
> > \set WATCH_INTERVAL <some value> \watch. In this case it's not.
>
> Having a watch interval of zero is IMHO somewhat nonsensical, but since it was
> done intentionally in 6f9ee74d45 (which I had missed) I agree that the default
> should support it as well. Fixed.
>
> > In fact, we should use the same validation code in both the cases. Why
> > don't we perform the same additional validations in
> > ParseVariableDouble() in exec_watch_command() as well?
>
> Well, they don't use the same code since they are two different things
> (variables and command input, while using the same syscalls they have different
> errorhandling requirements). If executing \watch would use ParseVariableDouble
> it would make errorhandling quite awkward at best. I added a comment in
> exec_command_watch that any changes in internval parsing should take default
> intervals into consideration.

There are following differences between command input parsing and
variable value parsing
1. empty string is considered as 0 value while parsing command input
whereas it wiil cause error when setting to a variable.
#\watch c=1 i=
Fri 21 Mar 2025 08:27:25 AM IST (every 0s)

?column?
----------
1
(1 row)
#\set WATCH_INTERVAL
invalid input syntax for "WATCH_INTERVAL"

That can be considered as an existing bug and maybe fixed later.

2. The maximum value that can be specified as command input is limited
by what strtod can handle but for variable it's 1000000 which is 15
days. I can't imagine someone would want to set default value higher
than that but we need to be prepared to change it if a request comes.
Or increase it to a much higher value like seconds worth 100 years.

3. ParseVariableDouble() gives better error message when strtod() can
not handle the input string by looking at the output as well as errno.
But exec_command_watch() lacks that finesse.

ParseVariableDouble is better at parsing the input string than
exec_command_watch(). But that's something that should have been
tackled when exec_command_watch()'s parsing code was written instead
of ParseVariableDouble().

>
> > The test only validate default variable value. We need a test where we
> > see variable value being honored lie tests between 369 to 376 in the
> > same file.
>
> I'm not sure it's worth spending test cycles on as this code doesn't affect the
> execution of \watch at all. That being said, I added a testcase which sets a
> default and then executes \watch. It doesn't test that the interval was
> correct (we don't do that for any), but at least it will catch if setting a
> default totally breaks \watch.
>

This looks ok. We do both test 0 as well as the variable.

With this patch, we are doing something unprecedented (at least
AFAIK); allowing command arguments defaults to be configurable through
a psql variable (as against an environment variable). I admit that
configurable through a psql variable is better since it doesn't meddle
with environment. Glancing through psql documentation, I didn't find a
lot of command which may need default argument to be configurable.
Nonetheless we should mention why this is special and set some
guidance for such future additions - preferrably in code or at least
in the commit message.

- of rows. Wait the specified number of seconds (default 2) between executions.
- For backwards compatibility,
+ of rows. Wait the specified number of seconds (defaults to 2 if
omitted, which can be
+ changed with the variable <xref linkend="app-psql-variables-watch-interval"/>)
+ between executions. For backwards compatibility,

The text in parenthesis is quite long and it's hard to read ...
seconds between execution. I suggest
"Wait the specified number of seconds (default 2) between executions.
The default wait can be changed by setting the variable <xref
linkend="app-psql-variables-watch-interval"/>."

+ " number of seconds \\watch waits between executing the query buffer\n");

I am feeling that this should mention "default" somewhere - maybe just
add it before "number of ".

--
Best Wishes,
Ashutosh Bapat