Re: Making background psql nicer to use in tap tests

> On 17 Mar 2023, at 14:48, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2023-03-17 Fr 05:48, Daniel Gustafsson wrote:
>>> On 15 Mar 2023, at 02:03, Andres Freund <andres@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.
>
> I think the perlish way is use the `wantarray` function. Perl knows if you're expecting a scalar return value or a
list(which includes a hash). 
>
>    return wantarray ? $retval : (list or hash);

Aha, TIL. That seems like precisely what we want.

> A common perl idiom is to start private routine names with an underscore. so I'd rename wait_connect to
_wait_connect;

There are quite a few routines documented as internal in Cluster.pm which don't
start with an underscore.  Should we change them as well?  I'm happy to prepare
a separate patch to address that if we want that.

> Why is $restart_before_query a package/class level value instead of an instance value? And why can we only ever set
itto 1 but not back again? Maybe we don't want to, but it looks odd. 

It was mostly a POC to show what I meant with the functionality.  I think there
should be a way to turn it off (set it to zero) even though I doubt it will be
used much.

> If we are going to keep this as a separate package, then we should put some code in the constructor to prevent it
beingcalled from elsewhere than the Cluster package. e.g. 
>
>     # this constructor should only be called from PostgreSQL::Test::Cluster
>     my ($package, $file, $line) = caller;
>
>     die "Forbidden caller of constructor: package: $package, file: $file:$line"
>       unless $package eq 'PostgreSQL::Test::Cluster';

I don't have strong feelings about where to place this, but Cluster.pm is
already quite long so I see a small upside to keeping it separate to not make
that worse.

--
Daniel Gustafsson