|From:||Andrew Dunstan <andrew(at)dunslane(dot)net>|
|To:||Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>|
|Subject:||Re: cleaning up PostgresNode.pm|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 7/18/21 11:48 AM, Andrew Dunstan wrote:
> On 7/16/21 3:32 PM, Andrew Dunstan wrote:
>> On 6/28/21 1:02 PM, Andrew Dunstan wrote:
>>> On 4/24/21 3:14 PM, Alvaro Herrera wrote:
>>>> On 2021-Apr-24, Andrew Dunstan wrote:
>>>>> I would like to undertake some housekeeping on PostgresNode.pm.
>>>>> 1. OO modules in perl typically don't export anything. We should remove
>>>>> the export settings. That would mean that clients would have to call
>>>>> "PostgresNode->get_new_node()" (but see item 2) and
>>>>> "PostgresNode::get_free_port()" instead of the unadorned calls they use now.
>>>>> 2. There are two constructors, new() and get_new_node(). AFAICT nothing
>>>>> in our tests uses new(), and they almost certainly shouldn't anyway.
>>>>> get_new_node() calls new() to do some work, and I'd like to merge these
>>>>> two. The name of a constructor in perl is conventionally "new" as it is
>>>>> in many other OO languages, although in perl this can't apply where a
>>>>> class provides more than one constructor. Still, if we're merging them
>>>>> then the preference would be to call the merged function "new". Since
>>>>> we'd proposing to modify the calls anyway (see item 1) this shouldn't
>>>>> impose a huge extra workload.
>>>> +1 on "new". I think we weren't 100% clear on where we wanted it to go
>>>> initially, but it's now clear that get_new_node() is the constructor,
>>>> and that new() is merely a helper. So let's rename them in a sane way.
>>>>> Another item that needs looking at is the consistent use of Carp.
>>>>> PostgresNode, TestLib and RecursiveCopy all use the Carp module, but
>>>>> contain numerous calls to "die" where they should probably have calls to
>>>>> "croak" or "confess".
>>>> I wonder if it would make sense to think of PostgresNode as a feeder of
>>>> sorts to Test::More and the like, so make it use diag(), note(),
>>> Here is a set of small(ish) patches that does most of the above and then
>>> Patch 1 adds back the '-w' flag to pg_ctl in the start() method. It's
>>> redundant on modern versions of Postgres but it's harmless, and helps
>>> with subclassing for older versions where it wasn't the default.
>>> Patch 2 adds a method for altering config files as opposed to just
>>> appending to them. Again, this helps a lot in subclassing for older
>>> versions, which can call the parent's init() and then adjust whatever
>>> doesn't work.
>>> Patch 3 unifies the constructor methods and stops exporting a
>>> constructor. There is one constructor: PostgresNode::new()
>>> Patch 4 removes what's left of Exporter in PostgresNode, so it becomes a
>>> pure OO style module.
>>> Patch 5 adds a method for getting the major version string from a
>>> PostgresVersion object, again useful in subclassing.
>>> Patch 6 adds a method for getting the install_path of a PostgresNode
>>> object. While not strictly necessary it's consistent with other fields
>>> that have getter methods. Clients should not pry into the internals of
>>> objects. Experience has shown this method to be useful.
>>> Patches 7 8 and 9 contain additions to Patch 3 for things that I
>>> overlooked or that were not present when I originally prepared the
>>> patches. They would be applied alongside Patch 3, not separately.
>>> These patches are easily broken by e.g. the addition of a new TAP test
>>> or the modification of an existing test. So I'm hoping to get these
>>> added soon. I will add this email to the CF.
>> New version with a small change to fix bitrot.
> New set with fixups incorporated and review comments attended to. I'm
> intending to apply this later this week.
This time without a missing comma.
|Next Message||Alexander Korotkov||2021-07-18 18:20:32||Re: unnesting multirange data types|
|Previous Message||Tomas Vondra||2021-07-18 17:23:31||Re: slab allocator performance issues|