Обсуждение: cleaning up PostgresNode.pm
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. These changes would make the module look more like a conventional perl module. 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". cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
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(). -- Álvaro Herrera Valdivia, Chile "If you have nothing to say, maybe you need just the right tool to help you not say it." (New York Times, about Microsoft PowerPoint)
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. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Вложения
- 0001-Add-w-back-to-the-flags-for-pg_ctl-start-in-Postgres.patch
- 0002-Add-adjust_conf-method-to-PostgresNode.patch
- 0003-Unify-PostgresNode-s-new-and-get_new_node-methods.patch
- 0004-Remove-the-last-vestiges-of-Exporter-from-PostgresNo.patch
- 0005-Add-a-method-to-PostgresVersion.pm-to-produce-the-ma.patch
- 0006-Add-a-getter-function-for-a-PostgresNode-install_pat.patch
- 0007-fixup-recovery-tests.patch
- 0008-fixup-README.patch
- 0009-new-recovery-fixups.patch
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
Вложения
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
On 2021-Jun-30, Andrew Dunstan wrote: > On 6/30/21 12:35 AM, Michael Paquier wrote: > > On Mon, Jun 28, 2021 at 01:02:37PM -0400, Andrew Dunstan wrote: > > 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. I thought it was about recovery.conf ... -- Álvaro Herrera Valdivia, Chile https://www.EnterpriseDB.com/
On 6/30/21 8:30 AM, Alvaro Herrera wrote: > On 2021-Jun-30, Andrew Dunstan wrote: > >> On 6/30/21 12:35 AM, Michael Paquier wrote: >>> On Mon, Jun 28, 2021 at 01:02:37PM -0400, Andrew Dunstan wrote: >>> 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. > I thought it was about recovery.conf ... Yes, that too. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
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
Вложения
- 0001-Add-w-back-to-the-flags-for-pg_ctl-start-in-Postgres.patch
- 0002-Add-adjust_conf-method-to-PostgresNode.patch
- 0003-Unify-PostgresNode-s-new-and-get_new_node-methods.patch
- 0004-Remove-the-last-vestiges-of-Exporter-from-PostgresNo.patch
- 0005-Add-a-method-to-PostgresVersion.pm-to-produce-the-ma.patch
- 0006-Add-a-getter-function-for-a-PostgresNode-install_pat.patch
- 0007-fixup-recovery-tests.patch
- 0008-fixup-README.patch
- 0009-new-recovery-fixups.patch
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
Вложения
- 0001-Add-w-back-to-the-flags-for-pg_ctl-re-start-in-Postg.patch
- 0002-Add-adjust_conf-method-to-PostgresNode.patch
- 0003-Unify-PostgresNode-s-new-and-get_new_node-methods.patch
- 0004-Remove-the-last-vestiges-of-Exporter-from-PostgresNo.patch
- 0005-Add-PostgresVersion.pm-method-to-emit-the-major-vers.patch
- 0006-Add-a-getter-function-for-a-PostgresNode-install_pat.patch
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. >>>> +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. > This time without a missing comma. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Вложения
- 0001-Add-w-back-to-the-flags-for-pg_ctl-re-start-in-Postg.patch
- 0002-Add-adjust_conf-method-to-PostgresNode.patch
- 0003-Unify-PostgresNode-s-new-and-get_new_node-methods.patch
- 0004-Remove-the-last-vestiges-of-Exporter-from-PostgresNo.patch
- 0005-Add-PostgresVersion.pm-method-to-emit-the-major-vers.patch
- 0006-Add-a-getter-function-for-a-PostgresNode-install_pat.patch