Horton

Thread

Commits

  1. Make PostgresNode easily subclassable

  1. TAP: allow overriding PostgresNode in get_new_node

    Craig Ringer <craig@2ndquadrant.com> — 2017-05-31T00:43:34Z

    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

  2. Re: TAP: allow overriding PostgresNode in get_new_node

    Craig Ringer <craig@2ndquadrant.com> — 2017-06-01T02:18:46Z

    On 31 May 2017 at 08:43, Craig Ringer <craig@2ndquadrant.com> 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

  3. Re: TAP: allow overriding PostgresNode in get_new_node

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-01T07:36:03Z

    On Wed, May 31, 2017 at 7:18 PM, Craig Ringer <craig@2ndquadrant.com> 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

  4. Re: TAP: allow overriding PostgresNode in get_new_node

    Robert Haas <robertmhaas@gmail.com> — 2017-06-02T16:50:16Z

    On Thu, Jun 1, 2017 at 3:36 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, May 31, 2017 at 7:18 PM, Craig Ringer <craig@2ndquadrant.com> 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

  5. Re: TAP: allow overriding PostgresNode in get_new_node

    J Chapman Flack <jflack@math.purdue.edu> — 2017-06-02T19:47:12Z

    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

  6. Re: TAP: allow overriding PostgresNode in get_new_node

    Chapman Flack <chap@anastigmatix.net> — 2017-06-02T19:51:57Z

    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

  7. Re: TAP: allow overriding PostgresNode in get_new_node

    Chapman Flack <chap@anastigmatix.net> — 2017-06-08T01:42:30Z

    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

  8. Re: TAP: allow overriding PostgresNode in get_new_node

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-07-25T23:12:41Z

    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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: TAP: allow overriding PostgresNode in get_new_node

    Craig Ringer <craig@2ndquadrant.com> — 2017-07-26T00:45:12Z

    On 26 July 2017 at 07:12, Alvaro Herrera <alvherre@2ndquadrant.com> 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