Re: psql \watch 2nd argument: iteration count - Mailing list pgsql-hackers

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>