Re: Making background psql nicer to use in tap tests

Hi,

On 2023-01-30 15:06:46 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > It's annoyingly hard to wait for the result of a query in a generic way with
> > background_psql(), and more generally for psql. background_psql() uses -XAtq,
> > which means that we'll not get "status" output (like "BEGIN" or "(1 row)"),
> > and that queries not returning anything are completely invisible.
>
> Yeah, the empty-query-result problem was giving me fits recently.
> +1 for wrapping this into something more convenient to use.

I've hacked some on this. I first tried to just introduce a few helper
functions in Cluster.pm, but that ended up being awkward. So I bit the bullet
and introduced a new class (in BackgroundPsql.pm), and made background_psql()
and interactive_psql() return an instance of it.

This is just a rough prototype. Several function names don't seem great, it
need POD documentation, etc.


The main convenience things it has over the old interface:
- $node->background_psql('dbname') is enough
- $psql->query(), which returns the query results as a string, is a lot easier
  to use than having to pump, identify query boundaries via regex etc.
- $psql->query_safe(), which dies if any query fails (detected via stderr)
- $psql->query_until() is a helper that makes it a bit easier to start queries
  that won't finish until a later point


I don't quite like the new interface yet:
- It's somewhat common to want to know if there was a failure, but also get
  the query result, not sure what the best function signature for that is in
  perl.
- query_until() sounds a bit too much like $node->poll_query_until(). Maybe
  query_wait_until() is better? OTOH, the other function has poll in the name,
  so maybe it's ok.
- right now there's a bit too much logic in background_psql() /
  interactive_psql() for my taste


Those points aside, I think it already makes the tests a good bit more
readable. My WIP vacuum_defer_cleanup_age patch shrunk by half with it.

I think with a bit more polish it's easy enough to use that we could avoid a
good number of those one-off psql's that we do all over.


I didn't really know what this, insrc/test/subscription/t/015_stream.pl, is
about:

$h->finish;    # errors make the next test fail, so ignore them here

There's no further test?

I'm somewhat surprised it doesn't cause problems in another ->finish later on,
where we then afterwards just use $h again. Apparently IPC::Run just
automagically restarts psql?


Greetings,

Andres Freund