psql \watch 2nd argument: iteration count

On 17.02.23 00:33, Andrey Borodin wrote:

From time to time I want to collect some stats from locks, activity
and other stat views into one table from different time points. In
this case the \watch psql command is very handy. However, it's not
currently possible to specify the number of times a query is
performed.

The watch command on my OS has a lot of options, but this is not one of
them. So probably no one has really needed it so far.

Also, if we do not provide a timespan, 2 seconds are selected. But if
we provide an incorrect argument - 1 second is selected.
PFA the patch that adds iteration count argument and makes timespan
argument more consistent.

That should probably be fixed.

Greetings,

* Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:

On 17.02.23 00:33, Andrey Borodin wrote:

From time to time I want to collect some stats from locks, activity
and other stat views into one table from different time points. In
this case the \watch psql command is very handy. However, it's not
currently possible to specify the number of times a query is
performed.

The watch command on my OS has a lot of options, but this is not one of
them. So probably no one has really needed it so far.

watch doesn't ... but top does, and I can certainly see how our watch
having an iterations count could be helpful in much the same way as
top's batch mode does.

Also, if we do not provide a timespan, 2 seconds are selected. But if
we provide an incorrect argument - 1 second is selected.
PFA the patch that adds iteration count argument and makes timespan
argument more consistent.

That should probably be fixed.

And should probably be independent patches.

Thanks,

Stephen

Also, if we do not provide a timespan, 2 seconds are selected. But if
we provide an incorrect argument - 1 second is selected.
PFA the patch that adds iteration count argument and makes timespan
argument more consistent.

That should probably be fixed.

And should probably be independent patches.

PFA 2 independent patches.

Also, I've fixed a place to break after an iteration. Now if we have
e.g. 2 iterations - there will be only 1 sleep time.

Thanks!

Best regards, Andrey Borodin.

At Mon, 20 Feb 2023 10:45:53 -0800, Andrey Borodin <amborodin86@gmail.com> wrote in

Also, if we do not provide a timespan, 2 seconds are selected. But if
we provide an incorrect argument - 1 second is selected.
PFA the patch that adds iteration count argument and makes timespan
argument more consistent.

That should probably be fixed.

And should probably be independent patches.

PFA 2 independent patches.

Also, I've fixed a place to break after an iteration. Now if we have
e.g. 2 iterations - there will be only 1 sleep time.

IMHO the current behavior for digit inputs looks fine to me. I feel
that the command should selently fix the input to the default in the
case of digits inputs like '-1'. But that may not be the case for
everyone. FWIW the patch still accepts an incorrect parameter '1abc'
by ignoring any trailing garbage.

In any case, I reckon the error message should be more specific. In
other words, it would be better if it suggests the expected input
format and range.

Regarding the second patch, if we want \watch to throw an error
message for the garbage trailing to sleep times, I think we should do
the same for iteration counts. Additionally, we need to update the
documentation.

By the way, when I looked this patch, I noticed that
exec_command_bind() doesn't free the malloc'ed return strings from
psql_scan_slash_option(). The same mistake is seen in some other
places. I'll take a closer look and get back in another thread.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thanks for looking into this!

On Mon, Feb 20, 2023 at 6:15 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

FWIW the patch still accepts an incorrect parameter '1abc'
by ignoring any trailing garbage.

Indeed, fixed.

In any case, I reckon the error message should be more specific. In
other words, it would be better if it suggests the expected input
format and range.

+1.
Not a range, actually, because upper limits have no sense for a user.

Regarding the second patch, if we want \watch to throw an error
message for the garbage trailing to sleep times, I think we should do
the same for iteration counts.

+1, done.

Additionally, we need to update the
documentation.

Done.

Thanks for the review!

Best regards, Andrey Borodin.

+1 for adding an iteration count argument to \watch.

+			char *opt_end;
+			sleep = strtod(opt, &opt_end);
+			if (sleep <= 0 || *opt_end)
+			{
+				pg_log_error("Watch period must be positive number, but argument is '%s'", opt);
+				free(opt);
+				resetPQExpBuffer(query_buf);
+				return PSQL_CMD_ERROR;
+			}

Is there any reason to disallow 0 for the sleep argument? I often use
commands like "\watch .1" to run statements repeatedly with very little
time in between, and I'd use "\watch 0" instead if it was available.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Mar 8, 2023 at 10:49 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

Is there any reason to disallow 0 for the sleep argument? I often use
commands like "\watch .1" to run statements repeatedly with very little
time in between, and I'd use "\watch 0" instead if it was available.

Yes, that makes sense! Thanks!

Best regards, Andrey Borodin.

+ pg_log_error("Watch period must be non-negative number, but argument is '%s'", opt);

After looking around at the other error messages in this file, I think we
should make this more concise. Maybe something like

pg_log_error("\\watch: invalid delay interval: %s", opt);

+				free(opt);
+				resetPQExpBuffer(query_buf);
+				return PSQL_CMD_ERROR;

Is this missing psql_scan_reset(scan_state)?

I haven't had a chance to look closely at 0002 yet.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Mar 9, 2023 at 11:25 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

+ pg_log_error("Watch period must be non-negative number, but argument is '%s'", opt);

After looking around at the other error messages in this file, I think we
should make this more concise. Maybe something like

pg_log_error("\\watch: invalid delay interval: %s", opt);

In the review above Kyotaro-san suggested that message should contain
information on what it expects... So, maybe then
pg_log_error("\\watch interval must be non-negative number, but
argument is '%s'", opt); ?
Or perhaps with articles? pg_log_error("\\watch interval must be a
non-negative number, but the argument is '%s'", opt);

+                               free(opt);
+                               resetPQExpBuffer(query_buf);
+                               return PSQL_CMD_ERROR;

Is this missing psql_scan_reset(scan_state)?

Yes, fixed.

Best regards, Andrey Borodin.

On Sun, Mar 12, 2023 at 01:05:39PM -0700, Andrey Borodin wrote:

In the review above Kyotaro-san suggested that message should contain
information on what it expects... So, maybe then
pg_log_error("\\watch interval must be non-negative number, but
argument is '%s'", opt); ?
Or perhaps with articles? pg_log_error("\\watch interval must be a
non-negative number, but the argument is '%s'", opt);

-       HELP0("  \\watch [SEC]           execute query every SEC seconds\n");
+       HELP0("  \\watch [SEC [N]]       execute query every SEC seconds N times\n");

Is that really the interface we'd want to work with in the long-term?
For one, this does not give the option to specify only an interval
while relying on the default number of seconds. This may be fine, but
it does not strike me as the best choice. How about doing something
more extensible, for example:
\watch [ (option=value [, option=value] .. ) ] [SEC]

I am not sure that this will be the last option we'll ever add to
\watch, so I'd rather have us choose a design more flexible than
what's proposed here, in a way similar to \g or \gx.
--
Michael

On Mon, Mar 13, 2023 at 10:17:12AM +0900, Michael Paquier wrote:

I am not sure that this will be the last option we'll ever add to
\watch, so I'd rather have us choose a design more flexible than
what's proposed here, in a way similar to \g or \gx.

While on it, I have some comments about 0001.

-    sleep = strtod(opt, NULL);
-    if (sleep <= 0)
-            sleep = 1;
+    char *opt_end;
+    sleep = strtod(opt, &opt_end);
+    if (sleep < 0 || *opt_end)
+    {
+            pg_log_error("\\watch interval must be non-negative number, "
+                                     "but argument is '%s'", opt);
+            free(opt);
+            resetPQExpBuffer(query_buf);
+            psql_scan_reset(scan_state);
+            return PSQL_CMD_ERROR;
+    }

Okay by me to make this behavior a bit better, though it is not
something I would backpatch as it can influence existing workflows,
even if they worked in an inappropriate way.

Anyway, are you sure that this is actually OK? It seems to me that
this needs to check for three things:
- If sleep is a negative value.
- errno should be non-zero.
- *opt_end == opt.

So this needs three different error messages to show the exact error
to the user? Wouldn't it be better to have a couple of regression
tests, as well?
--
Michael

Michael, thanks for reviewing this!

On Sun, Mar 12, 2023 at 6:17 PM Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Mar 12, 2023 at 01:05:39PM -0700, Andrey Borodin wrote:

In the review above Kyotaro-san suggested that message should contain
information on what it expects... So, maybe then
pg_log_error("\\watch interval must be non-negative number, but
argument is '%s'", opt); ?
Or perhaps with articles? pg_log_error("\\watch interval must be a
non-negative number, but the argument is '%s'", opt);

-       HELP0("  \\watch [SEC]           execute query every SEC seconds\n");
+       HELP0("  \\watch [SEC [N]]       execute query every SEC seconds N times\n");

Is that really the interface we'd want to work with in the long-term?
For one, this does not give the option to specify only an interval
while relying on the default number of seconds. This may be fine, but
it does not strike me as the best choice. How about doing something
more extensible, for example:
\watch [ (option=value [, option=value] .. ) ] [SEC]

I am not sure that this will be the last option we'll ever add to
\watch, so I'd rather have us choose a design more flexible than
what's proposed here, in a way similar to \g or \gx.

I've attached an implementation of this proposed interface (no tests
and help message yet, though, sorry).
I tried it a little bit, and it works for me.
fire query 3 times
SELECT 1;\watch c=3 0
or with 200ms interval
SELECT 1;\watch i=.2 c=3
nonsense, but correct
SELECT 1;\watch i=1e-100 c=1

Actually Nik was asking for the feature. Nik, what do you think?

On Sun, Mar 12, 2023 at 6:26 PM Michael Paquier <michael@paquier.xyz> wrote:

While on it, I have some comments about 0001.

-    sleep = strtod(opt, NULL);
-    if (sleep <= 0)
-            sleep = 1;
+    char *opt_end;
+    sleep = strtod(opt, &opt_end);
+    if (sleep < 0 || *opt_end)
+    {
+            pg_log_error("\\watch interval must be non-negative number, "
+                                     "but argument is '%s'", opt);
+            free(opt);
+            resetPQExpBuffer(query_buf);
+            psql_scan_reset(scan_state);
+            return PSQL_CMD_ERROR;
+    }

Okay by me to make this behavior a bit better, though it is not
something I would backpatch as it can influence existing workflows,
even if they worked in an inappropriate way.

+1

Anyway, are you sure that this is actually OK? It seems to me that
this needs to check for three things:
- If sleep is a negative value.
- errno should be non-zero.

I think we can treat errno and negative values equally.

- *opt_end == opt.

So this needs three different error messages to show the exact error
to the user?

I've tried this approach, but could not come up with sufficiently
different error messages...

Wouldn't it be better to have a couple of regression
tests, as well?

Added two tests.

Thanks!

Best regards, Andrey Borodin.

On Sun, Mar 12, 2023 at 08:59:44PM -0700, Andrey Borodin wrote:

I've tried this approach, but could not come up with sufficiently
different error messages...

Wouldn't it be better to have a couple of regression
tests, as well?

Added two tests.

It should have three tests with one for ERANGE on top of the other
two. Passing down a value like "10e400" should be enough to cause
strtod() to fail, as far as I know.

+ if (sleep == 0)
+ continue;

While on it, forgot to comment on this one.. Indeed, this choice to
authorize 0 and not wait between two commands is more natural.

I have tweaked things as bit as of the attached, and ran pgindent.
What do you think?
--
Michael

On Mon, Mar 13, 2023 at 5:26 PM Michael Paquier <michael@paquier.xyz> wrote:

I have tweaked things as bit as of the attached, and ran pgindent.
What do you think?

Looks good to me.
Thanks!

Best regards, Andrey Borodin.

On Mon, Mar 13, 2023 at 06:14:18PM -0700, Andrey Borodin wrote:

Looks good to me.

Ok, thanks for looking. Let's wait a bit and see if others have an
opinion to offer. At least, the CI is green.
--
Michael

At Tue, 14 Mar 2023 11:36:17 +0900, Michael Paquier <michael@paquier.xyz> wrote in

Ok, thanks for looking. Let's wait a bit and see if others have an
opinion to offer. At least, the CI is green.

+				if (*opt_end)
+					pg_log_error("\\watch: incorrect interval value '%s'", opt);
+				else if (errno == ERANGE)
+					pg_log_error("\\watch: out-of-range interval value '%s'", opt);
+				else
+					pg_log_error("\\watch: interval value '%s' less than zero", opt);

I'm not sure if we need error messages for that resolution and I'm a
bit happier to have fewer messages to translate:p. Merging the cases
of ERANGE and negative values might be better. And I think we usually
refer to unparsable input as "invalid".

if (*opt_end)
pg_log_error("\\watch: invalid interval value '%s'", opt);
else
pg_log_error("\\watch: interval value '%s' out of range", opt);

It looks good other than that.

By the way, I noticed that \watch erases the query buffer. That
behavior differs from other commands, such as \g. And the difference
is not documented. Why do we erase the query buffer only in the case
of \watch?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Mar 14, 2023 at 01:58:59PM +0900, Kyotaro Horiguchi wrote:

+				if (*opt_end)
+					pg_log_error("\\watch: incorrect interval value '%s'", opt);
+				else if (errno == ERANGE)
+					pg_log_error("\\watch: out-of-range interval value '%s'", opt);
+				else
+					pg_log_error("\\watch: interval value '%s' less than zero", opt);

I'm not sure if we need error messages for that resolution and I'm a
bit happier to have fewer messages to translate:p. Merging the cases
of ERANGE and negative values might be better. And I think we usually
refer to unparsable input as "invalid".

if (*opt_end)
pg_log_error("\\watch: invalid interval value '%s'", opt);
else
pg_log_error("\\watch: interval value '%s' out of range", opt);

+1, I don't think it's necessary to complicate these error messages too
much. This code hasn't reported errors for nearly 10 years, and I'm not
aware of any complaints. I ѕtill think we could simplify this to "\watch:
invalid delay interval: %s" and call it a day.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Tue, 14 Mar 2023 12:03:00 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in

On Tue, Mar 14, 2023 at 01:58:59PM +0900, Kyotaro Horiguchi wrote:

+				if (*opt_end)
+					pg_log_error("\\watch: incorrect interval value '%s'", opt);
+				else if (errno == ERANGE)
+					pg_log_error("\\watch: out-of-range interval value '%s'", opt);
+				else
+					pg_log_error("\\watch: interval value '%s' less than zero", opt);

I'm not sure if we need error messages for that resolution and I'm a
bit happier to have fewer messages to translate:p. Merging the cases
of ERANGE and negative values might be better. And I think we usually
refer to unparsable input as "invalid".

if (*opt_end)
pg_log_error("\\watch: invalid interval value '%s'", opt);
else
pg_log_error("\\watch: interval value '%s' out of range", opt);

+1, I don't think it's necessary to complicate these error messages too
much. This code hasn't reported errors for nearly 10 years, and I'm not
aware of any complaints. I till think we could simplify this to "\watch:
invalid delay interval: %s" and call it a day.

I hesitated to propose such a level of simplification, but basically I
was alsothinking the same thing.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Mar 15, 2023 at 10:19:28AM +0900, Kyotaro Horiguchi wrote:

I hesitated to propose such a level of simplification, but basically I
was alsothinking the same thing.

Okay, fine by me to use one single message. I'd rather still keep the
three tests, though, as they check the three conditions upon which the
error would be triggered.
--
Michael