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

On 09/10/2024 16:08, Daniel Gustafsson wrote:
> Scratching an old itch; I've long wanted to be able to set the default interval
> for \watch in psql since I almost never want a 2 second wait.  The interval can
> of course be set by passing it to \watch but it's handy during testing and
> debugging to save that with just quick \watch.
> 
> The attached adds a new variable, WATCH_INTERVAL, which is used as the default
> value in case no value is defined when executing the command.  The defualt of
> this remains at 2 seconds as it is now.  The count and min_rows values are not
> affected by this as those seem less meaningful to set defaults on.

../src/bin/psql/startup.c:953:80: error: too many arguments to function 
call, expected 4, have 5
         return ParseVariableDouble(newval, "WATCH_INTERVAL", 
&pset.watch_interval, 0, 1000);
                ~~~~~~~~~~~~~~~~~~~ 
               ^~~~
../src/bin/psql/variables.h:84:7: note: 'ParseVariableDouble' declared here
bool            ParseVariableDouble(const char *value, const char *name,
                 ^
1 error generated.

I guess the '1000' was supposed to be the maximum, but 
ParseVariableDouble doesn't take a maximum.

After fixing that by removing the '1000' argument:


postgres=# \set WATCH_INTERVAL -10
invalid value "-10" for "WATCH_INTERVAL": must be greater than 0.00

That's a little inaccurate: 0 is also accepted, so should be "must be 
greater than *or equal to* 0". Or maybe "cannot be negative". -0 is also 
accepted, though.

> +        This variable set the default interval which <command>\watch</command>

set -> sets

> +       HELP0("  WATCH_INTERVAL\n"
> +                 "    number of seconds \\watch waits beetween queries\n");

beetween -> between

-- 
Heikki Linnakangas
Neon (https://neon.tech)

> On 10 Oct 2024, at 02:01, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Oct 09, 2024 at 04:24:27PM +0200, Daniel Gustafsson wrote:
>> Fixed.
>
> -        double        sleep = 2;
> +        double        sleep = pset.watch_interval;
>
> This forces the use of seconds as unit.  The interval values I have
> been using a lot myself are between 0.2s and 0.5s because I usually
> want a lot more granularity in my lookups than the 1s interval.  Could
> it be better to allow values lower than 1s or let this value be a
> string with optional "s" or "ms" units?

I'm not sure I follow, it's true that the unit is seconds but the patch doesn't
change the ability to use fractions of a second that we already support today.

db=# \echo :WATCH_INTERVAL
2
db=# \set WATCH_INTERVAL 0.1
db=# \echo :WATCH_INTERVAL
0.1
db=# select 1;
 ?column?
----------
        1
(1 row)

danielg=# \watch
Thu Oct 10 09:32:05 2024 (every 0.1s)

 ?column?
----------
        1
(1 row)

Thu Oct 10 09:32:05 2024 (every 0.1s)

 ?column?
----------
        1
(1 row)

Or did I misunderstand your email?

We could support passing in an optional unit, and assume the unit to be seconds
if none was used, but it doesn't really fit nicely with the current API we have
so I wonder if the added complexity is worth it?

--
Daniel Gustafsson

On Wed, 9 Oct 2024 at 19:24, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 9 Oct 2024, at 16:05, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> Thanks for looking!
>
> > I guess the '1000' was supposed to be the maximum, but ParseVariableDouble doesn't take a maximum.
>
> Doh, I had a max parameter during hacking but removed it since I didn't see a
> clear usecase for it.  Since it's not an externally published API we can
> alwasys add it when there is need.  Clearly I managed to generate the patch at
> the wrong time without noticing.  Fixed.
>
> > That's a little inaccurate: 0 is also accepted, so should be "must be greater than *or equal to* 0". Or maybe
"cannotbe negative". -0 is also accepted, though.
 
>
> I changed to "must be at least XX" to keep the message short.
>
> > set -> sets
> >
> > beetween -> between
>
> Fixed.
>
> --
> Daniel Gustafsson
>

Hi!
I'm mostly fine with this patch, but maybe we need to handle `errno ==
ERANGE` inside ParseVariableDouble and provide a better error msg in
this case?
Something like:
```
reshke=# \set WATCH_INTERVAL -1e-309
underflow while parsing parameter
```
Also, maybe we should provide `double max` arg to the
ParseVariableDouble function, because this is a general-use function?


-- 
Best regards,
Kirill Reshke

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