Prevent psql \watch from running queries that return no rows

Attached is a patch to allow a new behavior for the \watch command in psql.
When enabled, this instructs \watch to stop running once the query returns
zero rows. The use case is the scenario in which you are watching the
output of something long-running such as pg_stat_progress_create_index, but
once it finishes you don't need thousands of runs showing empty rows from
the view.

This adds a new argument "zero" to the existing i=SEC and c=N arguments

Notes:

* Not completely convinced of the name "zero" (better than
"stop_when_no_rows_returned"). Considered adding a new x=y argument, or
overloading c (c=-1) but neither seemed very intuitive. On the other hand,
it's tempting to stick to a single method moving forward, although this is
a boolean option not a x=y one like the other two.

* Did not update help.c on purpose - no need to make \watch span two lines
there.

* Considered leaving early (e.g. don't display the last empty result) but
seemed better to show the final empty result as an explicit confirmation as
to why it stopped.

* Quick way to test:
select * from pg_stat_activity where backend_start > now() - '20
seconds'::interval;
\watch zero

Cheers,
Greg

On Fri, Jun 02, 2023 at 11:47:16AM -0400, Greg Sabino Mullane wrote:
> * Not completely convinced of the name "zero" (better than
> "stop_when_no_rows_returned"). Considered adding a new x=y argument, or
> overloading c (c=-1) but neither seemed very intuitive. On the other hand,
> it's tempting to stick to a single method moving forward, although this is
> a boolean option not a x=y one like the other two.

Wouldn't something like a target_rows be more flexible? You could use
this parameter with a target number of rows to expect, zero being one
choice in that.
--
Michael

On Sat, Jun 3, 2023 at 5:58 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

>
> Wouldn't something like a target_rows be more flexible? You could use
> this parameter with a target number of rows to expect, zero being one
> choice in that.
>

Thank you! That does feel better to me. Please see attached a new v2 patch
that uses
a min_rows=X syntax (defaults to 0). Also added some help.c changes.

Cheers,
Greg

> On 4 Jun 2023, at 20:55, Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
>
> On Sat, Jun 3, 2023 at 5:58 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> Wouldn't something like a target_rows be more flexible? You could use
> this parameter with a target number of rows to expect, zero being one
> choice in that.
>
> Thank you! That does feel better to me. Please see attached a new v2 patch that uses
> a min_rows=X syntax (defaults to 0). Also added some help.c changes.

This is a feature I've wanted on several occasions so definitely a +1 on this
suggestion. I've yet to test it out and do a full review, but a few comments
from skimming the patch:

- bool is_watch,
+ bool is_watch, int min_rows,

The comment on ExecQueryAndProcessResults() needs to be updated with an
explanation of what this parameter is.

- return cancel_pressed ? 0 : success ? 1 : -1;
+ return (cancel_pressed || return_early) ? 0 : success ? 1 : -1;

I think this is getting tangled up enough that it should be replaced with
separate if() statements for the various cases.

- HELP0(" \\watch [[i=]SEC] [c=N] execute query every SEC seconds, up to N times\n");
+ HELP0(" \\watch [[i=]SEC] [c=N] [m=ROW]\n");
+ HELP0(" execute query every SEC seconds, up to N times\n");
+ HELP0(" stop if less than ROW minimum rows are rerturned\n");

"less than ROW minimum rows" reads a bit awkward IMO, how about calling it
[m=MIN] and describe as "stop if less than MIN rows are returned"? Also, there
is a typo: s/rerturned/returned/.

--
Daniel Gustafsson

Thanks for the feedback!

On Wed, Jul 5, 2023 at 5:51 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

>
> The comment on ExecQueryAndProcessResults() needs to be updated with an
> explanation of what this parameter is.
>

I added a comment in the place where min_rows is used, but not sure what
you mean by adding it to the main comment at the top of the function? None
of the other args are explained there, even the non-intuitive ones (e.g.
svpt_gone_p)

- return cancel_pressed ? 0 : success ? 1 : -1;
> + return (cancel_pressed || return_early) ? 0 : success ? 1 : -1;
>
> I think this is getting tangled up enough that it should be replaced with
> separate if() statements for the various cases.
>

Would like to hear others weigh in, I think it's still only three states
plus a default, so I'm not convinced it warrants multiple statements yet. :)

+ HELP0(" \\watch [[i=]SEC] [c=N] [m=ROW]\n");
> + HELP0(" execute query every SEC seconds,
> up to N times\n");
> + HELP0(" stop if less than ROW minimum
> rows are rerturned\n");
>
> "less than ROW minimum rows" reads a bit awkward IMO, how about calling it
> [m=MIN] and describe as "stop if less than MIN rows are returned"? Also,
> there
> is a typo: s/rerturned/returned/.
>

Great idea: changed and will attach a new patch

Cheers,
Greg

On Wed, Jul 05, 2023 at 10:14:22AM -0400, Greg Sabino Mullane wrote:
> Would like to hear others weigh in, I think it's still only three states
> plus a default, so I'm not convinced it warrants multiple statements yet. :)

I find that hard to parse, so having more lines to get a better idea
of what the states are would be good.

While on it, I can see that the patch has no tests. Could you add
something in psql's 001_basic.pl? The option to define the number of
iterations for \watch can make the tests predictible even for the
non-failure cases. I would do checks with incorrect values, as well,
see the part of the test script about the intervals.
--
Michael

Thank you for the feedback, everyone. Attached is version 4 of the patch,
featuring a few tests and minor rewordings.

Cheers,
Greg

> On 22 Aug 2023, at 23:23, Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
>
> Thank you for the feedback, everyone. Attached is version 4 of the patch, featuring a few tests and minor rewordings.

Thanks, the changes seem good from a quick skim. I'll take a better look
tomorrow to hopefully close this one.

--
Daniel Gustafsson

> On 22 Aug 2023, at 23:23, Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
>
> Thank you for the feedback, everyone. Attached is version 4 of the patch, featuring a few tests and minor rewordings.

I had another look, and did some playing around with this and I think this
version is ready to go in, so I will try to get that sorted shortly.

--
Daniel Gustafsson

> On 22 Aug 2023, at 23:23, Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
>
> Thank you for the feedback, everyone. Attached is version 4 of the patch, featuring a few tests and minor rewordings.

I went over this once more, and pushed it along with pgindenting. I did reduce
the number of tests since they were overlapping and a bit too expensive to have
multiples of. Thanks for the patch!

--
Daniel Gustafsson