Hi,
Thanks for developing this useful feature!
I've tested it and reviewed the patch. I'd like to provide some
feedback.
(1)
I tested with v3 patch and found the following compile error.
It seems that math.h needs to be included in variables.c.
variables.c: In function 'ParseVariableDouble':
variables.c:220:54: error: 'HUGE_VAL' undeclared (first use in this
function)
220 | (dblval == 0.0 || dblval >= HUGE_VAL ||
dblval <= -HUGE_VAL))
| ^~~~~~~~
variables.c:220:54: note: each undeclared identifier is reported only
once for each function it appears in
variables.c:232:1: warning: control reaches end of non-void function
[-Wreturn-type]
232 | }
| ^
(2)
Although the error handling logic is being discussed now, I think it
would be better,
at least, to align the logic and messages of exec_command_watch() and
ParseVariableDouble().
I understand that the error message 'for "WATCH_INTERVAL"' will remain
as a difference
since it should be considered when loaded via psqlrc.
# v3 patch test result
* minus value
=# \watch i=-1
\watch: incorrect interval value "-1"
=# \set WATCH_INTERVAL -1
invalid value "-1" for "WATCH_INTERVAL": must be at least 0.00
* not interval value
=# \watch i=1s
\watch: incorrect interval value "1s"
=# \set WATCH_INTERVAL 1s
invalid value "1s" for "WATCH_INTERVAL"
* maximum value
=# \watch i=1e500
\watch: incorrect interval value "1e500"
=# \set WATCH_INTERVAL 1e500
"1e500" is out of range for "WATCH_INTERVAL"
(3)
ParseVariableDouble() doesn't have a comment yet, though you may be
planning to add one later.
(4)
I believe the default value is 2 after the WATCH_INTERVAL is specified
because \unset
WATCH_INTERVAL sets it to '2'. So, why not update the following sentence
accordingly?
- of rows. Wait the specified number of seconds (default 2)
between executions.
- For backwards compatibility,
+ of rows. Wait the specified number of seconds (default 2, which
can be
+ changed with the variable
+ between executions. For backwards compatibility,
For example,
> Wait <varname>WATCH_INTERVAL</varname> seconds unless the interval
> option is specified.
> If the interval option is specified, wait the specified number of
> seconds instead.
+ This variable sets the default interval which
<command>\watch</command>
+ waits between executing the query. Specifying an interval in
the
+ command overrides this variable.
> This variable sets the interval in seconds that
> <command>\watch</command> waits
> between executions. The default value is 2.0.
(5)
I think it's better to replace queries with executions because the
\watch
documentation says so.
+ HELP0(" WATCH_INTERVAL\n"
+ " number of seconds \\watch waits between queries\n");
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
