Re: multi-install PostgresNode fails with older postgres versions - Mailing list pgsql-hackers

On 4/7/21 4:48 PM, Mark Dilger wrote:
>
>> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera <alvherre@alvh.no-ip.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
limitationsand 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
implementeddifferently in one class than another.  I think the subclassing solution is cleaner if the number of version
testsis 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
functionsthat work a certain way, but cannot work that same way with older versions of postgres that don't have the
necessarysupport.  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
avoidedit.  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_crashbe 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
wantit 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