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-16 19:32:26 |
Message-ID: | f25b680c-b60c-9f54-3854-bd570c9a611e@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
0001-Add-w-back-to-the-flags-for-pg_ctl-start-in-Postgres.patch | text/x-patch | 1.1 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 | 80.9 KB |
0004-Remove-the-last-vestiges-of-Exporter-from-PostgresNo.patch | text/x-patch | 4.2 KB |
0005-Add-a-method-to-PostgresVersion.pm-to-produce-the-ma.patch | text/x-patch | 1.5 KB |
0006-Add-a-getter-function-for-a-PostgresNode-install_pat.patch | text/x-patch | 982 bytes |
0007-fixup-recovery-tests.patch | text/x-patch | 2.2 KB |
0008-fixup-README.patch | text/x-patch | 747 bytes |
0009-new-recovery-fixups.patch | text/x-patch | 3.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Justin Pryzby | 2021-07-16 19:33:19 | Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers |
Previous Message | James Coleman | 2021-07-16 19:16:10 | Re: Consider parallel for lateral subqueries with limit |