On 2021-Apr-07, Mark Dilger wrote:
> It seems we're debating between two designs. In the first, each
> PostgresNode function knows about version limitations and has code
> like:
>
> DoSomething() if $self->at_least_version("11")
Yeah, I didn't like this approach -- it is quite messy.
> and in the second design we're subclassing for each postgres release
> where something changed, so that DoSomething is implemented
> differently in one class than another.
So DoSomething still does Something, regardless of what it has to do in
order to get it done.
> I think the subclassing solution is cleaner if the number of version
> tests is large, but not so much otherwise.
Well, your patch has rather a lot of at_least_version() tests.
> To wit:
>
> # "restart_after_crash" was introduced in version 9.1. Older versions
> # always restart after crash.
> print $conf "restart_after_crash = off\n"
> if $self->at_least_version("9.1");
>
> PostgresNode is mostly designed around supporting regression tests for
> the current postgres version under development.
I think we should make PostgresNode do what makes the most sense for the
current branch, and go to whatever contortions are necessary to do the
same thing in older versions as long as it is sensible. If we were
robots, then we would care to preserve behavior down to the very last
byte, but I think we can make judgement calls to ignore the changes that
are not relevant. Whenever we introduce a behavior that is not
supportable by the older version, then the function would throw an error
if that behavior is requested from that older version.
> Prior to Andrew's recent introduction of support for alternate
> installation paths, it made sense to have restart_after_crash be off.
> But now, if you spin up a postgres node for version 9.0 or before, you
> get different behavior, because the prior behavior is to implicitly
> have this "on", not "off".
That seems mostly okay, except in a very few narrow cases where the node
staying down is critical. So we can let things be.
> Again:
>
> # "log_replication_commands" was introduced in 9.5. Older versions do
> # not log replication commands.
> print $conf "log_replication_commands = on\n"
> if $self->at_least_version("9.5");
>
> Should we have "log_replication_commands" be off by default so that
> nodes of varying postgres version behave more similarly?
If it's important for the tests, then let's have a method to change it
if necessary. Otherwise, we don't care.
--
Álvaro Herrera Valdivia, Chile