Re: Making background psql nicer to use in tap tests

> On 15 Mar 2023, at 02:03, Andres Freund <andres@anarazel.de> wrote:

>> Returning a hash seems like a worse option since it will complicate callsites
>> which only want to know success/failure.
> 
> Yea. Perhaps it's worth having a separate function for this? ->query_rc() or such?

If we are returning a hash then I agree it should be a separate function.
Maybe Andrew has input on which is the most Perl way of doing this.

>>> - 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?
> 
> -EMISSINGWORD on my part. A bit too much duplicated logic.

That makes more sense, and I can kind of agree.  I don't think it's too bad but
I agree there is room for improvement.

>> +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?
> 
> "definining" in the sense of accessing it? Or passing one in?

I meant passing one in.

>> 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.
> 
> I don't quite understand the use case, but I don't mind it as a functionality.

I've used it a lot when I want to run n command which each should finish
quickly or not at all.  So one time budget per command rather than having a
longer timeout for a set of commands that comprise a test.  It can be done
already today by calling ->start but it doesn't exactly make the code cleaner.

As mentioned off-list I did some small POD additions when reviewing, so I've
attached them here in a v2 in the hopes that it might be helpful.  I've also
included the above POC for restarting the timeout per query to show what I
meant.

--
Daniel Gustafsson