Re: Making background psql nicer to use in tap tests - Mailing list pgsql-hackers

> On 31 Jan 2023, at 01:00, Andres Freund <andres@anarazel.de> wrote:

> 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.

Thanks for working on this!

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

It might be rough around the edges but I don't think it's too far off a state
in which in can be committed, given that it's replacing something even rougher.
With documentation and some polish I think we can iterate on it in the tree.

I've played around a lot with it and it seems fairly robust.

> 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.

What if query() returns a list with the return value last?  The caller will get
the return value when assigning a single var as the return, and can get both in
those cases when it's interesting.  That would make for reasonably readable
code in most places?

   $ret_val = $h->query("SELECT 1;");
   ($query_result, $ret_val) = $h->query("SELECT 1;");

Returning a hash seems like a worse option since it will complicate callsites
which only want to know success/failure.

> - 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.

query_until isn't great but query_wait_until is IMO worse since the function
may well be used for tests which aren't using longrunning waits.  It's also
very useful for things which aren't queries at all, like psql backslash
commands.  I don't have any better ideas though, so +1 for sticking with
query_until.

> - right now there's a bit too much logic in background_psql() /
>  interactive_psql() for my taste

Not sure what you mean, I don't think they're especially heavy on logic?

> 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.

The test for \password in the SCRAM iteration count patch shrunk to 1/3 of the
previous coding.

> 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.

Agreed, and ideally implement tests which were left unwritten due to the old
API being clunky.

+       # feed the query to psql's stdin, follwed by \n (so psql processes the

s/follwed/followed/

+A default timeout of $PostgreSQL::Test::Utils::timeout_default is set up,
+which can modified later.

This require a bit of knowledge about the internals which I think we should
hide in this new API.  How about providing a function for defining the timeout?

Re timeouts: one thing I've done repeatedly is to use short timeouts and reset
them per query, and that turns pretty ugly fast.  I hacked up your patch to
provide $h->reset_timer_before_query() which then injects a {timeout}->start
before running each query without the caller having to do it.  Not sure if I'm
alone in doing that but if not I think it makes sense to add.

--
Daniel Gustafsson