Thread: psql \watch 2nd argument: iteration count

Hi hackers!

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.
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.
What do you think?


Best regards, Andrey Borodin.

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

On Tue, Mar 14, 2023 at 6:25 PM Michael Paquier <michael@paquier.xyz> 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.

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

+            sleep = strtod(opt, &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

On Tue, Mar 14, 2023 at 09:23:48PM -0700, Nathan Bossart wrote:
> +            sleep = strtod(opt, &opt_end);
> +            if (sleep < 0 || *opt_end || errno == ERANGE)
>
> Should we set errno to 0 before calling strtod()?

Yep.  You are right.
--
Michael

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.

-        <term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> ]</literal></term>
+        <term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> [ <replaceable
class="parameter">iterations</replaceable>] ]</literal></term>
 

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

On Wed, Mar 15, 2023 at 5:54 PM Michael Paquier <michael@paquier.xyz> 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...

>
> -        <term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> ]</literal></term>
> +        <term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> [ <replaceable
class="parameter">iterations</replaceable>] ]</literal></term> 
>
> 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.

Hello,

On Thu, 16 Mar 2023 21:15:30 -0700
Andrey Borodin <amborodin86@gmail.com> wrote:

> On Wed, Mar 15, 2023 at 5:54 PM Michael Paquier <michael@paquier.xyz> 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...
> 
> >
> > -        <term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> ]</literal></term>
> > +        <term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> [ <replaceable
class="parameter">iterations</replaceable>] ]</literal></term>
 
> >
> > 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.

+        <para>
+        If number of iterations is specified - query will be executed only
+        given number of times.
+        </para>

Is it common to use "-" here?  I think using comma like 
"If number of iterations is specified, "
is natural.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Thu, Mar 23, 2023 at 10:15 PM Yugo NAGATA <nagata@sraoss.co.jp> 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.

>
> +        <para>
> +        If number of iterations is specified - query will be executed only
> +        given number of times.
> +        </para>
>
> 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.

Kirk Wolak <wolakk@gmail.com> 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

On Thu, Apr 6, 2023 at 10:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Kirk Wolak <wolakk@gmail.com> 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.

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.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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