Re: cleaning up PostgresNode.pm

From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: cleaning up PostgresNode.pm
Date: 2021-07-18 15:48:10
Message-ID: 936b9ee3-f5c3-1789-f6d5-c9e803dda202@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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.
>>> +1
>>>
>>>> 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(),
>>> explain().
>>>
>>
>>
>> Here is a set of small(ish) patches that does most of the above and then
>> some.
>>
>>
>> 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.

cheers

andrew

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

Attachment Content-Type Size
0001-Add-w-back-to-the-flags-for-pg_ctl-re-start-in-Postg.patch text/x-patch 1.5 KB
0002-Add-adjust_conf-method-to-PostgresNode.patch text/x-patch 2.6 KB
0003-Unify-PostgresNode-s-new-and-get_new_node-methods.patch text/x-patch 85.9 KB
0004-Remove-the-last-vestiges-of-Exporter-from-PostgresNo.patch text/x-patch 4.2 KB
0005-Add-PostgresVersion.pm-method-to-emit-the-major-vers.patch text/x-patch 1.5 KB
0006-Add-a-getter-function-for-a-PostgresNode-install_pat.patch text/x-patch 983 bytes

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2021-07-18 17:23:31 Re: slab allocator performance issues
Previous Message Dilip Kumar 2021-07-18 15:45:03 Re: Toast compression method options