Re: Making background psql nicer to use in tap tests

> On 15 Mar 2023, at 02:03, Andres Freund <andres(at)anarazel(dot)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