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