Date: Wed, 30 Jun 2021 13:35:24 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: cleaning up PostgresNode.pm
Message-ID: <YNv0jNFBtsbOT5Wp@paquier.xyz>
References: <20210424191444.GA25008@alvherre.pgsql>
 <021d7feb-f91c-ab2e-5b80-ccdd37e78a18@dunslane.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2OXbw9+z5vRm9yFR"
Content-Disposition: inline
In-Reply-To: <021d7feb-f91c-ab2e-5b80-ccdd37e78a18@dunslane.net>
Archived-At: 
 <https://www.postgresql.org/message-id/YNv0jNFBtsbOT5Wp%40paquier.xyz>
Precedence: bulk


--2OXbw9+z5vRm9yFR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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()?

> 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.

+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?

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

> Patch 3 unifies the constructor methods and stops exporting a
> constructor. There is one constructor: PostgresNode::new()

Nice^2.  I agree that this is an improvement.

> 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.

> Patch 5 adds a method for getting the major version string from a
> PostgresVersion object, again useful in subclassing.

WFM.

> 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.

I have done that as well when looking at the test business with
pg_upgrade.

> 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.

That happens.

> 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.
--
Michael

--2OXbw9+z5vRm9yFR
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmDb9IwACgkQnvQgOdby
QH0QYQ/8DWm0oTeJxDtEGKGenYnFQAGwKRWvsx6G/QjtWIPzzegw2al7UQxNG1JH
PSrRWQgavAq87i9pBXOF9K9/23F+pLnXz8ZaY28Czzf3HVV7hCa3GIcc9b8nkyP1
p9sqPsvXun114RKCwTcmsHrhPQgRq+c6iyKdBkyh+zGj8IoxvtyNBXN4dcmYOEvA
Dmm3ISjXTfG98UO1cqwwVPAuAl7L/NTFsea/tLUQkNn90SGWFTJNNnNSz6xfk1wF
yuko/YJ3pHSnQdV6Baltpeks0JcrI/AsgCThpdcttNbj6J2tMXZwrOuJqSNX0B7Q
yP2CygKmoIo4onqe0CUaT3m03OHqZxvrRPjMQ2OYQ8hyGOP62gzpfi0H5BihxWvq
fRqG9L78S58B7JPmbvtJ9SwPMPD2PLA74I4cXJTixb94HVMJzznqFNIJe9ivTukx
RBISSo7jxW6cwJAY9mJ9ojG6iFSkM7tloW+2kIY/rOFb59hwoCUc5+Muw6yYzeMb
XtZGa/fctsus8HdMMsD0rAitTkosBP0zbvyKYlDLBFPQWAy6GZXb6uDvHzzzgZ4n
LGTiiCTVMCB8RMAY0nXbDx2VHqvd0EKfu6I3jbzifw49FNqbz/dHOfBdIqWgIH4s
bk89w48ryUuwfRGijKmBxrF2QQFTHKOrbPLgwJOsqxC1RLH/8v0=
=B2pe
-----END PGP SIGNATURE-----

--2OXbw9+z5vRm9yFR--