Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On 26 July 2017 at 07:12, Alvaro Herrera wrote: > Chapman Flack wrote: > > > Any takers if I propose this amendment in the form of a patch? > > > > Relying on the perl idiom instead of a $node->isa() test shortens > > the patch; does that ameliorate at all the concern about complicating > > core for the benefit of modules? > > Yeah, I like this (I also got word from Abhijit Menon-Sen that this is a > saner way to do it, which counts as +1000 at least.) This is how we > should have built the method in the first place, rather than creating an > exported function. Not sure how we missed that. > > I changed the POD docs so that the class method version is the preferred > form, and the exported function usage "is just backwards compatibility". > All current usage uses that form, but hey, we can updated these later > (if at all). > > Pushed to 9.6 and HEAD. > Thanks. An upvote from our resident Perl wizard certainly does help :) -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
Chapman Flack wrote: > Any takers if I propose this amendment in the form of a patch? > > Relying on the perl idiom instead of a $node->isa() test shortens > the patch; does that ameliorate at all the concern about complicating > core for the benefit of modules? Yeah, I like this (I also got word from Abhijit Menon-Sen that this is a saner way to do it, which counts as +1000 at least.) This is how we should have built the method in the first place, rather than creating an exported function. Not sure how we missed that. I changed the POD docs so that the class method version is the preferred form, and the exported function usage "is just backwards compatibility". All current usage uses that form, but hey, we can updated these later (if at all). Pushed to 9.6 and HEAD. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On 06/02/17 15:51, Chapman Flack wrote: > But what it buys you is then if your MyExtraPGNode has PostgresNode > as a base, the familiar idiom > > MyExtraPGNode->get_new_node('foo'); > > works, as it inserts the class as the first argument. > > As a bonus, you then don't need to complicate get_new_node > with a test for (not ($node->isa("PostgresNode"))) because > if it weren't, it wouldn't have inherited get_new_node Any takers if I propose this amendment in the form of a patch? Relying on the perl idiom instead of a $node->isa() test shortens the patch; does that ameliorate at all the concern about complicating core for the benefit of modules? I'm not fully persuaded that just re-blessing a PostgresNode suffices as a workaround ... if the subclass expects to have additional state set up by its own constructor, the deception seems likely to be exposed. So I think there are more than just cosmetic grounds for allowing this. -Chap --- src/test/perl/PostgresNode.pm 2017-06-07 20:15:52.827639829 -0400 +++ src/test/perl/PostgresNode.pm 2017-06-07 20:57:49.205145761 -0400 @@ -853,20 +853,27 @@ =pod -=item get_new_node(node_name) +=item get_new_node(node_name) I PostgresNode->get_new_node(node_name) -Build a new PostgresNode object, assigning a free port number. Standalone -function that's automatically imported. +Build a new PostgresNode object, assigning a free port number. This can be +called either as a standalone function that's automatically imported, or as +a class method on PostgresNode or any subclass. Remembers the node, to prevent its port number from being reused for another node, and to ensure that it gets shut down when the test script exits. You should generally use this instead of PostgresNode::new(...). +If you have a subclass of PostgresNode you want created, use the class-method +form of the call, as in Cget_new_node(node_name)>. +The standalone-function form creates an instance of PostgresNode. + =cut sub get_new_node { + my $class = 'PostgresNode'; + $class = shift if 1 < scalar @_; my $name = shift; my $found = 0; my $port = $last_port_assigned; @@ -911,7 +918,7 @@ print "# Found free port $port\n"; # Lock port number found by creating a new node - my $node = new PostgresNode($name, $test_pghost, $port); + my $node = $class->new($name, $test_pghost, $port); # Add node to list of nodes push(@all_nodes, $node); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On 06/02/2017 12:50 PM, Robert Haas wrote: > On Thu, Jun 1, 2017 at 3:36 AM, Michael Paquier >> >> +$pgnclass = 'PostgresNode' unless defined $pgnclass; >> I'd rather leave any code of this kind for the module maintainers, > > Craig's proposal is a standard Perl idiom, though. Would it not be even more idiomatic to have the class, if present, be the first argument? That is: my $pgnclass = 'PostgresNode'; $pgnclass = shift if 1 < scalar @_; my $name = shift; That part's a little weird (an optional FIRST argument?) in order to preserve compatibility with callers who don't pass a class. But what it buys you is then if your MyExtraPGNode has PostgresNode as a base, the familiar idiom MyExtraPGNode->get_new_node('foo'); works, as it inserts the class as the first argument. As a bonus, you then don't need to complicate get_new_node with a test for (not ($node->isa("PostgresNode"))) because if it weren't, it wouldn't have inherited get_new_node so MyExtraPGNode->get_new_node('foo') would have failed. -Chap -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On 06/02/2017 12:50 PM, Robert Haas wrote: > On Thu, Jun 1, 2017 at 3:36 AM, Michael Paquier >> >> +$pgnclass = 'PostgresNode' unless defined $pgnclass; >> I'd rather leave any code of this kind for the module maintainers, > > Craig's proposal is a standard Perl idiom, though. Would it not be even more idiomatic to have the class, if present, be the first argument? That is: my $pgnclass = 'PostgresNode'; $pgnclass = shift if 1 < scalar @_; my $name = shift; That part's a little weird (an optional FIRST argument?) in order to preserve compatibility with callers who don't pass a class. But what it buys you is then if your MyExtraPGNode has PostgresNode as a base, the familiar idiom MyExtraPGNode->get_new_node('foo'); works, as it inserts the class as the first argument. As a bonus, you then don't need to complicate get_new_node with a test for (not ($node->isa("PostgresNode"))) because if it weren't, it wouldn't have inherited get_new_node so MyExtraPGNode->get_new_node('foo') would have failed. -Chap -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On Thu, Jun 1, 2017 at 3:36 AM, Michael Paquier wrote: > On Wed, May 31, 2017 at 7:18 PM, Craig Ringer wrote: >> Note that you can achieve the same effect w/o patching >> PostgresNode.pm, albeit in a somewhat ugly manner, by re-blessing the >> returned object. >> >> sub get_new_mywhatever_node { >> my $self = PostgresNode::get_new_node($name); >> $self = bless $self, 'MyWhateverNode'; >> return $self; >> } >> >> so this would be cosmetically nice, but far from functionally vital. > > +$pgnclass = 'PostgresNode' unless defined $pgnclass; > I'd rather leave any code of this kind for the module maintainers, > there is no actual reason to complicate PostgresNode.pm with code > that's not directly useful for what is in-core. Craig's proposal is a standard Perl idiom, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
Moin, On Wed, May 31, 2017 10:18 pm, Craig Ringer wrote: > On 31 May 2017 at 08:43, Craig Ringer wrote: >> Hi all >> >> More and more I'm finding it useful to extend PostgresNode for project >> specific helper classes. But PostgresNode::get_new_node is a factory >> that doesn't provide any mechanism for overriding, so you have to >> create a PostgresNode then re-bless it as your desired subclass. Ugly. >> >> The attached patch allows an optional second argument, a class name, >> to be passed to PostgresNode::get_new_node . It's instantiated instead >> of PostgresNode if supplied. Its 'new' constructor must take the same >> arguments. > > Note that you can achieve the same effect w/o patching > PostgresNode.pm, albeit in a somewhat ugly manner, by re-blessing the > returned object. > > sub get_new_mywhatever_node { > my $self = PostgresNode::get_new_node($name); > $self = bless $self, 'MyWhateverNode'; > return $self; > } > > so this would be cosmetically nice, but far from functionally vital. It's good style in Perl to have constructors bless new objects with the class that is passed in, tho. (I'd even go so far as to say that any Perl OO code that uses fixed class names is broken). While technically you can rebless a returned object, that breaks thge subclassing, sometimes subtle, and sometimes really bad. For instances, any method calls the constructor does, are happening in the "hardcoded" package, not in the subclass you are using, because the reblessing happens later. Consider for instance: package MyBase; sub new { my $self = bless {}, 'MyBase'; # it should be instead: # my $self = bless {}, shift; $self->_init(); } sub _init { my ($self) = @_; $self->{foo} = 'bar'; # return the initialized object $self; } If you do the this: package MySubclass; use MyBase; use vars qw/@ISA/; @ISA = qw/MyBase/; sub _init { my ($self) = @_; # call the base's _init $self->SUPER::_init(); # initialize our own stuff and override some $self->{foo} = 'subclass'; $self->{baz} = 1; # return the initialized object $self; } and try to use it like this: package main; use MySubclass; my $thingy = MySubclass->new(); print $thingy->{foo},"\n"; you get "bar", not "subclass" - even if you rebless $thingy into the correct class. And as someone who subclasses MyBase, you have no idea why or how and it will break with the next update to MyBase's code. While technically you can work around that by "peeking" into MyBase's code and maybe some reblessing, the point is that you shouldn't do nor need to do this. Please SEE: http://perldoc.perl.org/perlobj.html Regards, Tels -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On Wed, May 31, 2017 at 7:18 PM, Craig Ringer wrote: > Note that you can achieve the same effect w/o patching > PostgresNode.pm, albeit in a somewhat ugly manner, by re-blessing the > returned object. > > sub get_new_mywhatever_node { > my $self = PostgresNode::get_new_node($name); > $self = bless $self, 'MyWhateverNode'; > return $self; > } > > so this would be cosmetically nice, but far from functionally vital. +$pgnclass = 'PostgresNode' unless defined $pgnclass; I'd rather leave any code of this kind for the module maintainers, there is no actual reason to complicate PostgresNode.pm with code that's not directly useful for what is in-core. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] TAP: allow overriding PostgresNode in get_new_node
On 31 May 2017 at 08:43, Craig Ringer wrote: > Hi all > > More and more I'm finding it useful to extend PostgresNode for project > specific helper classes. But PostgresNode::get_new_node is a factory > that doesn't provide any mechanism for overriding, so you have to > create a PostgresNode then re-bless it as your desired subclass. Ugly. > > The attached patch allows an optional second argument, a class name, > to be passed to PostgresNode::get_new_node . It's instantiated instead > of PostgresNode if supplied. Its 'new' constructor must take the same > arguments. Note that you can achieve the same effect w/o patching PostgresNode.pm, albeit in a somewhat ugly manner, by re-blessing the returned object. sub get_new_mywhatever_node { my $self = PostgresNode::get_new_node($name); $self = bless $self, 'MyWhateverNode'; return $self; } so this would be cosmetically nice, but far from functionally vital. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] TAP: allow overriding PostgresNode in get_new_node
Hi all More and more I'm finding it useful to extend PostgresNode for project specific helper classes. But PostgresNode::get_new_node is a factory that doesn't provide any mechanism for overriding, so you have to create a PostgresNode then re-bless it as your desired subclass. Ugly. The attached patch allows an optional second argument, a class name, to be passed to PostgresNode::get_new_node . It's instantiated instead of PostgresNode if supplied. Its 'new' constructor must take the same arguments. BTW, it strikes me that we should really template a Perl file with the postgres version. Or finally add a --version-num to pg_config so we don't have to do version parsing. When you're writing extension TAP tests you often need to know the target postgres version and parsing our version strings, already annoying, got even more so with Pg 10. I prefer adding --version-num to pg_config. Any objections? Will submit patch if none. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services From 8a19793c89b165c25c88cd7149650e20ef27bd55 Mon Sep 17 00:00:00 2001 From: Craig Ringer Date: Tue, 30 May 2017 15:18:00 +0800 Subject: [PATCH v1] Allow creation of PostgresNode subclasses from get_new_node --- src/test/perl/PostgresNode.pm | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 42e66ed..9f28963 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -853,7 +853,7 @@ sub _update_pid =pod -=item get_new_node(node_name) +=item get_new_node(node_name, node_class) Build a new PostgresNode object, assigning a free port number. Standalone function that's automatically imported. @@ -863,14 +863,20 @@ node, and to ensure that it gets shut down when the test script exits. You should generally use this instead of PostgresNode::new(...). +If you have a subclass of PostgresNode you want created, pass its name as the +second argument. By default a 'PostgresNode' is created. + =cut sub get_new_node { my $name = shift; + my $pgnclass = shift; my $found = 0; my $port = $last_port_assigned; + $pgnclass = 'PostgresNode' unless defined $pgnclass; + while ($found == 0) { @@ -911,7 +917,9 @@ sub get_new_node print "# Found free port $port\n"; # Lock port number found by creating a new node - my $node = new PostgresNode($name, $test_pghost, $port); + my $node = $pgnclass->new($name, $test_pghost, $port); + + die "$pgnclass must have PostgresNode as a base" if (not ($node->isa("PostgresNode"))); # Add node to list of nodes push(@all_nodes, $node); -- 2.9.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers