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

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

čt 10. 10. 2024 v 2:02 odesílatel Michael Paquier <michael@paquier.xyz>
napsal:

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?

Linux "watch" uses just seconds. If I remember correctly the psql doesn't
use units in settings, so I prefer just the value from interval 0.1 .. 3600
* n

and the number can be rounded to 0.1

Regards

Pavel

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 Thu, Oct 10, 2024 at 09:43:27AM +0200, Daniel Gustafsson wrote:

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)

Or did I misunderstand your email?

Nope, I just got it wrong. No need to bother about my previous
message :)
--
Michael

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

On 4 Nov 2024, at 14:55, Kirill Reshke <reshkekirill@gmail.com> wrote:

I'm mostly fine with this patch,

Thanks for review!

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

Fair point, I've added ERANGE handling in the attached v3.

Also, maybe we should provide `double max` arg to the
ParseVariableDouble function, because this is a general-use function?

It's general use, but not generally used. Since it's not an exposed API it
seems premature to add handling of a max value when there is no need, once
there is a caller (if one ever comes) that needs it we can deal with it at that
point.

--
Daniel Gustafsson

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

On 19 Nov 2024, at 11:20, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

I've tested it and reviewed the patch. I'd like to provide some feedback.

Thank you very much for your review and I do apologize for the late response.

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))
| ^~~~~~~~

Odd, I don't see that, but I've nonetheless fixed it by including math.h

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 see your point, especially since ParseVariableBuffer is only used for this at
the moment. It is however intended as generic functionality and would require
to diverge from the other ParseVariableXXX to support custom error messages.
I'm not sure it's worth the added complexity for something which is likely to
be a quite niche feature.

ParseVariableDouble() doesn't have a comment yet, though you may be planning to add one later.

Fixed.

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.

I don't think this improves the documentation. The patch only changes the
defult interval which is used if the interval is omitted from the command, the
above sections discuss how interval is handled when specified. I did some
minor tweaks to the docs to try and clarify things.

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

Fair point, fixed.

The attached v4 fixes the above and also adds tests.

--
Daniel Gustafsson

On 2 Feb 2025, at 19:25, Daniel Gustafsson <daniel@yesql.se> wrote:

Attached is rebase with some small levels of polish, unless objected to I would
like to go ahead with this version.

--
Daniel Gustafsson

On Fri, 2025-02-14 at 16:15 +0100, Daniel Gustafsson wrote:

On 2 Feb 2025, at 19:25, Daniel Gustafsson <daniel@yesql.se> wrote:

Attached is rebase with some small levels of polish, unless objected to I would
like to go ahead with this version.

I gave the patch a try, and it works as expected.
The code looks good to me.

Yours,
Laurenz Albe

Patch looks good. One minor issue:

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

We should disallow 0 as the error message implies

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.

Curious what other's personal defaults are? I usually use 1 second or 0.5
depending on things.

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

On 13 Mar 2025, at 15:03, Greg Sabino Mullane <htamfids@gmail.com> wrote:

Patch looks good. One minor issue:

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

We should disallow 0 as the error message implies

Ah, nice catch, fixed in the attached along with a test for the minimum bound (ie zero).

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.

Curious what other's personal defaults are? I usually use 1 second or 0.5 depending on things.

I rarely use anything higher than 0.5.

--
Daniel Gustafsson



New patch looks good. TIL I learned you can even use things like

\set WATCH_INTERVAL 0xAp-4

:)

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

On Fri, Mar 14, 2025 at 09:11:15AM -0400, Greg Sabino Mullane wrote:

New patch looks good. TIL I learned you can even use things like

\set WATCH_INTERVAL 0xAp-4

:)

You have taste.
--
Michael

Hi Daniel,

On Fri, Mar 14, 2025 at 2:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 13 Mar 2025, at 15:03, Greg Sabino Mullane <htamfids@gmail.com> wrote:

Patch looks good. One minor issue:

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

We should disallow 0 as the error message implies

Ah, nice catch, fixed in the attached along with a test for the minimum bound (ie zero).

#\watch c=4 i=
Mon 17 Mar 2025 05:52:50 PM IST (every 0s)

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

Mon 17 Mar 2025 05:52:50 PM IST (every 0s)

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

Mon 17 Mar 2025 05:52:50 PM IST (every 0s)

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

Mon 17 Mar 2025 05:52:50 PM IST (every 0s)

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

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

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.

--
Best Wishes,
Ashutosh Bapat

On 17 Mar 2025, at 13:37, Ashutosh Bapat <ashutosh.bapat.oss@gmail.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.

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.

--
Daniel Gustafsson

On Thu, Mar 20, 2025 at 4:45 PM Daniel Gustafsson <daniel@yesql.se> wrote:

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.

Yeah, I forgot about that too. The new patch looks good except for one
tiny little thing: the error should say "must be at least 0.00" or similar,
instead of "must be greater than 0.00" now that we allow 0.00

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support