Re: Making background psql nicer to use in tap tests

Hi,

On 2023-03-14 21:24:32 +0100, Daniel Gustafsson wrote:
> > On 31 Jan 2023, at 01:00, Andres Freund <andres(at)anarazel(dot)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!

Thanks for helping it move along :)

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

Cool.

> > 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;");

I hate perl.

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

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

> +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?

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

Greetings,

Andres Freund