Re: Making background psql nicer to use in tap tests

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Melanie Plageman <melanieplageman(at)gmail(dot)com>
Subject: Re: Making background psql nicer to use in tap tests
Date: 2023-03-17 09:48:09
Message-ID: 8D2271F2-7C36-4B79-A5CB-6BAAEDB07145@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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

Attachment Content-Type Size
v2-0001-Refactor-background-psql-TAP-functions.patch application/octet-stream 29.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2023-03-17 12:07:18 Re: Data is copied twice when specifying both child and parent table in publication
Previous Message Jim Jones 2023-03-17 09:46:36 Re: [PATCH] Add CANONICAL option to xmlserialize