> On Apr 7, 2021, at 1:28 PM, Alvaro Herrera <alvhe...@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 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.
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*?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
