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.
