From: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Melanie Plageman <melanieplageman(at)gmail(dot)com> |
Subject: | Re: Making background psql nicer to use in tap tests |
Date: | 2023-03-17 16:25:14 |
Message-ID: | 7fd9ea7e-51d6-8b4d-4d95-3fe650d894bd@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2023-03-17 Fr 10:08, Daniel Gustafsson wrote:
>
>> 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
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2023-03-17 16:41:11 | Re: Memory leak from ExecutorState context? |
Previous Message | Paul Jungwirth | 2023-03-17 16:03:09 | Re: Exclusion constraints on partitioned tables |