Horton

Re: Making background psql nicer to use in tap tests

Andrew Dunstan <andrew@dunslane.net>

View thread View on postgr.es
From: Andrew Dunstan <andrew@dunslane.net>
To: Daniel Gustafsson <daniel@yesql.se>, Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Melanie Plageman <melanieplageman@gmail.com>
Date: 2023-03-17T13:48:31Z
Lists: pgsql-hackers 
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);


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

Commits

  1. Fix missing installation/uninstallation rules for BackgroundPsql.pm

  2. Backport BackgroundPsql perl test module

  3. Add missing uninstallation rule for BackgroundPsql.pm

  4. Fix missing installation rules for BackgroundPsql.pm

  5. Skip \password TAP test on old IPC::Run versions

  6. Test SCRAM iteration changes with psql \password

  7. Refactor background psql TAP functions