Re: psql \watch 2nd argument: iteration count

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.