Re: cleaning up PostgresNode.pm

On 6/30/21 12:35 AM, Michael Paquier wrote:
> On Mon, Jun 28, 2021 at 01:02:37PM -0400, Andrew Dunstan wrote:
>> 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.
> 05cd12e applied to all the actions, so wouldn't it be more consistent
> to do the same for stop(), restart() and promote()?

Yes to restart(), no to stop() as it's always been the default and
wasn't changed by 05cd12e, no to promote() as it's been the default
since release 10 and wasn't a valid option before that according to the
manuals, hence changing it would actually be a backwards compatibility
barrier.

>> 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.
> +unless skip_equals is true, in which case it will write
> Nit: two spaces here.

Will fix.

> +Modify the named config file setting with the value. If the value is undefined,
> +instead delete the setting. If the setting is not present no action is taken.
> This should mention that parameters commented out are ignored?

Not really. A commented out setting isn't present.

> skip_equals is not used. The only caller of adjust_conf is
> PostgresNode itself.

Well, nothing is using it right now :-) It's intended to be available to
subclasses.

My current subclass code doesn't actually use skip_equals either, but
earlier revisions did. Think of modifying pg_hba.conf.

>> Patch 4 removes what's left of Exporter in PostgresNode, so it becomes a
>> pure OO style module.
> I have mixed feelings on this one, in a range of -0.1~0.1+, but please
> don't consider that as a strong objection either.

`perldoc perlmodlib` says: As a general rule, if the module is trying to
be object oriented then export nothing.

I mostly follow that rule.

An alternative proposal would keep using Exporter but move get_free_node
to @EXPORT_OK, again in line with standard perl advice to avoid use of
@EXPORT, which means clients would have to import it explicitly with
"use PostgresNode qw(get_free_port);" I don't think there's much gain
from that though.

>> 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.
> I doubt that anybody would complain about any of the changes you are
> doing here. It would be better to get that merged early in the
> development cycle on the contrary.

That's my intention. Thanks for reviewing.

cheers

andrew

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