> On 15 Mar 2023, at 02:03, Andres Freund <and...@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
v2-0001-Refactor-background-psql-TAP-functions.patch
Description: Binary data
