Re: psql \watch 2nd argument: iteration count
Peter Eisentraut writes: > On 13.03.23 02:17, 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. > On the other hand, we also have option syntax in \connect that is like > -foo. Would that be a better match here? We should maybe decide before > we diverge and propagate two different option syntaxes in backslash > commands. Reasonable point to raise, but I think \connect's -reuse-previous is in the minority. \connect itself can use option=value syntax in the conninfo string (in fact, I guess -reuse-previous was spelled that way in hopes of not being confusable with a conninfo option). We also have option=value in the \g and \gx commands. I don't see any other psql metacommands that use options spelled like -foo. In short, I'm satisfied with the current answer. There's still time to debate it though. regards, tom lane
Re: psql \watch 2nd argument: iteration count
On 13.03.23 02:17, Michael Paquier 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.
On the other hand, we also have option syntax in \connect that is like
-foo. Would that be a better match here? We should maybe decide before
we diverge and propagate two different option syntaxes in backslash
commands.
Re: psql \watch 2nd argument: iteration count
On Thu, Apr 6, 2023 at 10:23 PM Tom Lane wrote: > > Kirk Wolak writes: > > Marked as Ready for Committer. > > Pushed with a pretty fair number of cosmetic changes. Great, thank you! > If you write a semicolon first, you get four, but it's the semicolon > producing the first result not \watch. I did not know that. Well, I knew it in parts, but did not understand as a whole. Thanks! Best regards, Andrey Borodin.
Re: psql \watch 2nd argument: iteration count
Kirk Wolak writes: > Marked as Ready for Committer. Pushed with a pretty fair number of cosmetic changes. One non-cosmetic change I made is that I didn't agree with your interpretation of the execution count. IMO this ought to produce three executions: regression=# select 1 \watch c=3 Thu Apr 6 13:17:50 2023 (every 2s) ?column? -- 1 (1 row) Thu Apr 6 13:17:52 2023 (every 2s) ?column? -- 1 (1 row) Thu Apr 6 13:17:54 2023 (every 2s) ?column? -- 1 (1 row) regression=# If you write a semicolon first, you get four, but it's the semicolon producing the first result not \watch. regards, tom lane
Re: psql \watch 2nd argument: iteration count
On Fri, Mar 24, 2023 at 10:32 PM Andrey Borodin wrote: > On Thu, Mar 23, 2023 at 10:15 PM Yugo NAGATA wrote: > > > > Here is my review on the v9 patch. > > > > + /* we do not prevent numerous names iterations like > i=1 i=1 i=1 */ > > + have_sleep = true; > > > > Why this is allowed here? I am not sure there is any reason to allow to > specify > > multiple "interval" options. (I would apologize it if I missed past > discussion.) > I do not know, it just seems normal to me. I've fixed this. > > > postgres=# select 1 \watch interval=3 4 > > \watch: incorrect interval value '4' > > > > I think it is ok, but this error message seems not user-friendly because, > > in the above example, interval values itself is correct, but it seems > just > > a syntax error. I wonder it is better to use "watch interval must be > specified > > only once" or such here, as the past patch. > Done. > > > > > + > > +If number of iterations is specified - query will be executed > only > > +given number of times. > > + > > > > Is it common to use "-" here? I think using comma like > > "If number of iterations is specified, " > > is natural. > Done. > > Thank for the review! > > > Best regards, Andrey Borodin. > Okay, I tested this. It handles bad param names, values correctly. Nice Feature, especially if you leave a 1hr task running and you need to step away... Built/Reviewed the Docs. They are correct. Reviewed \? command. It has the parameters updated, shown as optional Marked as Ready for Committer.
Re: psql \watch 2nd argument: iteration count
On Thu, Mar 23, 2023 at 10:15 PM Yugo NAGATA wrote: > > Here is my review on the v9 patch. > > + /* we do not prevent numerous names iterations like i=1 > i=1 i=1 */ > + have_sleep = true; > > Why this is allowed here? I am not sure there is any reason to allow to > specify > multiple "interval" options. (I would apologize it if I missed past > discussion.) I do not know, it just seems normal to me. I've fixed this. > postgres=# select 1 \watch interval=3 4 > \watch: incorrect interval value '4' > > I think it is ok, but this error message seems not user-friendly because, > in the above example, interval values itself is correct, but it seems just > a syntax error. I wonder it is better to use "watch interval must be specified > only once" or such here, as the past patch. Done. > > + > +If number of iterations is specified - query will be executed only > +given number of times. > + > > Is it common to use "-" here? I think using comma like > "If number of iterations is specified, " > is natural. Done. Thank for the review! Best regards, Andrey Borodin. v10-0001-Iteration-count-argument-to-psql-watch-command.patch Description: Binary data
Re: psql \watch 2nd argument: iteration count
Hello,
On Thu, 16 Mar 2023 21:15:30 -0700
Andrey Borodin wrote:
> On Wed, Mar 15, 2023 at 5:54 PM Michael Paquier wrote:
> >
> > On Wed, Mar 15, 2023 at 04:58:49PM +0900, Michael Paquier wrote:
> > > Yep. You are right.
> >
> > Fixed that and applied 0001.
> Great, thanks!
>
> >
> > +valptr++;
> > +if (strncmp("i", opt, strlen("i")) == 0 ||
> > +strncmp("interval", opt, strlen("interval")) == 0)
> > +{
> >
> > Did you look at process_command_g_options() and if some consolidation
> > was possible? It would be nice to have APIs shaped so as more
> > sub-commands could rely on the same facility in the future.
> I've tried, but they behave so differently. I could reuse only the
> "char *valptr = strchr(opt, '=');" thing from there :)
> And process_command_g_options() changes data in-place...
> Actually, I'm not sure having "i" == "interval" and "c"=="count" is a
> good idea here too. I mean I like it, but is it coherent?
> Also I do not like repeating 4 times this 5 lines
> + pg_log_error("\\watch: incorrect interval value '%s'", valptr);
> + free(opt);
> + resetPQExpBuffer(query_buf);
> + psql_scan_reset(scan_state);
> + return PSQL_CMD_ERROR;
> But I hesitate defining a new function for this...
>
> >
> > -\watch [ > class="parameter">seconds ]
> > +\watch [ > class="parameter">seconds [ > class="parameter">iterations ] ]
> >
> > This set of changes is not reflected in the documentation.
> Done.
>
> > With an interval in place, we could now automate some tests with
> > \watch where it does not fail. What do you think about adding a test
> > with a simple query, an interval of 0s and one iteration?
> Done. Also found a bug that we actually were doing N+1 iterations.
Here is my review on the v9 patch.
+ /* we do not prevent numerous names iterations like i=1 i=1
i=1 */
+ have_sleep = true;
Why this is allowed here? I am not sure there is any reason to allow to specify
multiple "interval" options. (I would apologize it if I missed past discussion.)
+ if (sleep < 0 || *opt_end || errno == ERANGE || have_sleep)
+ {
+ pg_log_error("\\watch: incorrect interval value '%s'",
Here, specifying an explicit "interval" option before an interval second without
option is prohibited.
postgres=# select 1 \watch interval=3 4
\watch: incorrect interval value '4'
I think it is ok, but this error message seems not user-friendly because,
in the above example, interval values itself is correct, but it seems just
a syntax error. I wonder it is better to use "watch interval must be specified
only once" or such here, as the past patch.
+
+If number of iterations is specified - query will be executed only
+given number of times.
+
Is it common to use "-" here? I think using comma like
"If number of iterations is specified, "
is natural.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: psql \watch 2nd argument: iteration count
On Wed, Mar 15, 2023 at 5:54 PM Michael Paquier wrote:
>
> On Wed, Mar 15, 2023 at 04:58:49PM +0900, Michael Paquier wrote:
> > Yep. You are right.
>
> Fixed that and applied 0001.
Great, thanks!
>
> +valptr++;
> +if (strncmp("i", opt, strlen("i")) == 0 ||
> +strncmp("interval", opt, strlen("interval")) == 0)
> +{
>
> Did you look at process_command_g_options() and if some consolidation
> was possible? It would be nice to have APIs shaped so as more
> sub-commands could rely on the same facility in the future.
I've tried, but they behave so differently. I could reuse only the
"char *valptr = strchr(opt, '=');" thing from there :)
And process_command_g_options() changes data in-place...
Actually, I'm not sure having "i" == "interval" and "c"=="count" is a
good idea here too. I mean I like it, but is it coherent?
Also I do not like repeating 4 times this 5 lines
+ pg_log_error("\\watch: incorrect interval value '%s'", valptr);
+ free(opt);
+ resetPQExpBuffer(query_buf);
+ psql_scan_reset(scan_state);
+ return PSQL_CMD_ERROR;
But I hesitate defining a new function for this...
>
> -\watch [ class="parameter">seconds ]
> +\watch [ class="parameter">seconds [ class="parameter">iterations ] ]
>
> This set of changes is not reflected in the documentation.
Done.
> With an interval in place, we could now automate some tests with
> \watch where it does not fail. What do you think about adding a test
> with a simple query, an interval of 0s and one iteration?
Done. Also found a bug that we actually were doing N+1 iterations.
Thank you for working on this!
Best regards, Andrey Borodin.
v9-0001-Iteration-count-argument-to-psql-watch-command.patch
Description: Binary data
Re: psql \watch 2nd argument: iteration count
On Wed, Mar 15, 2023 at 04:58:49PM +0900, Michael Paquier wrote:
> Yep. You are right.
Fixed that and applied 0001.
+valptr++;
+if (strncmp("i", opt, strlen("i")) == 0 ||
+strncmp("interval", opt, strlen("interval")) == 0)
+{
Did you look at process_command_g_options() and if some consolidation
was possible? It would be nice to have APIs shaped so as more
sub-commands could rely on the same facility in the future.
-\watch [ seconds ]
+\watch [ seconds [ iterations ] ]
This set of changes is not reflected in the documentation.
With an interval in place, we could now automate some tests with
\watch where it does not fail. What do you think about adding a test
with a simple query, an interval of 0s and one iteration?
--
Michael
signature.asc
Description: PGP signature
Re: psql \watch 2nd argument: iteration count
On Tue, Mar 14, 2023 at 09:23:48PM -0700, Nathan Bossart wrote: > + sleep = strtod(opt, _end); > + if (sleep < 0 || *opt_end || errno == ERANGE) > > Should we set errno to 0 before calling strtod()? Yep. You are right. -- Michael signature.asc Description: PGP signature
Re: psql \watch 2nd argument: iteration count
+ sleep = strtod(opt, _end); + if (sleep < 0 || *opt_end || errno == ERANGE) Should we set errno to 0 before calling strtod()? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: psql \watch 2nd argument: iteration count
On Tue, Mar 14, 2023 at 08:20:23PM -0700, Andrey Borodin wrote: > PFA v8. Thanks! Looks OK to me. I've looked as well at resetting query_buffer on failure, which I guess is better this way because this is an accumulation of the previous results, right? -- Michael signature.asc Description: PGP signature
Re: psql \watch 2nd argument: iteration count
On Tue, Mar 14, 2023 at 6:25 PM Michael Paquier wrote: > > 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. +1 > 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. PFA v8. Thanks! Best regards, Andrey Borodin. v8-0001-Fix-incorrect-argument-handling-in-psql-watch.patch Description: Binary data v8-0002-Add-iteration-count-argument-to-psql-watch-comman.patch Description: Binary data
Re: psql \watch 2nd argument: iteration count
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 signature.asc Description: PGP signature
Re: psql \watch 2nd argument: iteration count
At Tue, 14 Mar 2023 12:03:00 -0700, Nathan Bossart
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
Re: psql \watch 2nd argument: iteration count
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
Re: psql \watch 2nd argument: iteration count
At Tue, 14 Mar 2023 11:36:17 +0900, Michael Paquier 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
Re: psql \watch 2nd argument: iteration count
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 signature.asc Description: PGP signature
Re: psql \watch 2nd argument: iteration count
On Mon, Mar 13, 2023 at 5:26 PM Michael Paquier 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.
Re: psql \watch 2nd argument: iteration count
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
From f6dad7551c04c3e7b280c310a744f8d4dfc10d19 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 14 Mar 2023 09:21:32 +0900
Subject: [PATCH v7] Fix incorrect argument handling in psql \watch
Incorrectly parsed argument was silently substituted with 1 second.
This is changed to proper error message.
Authour: Andrey Borodin
Reviewed-by: Kyotaro Horiguchi
Reviewed-by: Nathan Bossart
Reviewed-by: Michael Paquier
Thread: https://postgr.es/m/CAAhFRxiZ2-n_L1ErMm9AZjgmUK%3DqS6VHb%2B0SaMn8sqqbhF7How%40mail.gmail.com
---
src/bin/psql/command.c | 21 ++---
src/bin/psql/t/001_basic.pl | 17 +
2 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 955397ee9d..60cabfd2c8 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2776,9 +2776,21 @@ exec_command_watch(PsqlScanState scan_state, bool active_branch,
/* Convert optional sleep-length argument */
if (opt)
{
- sleep = strtod(opt, NULL);
- if (sleep <= 0)
-sleep = 1;
+ char *opt_end;
+ sleep = strtod(opt, _end);
+ if (sleep < 0 || *opt_end || errno == ERANGE)
+ {
+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);
+free(opt);
+resetPQExpBuffer(query_buf);
+psql_scan_reset(scan_state);
+return PSQL_CMD_ERROR;
+ }
free(opt);
}
@@ -5183,6 +5195,9 @@ do_watch(PQExpBuffer query_buf, double sleep)
if (pagerpipe && ferror(pagerpipe))
break;
+ if (sleep == 0)
+ continue;
+
#ifdef WIN32
/*
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index 0f394420b2..b86ff99a63 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -350,4 +350,21 @@ psql_like(
'\copy from with DEFAULT'
);
+# Check \watch errors
+psql_fails_like(
+ $node,
+ 'SELECT 1;\watch -10',
+ qr/interval value '-10' less than zero/,
+ '\watch, negative interval');
+psql_fails_like(
+ $node,
+ 'SELECT 1;\watch 10ab',
+ qr/incorrect interval value '10ab'/,
+ '\watch incorrect interval');
+psql_fails_like(
+ $node,
+ 'SELECT 1;\watch 10e400',
+ qr/out-of-range interval value '10e400'/,
+ '\watch out-of-range interval');
+
done_testing();
--
2.39.2
signature.asc
Description: PGP signature
Re: psql \watch 2nd argument: iteration count
Michael, thanks for reviewing this!
On Sun, Mar 12, 2023 at 6:17 PM Michael Paquier 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 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, _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.
v6-0002-Add-iteration-count-argument-to-psql-watch-comman.patch
Description: Binary data
v6-0001-Fix-incorrect-argument-handling-in-psql-watch.patch
Description: Binary data
Re: psql \watch 2nd argument: iteration count
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, _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
signature.asc
Description: PGP signature
Re: psql \watch 2nd argument: iteration count
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
signature.asc
Description: PGP signature
Re: psql \watch 2nd argument: iteration count
On Thu, Mar 9, 2023 at 11:25 AM Nathan Bossart 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.
v5-0002-Add-iteration-count-argument-to-psql-watch-comman.patch
Description: Binary data
v5-0001-Fix-incorrect-argument-handling-in-psql-watch.patch
Description: Binary data
Re: psql \watch 2nd argument: iteration count
+ 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
Re: psql \watch 2nd argument: iteration count
On Wed, Mar 8, 2023 at 10:49 AM Nathan Bossart 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. v4-0002-Add-iteration-count-argument-to-psql-watch-comman.patch Description: Binary data v4-0001-Fix-incorrect-argument-handling-in-psql-watch.patch Description: Binary data
Re: psql \watch 2nd argument: iteration count
+1 for adding an iteration count argument to \watch.
+ char *opt_end;
+ sleep = strtod(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
Re: psql \watch 2nd argument: iteration count
Thanks for looking into this! On Mon, Feb 20, 2023 at 6:15 PM Kyotaro Horiguchi 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. v3-0002-Add-iteration-count-argument-to-psql-watch-comman.patch Description: Binary data v3-0001-Fix-incorrect-argument-handling-in-psql-watch.patch Description: Binary data
Re: psql \watch 2nd argument: iteration count
At Mon, 20 Feb 2023 10:45:53 -0800, Andrey Borodin 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
Re: psql \watch 2nd argument: iteration count
> > > 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. v2-0001-Fix-incorrect-argument-handling-in-psql-watch.patch Description: Binary data v2-0001-Add-iteration-count-argument-to-psql-watch-comman.patch Description: Binary data
Re: psql \watch 2nd argument: iteration count
Greetings, * Peter Eisentraut (peter.eisentr...@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 signature.asc Description: PGP signature
Re: 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.
