Re: psql \watch 2nd argument: iteration count

Hello,

On Thu, 16 Mar 2023 21:15:30 -0700
Andrey Borodin <amborodin86(at)gmail(dot)com> wrote:

> On Wed, Mar 15, 2023 at 5:54 PM Michael Paquier <michael(at)paquier(dot)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(at)sraoss(dot)co(dot)jp>