Re: multi-install PostgresNode fails with older postgres versions

On 4/7/21 4:48 PM, Mark Dilger wrote:
>
>> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>>
>> On 2021-Apr-07, Mark Dilger wrote:
>>
>>> I was commenting on the design to have the PostgresNode derived
>>> subclass hard-coded to return "10" as the version:
>>>
>>> sub version { return 10 }
>> That seems a minor bug rather than a showstopper design deficiency.
>> I agree that hardcoding the version in the source code is not very
>> usable; it should store the version number when it runs pg_config
>> --version in an instance variable that can be returned.
> 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")
>
> 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. I think the subclassing solution is cleaner if the number of version tests is large, but not so much otherwise.

I think you and I are of one mind here.

>
>
> There is a much bigger design decision to be made that I have delayed making. The PostgresNode implementation has functions that work a certain way, but cannot work that same way with older versions of postgres that don't have the necessary support. This means that
>
> $my_node->do_something(...)
>
> works differently based on which version of postgres $my_node is based upon, even though PostgresNode could have avoided it. 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. 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".
>
> 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?
>
> Again:
>
> # "wal_retrieve_retry_interval" was introduced in 9.5. Older versions
> # always wait 5 seconds.
> print $conf "wal_retrieve_retry_interval = '500ms'\n"
> if $self->at_least_version("9.5");
>
>
> Should we have "wal_retrieve_retry_interval" be 5 seconds for consistency?
>
> I didn't do these things, as I didn't want to break the majority of tests which don't care about cross version compatibility, but if we're going to debate this thing, subclassing is a distraction. The real question is, *what do we want it to do*?
>
>

Yeah, much more important. I think I would say that anything that's
simply not possible in an older version should cause an error and for
the rest the old version should probably behave by default as much like
its default as possible. I don't think we should try to make different
versions behave identically (or as close to as possible). In some
particular cases we should be able to override default behavior (e.g. by
setting the config explicitly).

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com