Re: Making background psql nicer to use in tap tests

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.

Possibly. There are two concerns. First, make sure that they really are private. Last time I looked I think I noticed at least one thing that was alleged to be private but was called from a TAP script. Second, unless we backpatch it there will be some drift between branches, which can make backpatching things a bit harder. But by all means prep a patch so we can see the scope of the issue.

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.

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.

A common idiom is to have a composite getter/setter method for object properties something like this

sub settingname

{

  my ($self, $arg) = @_;

  $self->{settingname} = $arg if defined $arg;

  return $self->{settingname};

}

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

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.

Yeah, I can go along with that.

cheers

andrew

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