Re: Making background psql nicer to use in tap tests

On 2023-03-17 Fr 05:48, Daniel Gustafsson wrote:
>> 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.

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

A few more issues:

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

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

If we are going to keep this as a separate package, then we should put
some code in the constructor to prevent it being called 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';

This should refer to the full class name:

+=item $node->background_psql($dbname, %params) => BackgroundPsql instance

Still reviewing ...

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com