On Wed, 7 Apr 2021 13:38:39 -0400
Andrew Dunstan <andrew@dunslane.net> wrote:
> On 4/7/21 1:19 PM, Jehan-Guillaume de Rorthais wrote:
> > On Wed, 7 Apr 2021 12:51:55 -0400
> > Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> >> On 2021-Apr-07, Jehan-Guillaume de Rorthais wrote:
> >>
> >>> When I'm creating a new node, I'm using the "pgaTester" factory class. It
> >>> relies on PATH to check the major version using pg_config, then loads the
> >>> appropriate class.
> >> From a code cleanliness point of view, I agree that having separate
> >> classes for each version is neater than what you call a forest of
> >> conditionals. I'm not sure I like the way you instantiate the classes
> >> in pgaTester though -- wouldn't it be saner to have PostgresNode::new
> >> itself be in charge of deciding which class to bless the object as?
> >> Since we're talking about modifying PostgresNode itself in order to
> >> support this, it would make sense to do that.
> > Yes, it would be much saner to make PostgresNode the factory class. Plus,
> > some more logic could be injected there to either auto-detect the version
> > (current behavior) or eg. use a given path to the binaries as Mark did in
> > its patch.
>
>
> Aren't you likely to end up duplicating substantial amounts of code,
> though? I'm certainly not at the stage where I think the version-aware
> code is creating too much clutter. The "forest of conditionals" seems
> more like a small thicket.
I started with a patched PostgresNode. Then I had to support backups and
replication for my tests. Then it become hard to follow the logic in
conditional blocks, moreover some conditionals needed to appear in multiple
places in the same methods, depending on the enabled features, the conditions,
what GUC was enabled by default or not, etc. So I end up with this design.
I really don't want to waste community brain cycles in discussions and useless
reviews. But as far as someone agree to review it, I already have the material
and I can create a patch with a limited amount of work to compare and review.
The one-class approach would need to support replication down to 9.0 to be fair
though :/
Thanks,
