From b10436a3d8ffacf49d2db6bd621863de0c470c99 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Tue, 6 Apr 2021 21:31:20 -0700 Subject: [PATCH v3 1/2] Extending PostgresNode cross-version functionality Extending the recently introduced functionality that allows a PostgresNode to be formed using an older installation. First, adding functions that allow nodes to be compared, such as if ($some_node->newer_than_version($some_other_node)) { ... } if ($node_node->at_least_version("12.2")) { ... } Fixing PostgresNode functions init(), start(), psql(), safe_psql() to work with versions back to 8.0 and possibly before. Sanity check the install_path argument. Working around IPC::Run::run() bug/misfeature which caches paths for applications. This could cause nodes based on differing installation directories to sometimes execute each other's client applications rather than their own. --- src/test/perl/PostgresNode.pm | 316 +++++++++++++++++++++++++++++++--- 1 file changed, 296 insertions(+), 20 deletions(-) diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index e26b2b3f30..0c82796fc1 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -432,13 +432,24 @@ sub init local %ENV = $self->_get_env(); + # Check version support for requested features + BAIL_OUT("Server version too old: 'allows_streaming' option not supported") + if ($params{allows_streaming} && ! $self->at_least_version('9.4')); + $params{allows_streaming} = 0 unless defined $params{allows_streaming}; $params{has_archiving} = 0 unless defined $params{has_archiving}; mkdir $self->backup_dir; mkdir $self->archive_dir; - TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N', + # initdb option -N/--no-sync was added in version 9.3. If the server + # version is new enough, we can use this option to make the tests run + # faster by not waiting for the disks to sync. + my @initdb_nosync_opt = ('-N') + if $self->at_least_version("9.3"); + + TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', + @initdb_nosync_opt, @{ $params{extra} }); TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata, @{ $params{auth_extra} }); @@ -446,11 +457,23 @@ sub init open my $conf, '>>', "$pgdata/postgresql.conf"; print $conf "\n# Added by PostgresNode.pm\n"; print $conf "fsync = off\n"; - print $conf "restart_after_crash = off\n"; + + # "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"); print $conf "log_line_prefix = '%m [%p] %q%a '\n"; print $conf "log_statement = all\n"; - print $conf "log_replication_commands = on\n"; - print $conf "wal_retrieve_retry_interval = '500ms'\n"; + + # "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"); + + # "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"); # If a setting tends to affect whether tests pass or fail, print it after # TEMP_CONFIG. Otherwise, print it before TEMP_CONFIG, thereby permitting @@ -461,8 +484,10 @@ sub init # XXX Neutralize any stats_temp_directory in TEMP_CONFIG. Nodes running # concurrently must not share a stats_temp_directory. - print $conf "stats_temp_directory = 'pg_stat_tmp'\n"; + print $conf "stats_temp_directory = 'pg_stat_tmp'\n" + if ($self->at_least_version("8.4")); + # Streaming replication was introduced in version 9.4. if ($params{allows_streaming}) { if ($params{allows_streaming} eq "logical") @@ -483,21 +508,27 @@ sub init # limit disk space consumption, too: print $conf "max_wal_size = 128MB\n"; } - else + # "wal_level" and "max_wal_senders" were introduced in version 9.0 + elsif ($self->at_least_version("9.0")) { print $conf "wal_level = minimal\n"; print $conf "max_wal_senders = 0\n"; } print $conf "port = $port\n"; + + # The configuration option "unix_socket_directory" was renamed in 9.3 + my $unix_sockdir = "unix_socket_directory"; + $unix_sockdir = "unix_socket_directories" + if $self->at_least_version("9.3"); if ($use_tcp) { - print $conf "unix_socket_directories = ''\n"; + print $conf "$unix_sockdir = ''\n"; print $conf "listen_addresses = '$host'\n"; } else { - print $conf "unix_socket_directories = '$host'\n"; + print $conf "$unix_sockdir = '$host'\n"; print $conf "listen_addresses = ''\n"; } close $conf; @@ -731,8 +762,12 @@ port = $port } else { + # The configuration option "unix_socket_directory" was renamed in 9.3 + my $unix_sockdir = "unix_socket_directory"; + $unix_sockdir = "unix_socket_directories" + if $self->at_least_version("9.3"); $self->append_conf('postgresql.conf', - "unix_socket_directories = '$host'"); + "$unix_sockdir = '$host'"); } $self->enable_streaming($root_node) if $params{has_streaming}; $self->enable_restoring($root_node, $params{standby}) @@ -795,10 +830,17 @@ sub start # connections in confusing ways. local %ENV = $self->_get_env(PGAPPNAME => undef); - # Note: We set the cluster_name here, not in postgresql.conf (in - # sub init) so that it does not get copied to standbys. - $ret = TestLib::system_log('pg_ctl', '-D', $self->data_dir, '-l', - $self->logfile, '-o', "--cluster-name=$name", 'start'); + # GUC "cluster_name" was added in 9.5 and adds the name of the cluster to + # the process titles for server processes. For older servers, nothing will + # be added to the process titles. For newer servers, we set the + # cluster_name here, not in postgresql.conf (in sub init) so that it does + # not get copied to standbys. + my @cluster_name_option = ('-o', "--cluster-name=$name") + if $self->at_least_version('9.5'); + $ret = TestLib::system_log('pg_ctl', '-w', '-D', $self->data_dir, '-l', + $self->logfile, + @cluster_name_option, + 'start'); if ($ret != 0) { @@ -911,7 +953,7 @@ sub restart print "### Restarting node \"$name\"\n"; - TestLib::system_or_bail('pg_ctl', '-D', $pgdata, '-l', $logfile, + TestLib::system_or_bail('pg_ctl', '-w', '-D', $pgdata, '-l', $logfile, 'restart'); $self->_update_pid(1); @@ -1196,9 +1238,174 @@ sub get_new_node # Add node to list of nodes push(@all_nodes, $node); + # Get information about the node + $node->_read_pg_config; + return $node; } +# Private routine to run the pg_config binary found in our environment (or in +# our install_path, if we have one), and collect all fields that matter to us. +# +sub _read_pg_config +{ + my ($self) = @_; + my $inst = $self->{_install_path}; + my $pg_config = "pg_config"; + + if (defined $inst) + { + # If the _install_path is invalid, our PATH variables might find an + # unrelated pg_config executable elsewhere. Sanity check the + # directory. + BAIL_OUT("directory not found: $inst") + unless -d $inst; + + # If the directory exists but is not the root of a postgresql + # installation, or if the user configured using + # --bindir=$SOMEWHERE_ELSE, we're not going to find pg_config, so + # complain about that, too. + $pg_config = "$inst/bin/pg_config"; + BAIL_OUT("pg_config not found: $pg_config") + unless -e $pg_config; + BAIL_OUT("pg_config not executable: $pg_config") + unless -x $pg_config; + + # Leave $pg_config install_path qualified, to be sure we get the right + # version information, below, or die trying + } + + local %ENV = $self->_get_env(); + + # We only want the version field + open my $fh, "-|", $pg_config, "--version" + or + BAIL_OUT("$pg_config failed: $!"); + my $version_line = <$fh>; + close $fh or die; + + $self->{_pg_version} = _pg_version_array($version_line); + + BAIL_OUT("could not parse pg_config --version output: $version_line") + unless defined $self->{_pg_version}; +} + +# Private routine which returns a reference to an array of integers +# representing the pg_version of a PostgresNode, or parsed from a postgres +# version string. Development versions (such as "14devel") are converted +# to an array with minus one as the last value (such as [14, -1]). +# +# For idempotency, will return the argument back to the caller if handed an +# array reference. +sub _pg_version_array +{ + my ($arg) = @_; + + # accept node arguments + return _pg_version_array($arg->{_pg_version}) + if (blessed($arg) && $arg->isa("PostgresNode")); + + # idempotency + return $arg + if (ref($arg) && ref($arg) =~ /ARRAY/); + + # Accept standard formats, in case caller has handed us the output of a + # postgres command line tool + $arg = $1 + if ($arg =~ m/\(?PostgreSQL\)? (\d+(?:\.\d+)*(?:devel)?)/); + + # Split into an array + my @result = split(/\./, $arg); + + # Treat development versions as having a minor/micro version one less than + # the first released version of that branch. + if ($result[$#result] =~ m/^(\d+)devel$/) + { + pop(@result); + push(@result, $1, -1); + } + + # Return an array reference + return \@result; +} + +# Private routine which compares the _pg_version_array obtained for the two +# arguments and returns -1, 0, or 1, allowing comparison between two +# PostgresNodes or a PostgresNode and a version string. +# +# To achieve intuitive behavior when comparing a PostgresNode against a version +# string, "X" is equal to "X.Y" for any value of Y. This allows calls like +# +# $node->newer_than_version("14") +# +# to return true starting with "15devel", but false for "14devel", "14.0", +# "14.1", etc. It also allows +# +# $node->at_least_version("14") +# +# to work for a node of version "14devel", where comparing against "14.0" would +# fail. +# +sub _pg_version_cmp +{ + my ($a, $b) = @_; + + $a = _pg_version_array($a); + $b = _pg_version_array($b); + + for (my $idx = 0; ; $idx++) + { + return 0 unless (defined $a->[$idx] && defined $b->[$idx]); + return $a->[$idx] <=> $b->[$idx] + if ($a->[$idx] <=> $b->[$idx]); + } +} + +=pod + +=item $node->older_than_version(other) + +Returns whether this node's postgres version is older than "other", which can be +either another PostgresNode or a version string. + +=cut + +sub older_than_version +{ + my ($node, $pg_version) = @_; + return _pg_version_cmp($node->{_pg_version}, $pg_version) < 0; +} + +=pod + +=item $node->newer_than_version(other) + +Returns whether this node's postgres version is newer than "other", which can +be either another PostgresNode or a version string. + +=cut + +sub newer_than_version +{ + my ($node, $pg_version) = @_; + return _pg_version_cmp($node->{_pg_version}, $pg_version) > 0; +} + +=pod + +=item $node->at_least_version(other) + +Returns whether this node's postgres version is at least as new as "other", +which can be either another PostgresNode or a version string. + +=cut + +sub at_least_version +{ + my ($node, $pg_version) = @_; + return _pg_version_cmp($node->{_pg_version}, $pg_version) >= 0; +} + # Private routine to return a copy of the environment with the PATH and # (DY)LD_LIBRARY_PATH correctly set when there is an install path set for # the node. @@ -1271,6 +1478,28 @@ sub _get_env return (%inst_env); } +# Private routine to get an installation path qualified command. +# +# IPC::Run maintains a cache, %cmd_cache, mapping commands to paths. Tests +# which use nodes spanning more than one postgres installation path need to +# avoid confusing which installation's binaries get run. Setting $ENV{PATH} is +# insufficient, as IPC::Run does not check to see if the path has changed since +# caching a command. +sub installed_command +{ + my ($self, $cmd) = @_; + + # Nodes using alternate installation locations use their installation's + # bin/ directory explicitly + return join('/', $self->{_install_path}, 'bin', $cmd) + if defined $self->{_install_path}; + + # Nodes implicitly using the default installation location rely on IPC::Run + # to find the right binary, which should not cause %cmd_cache confusion, + # because no nodes with other installation paths do it that way. + return $cmd; +} + =pod =item get_free_port() @@ -1516,6 +1745,14 @@ is set to true if the psql call times out. If set, use this as the connection string for the connection to the backend. +=item host => B + +If this parameter is set, this host is used for the connection attempt. + +=item port => B + +If this parameter is set, this port is used for the connection attempt. + =item replication => B If set, add B to the conninfo string. @@ -1568,7 +1805,25 @@ sub psql } $psql_connstr .= defined $replication ? " replication=$replication" : ""; - my @psql_params = ('psql', '-XAtq', '-d', $psql_connstr, '-f', '-'); + # The -w/--no-password option was introduced in version 8.4 + my @no_password = ('-w') + if ($params{no_password} && $self->at_least_version('8.4')); + + my @host = ('-h', $params{host}) + if defined $params{host}; + my @port = ('-p', $params{port}) + if defined $params{port}; + + my @psql_params = ( + $self->installed_command('psql'), + '-XAtq', + @no_password, + @host, + @port, + '-d', + $psql_connstr, + '-f', + '-'); # If the caller wants an array and hasn't passed stdout/stderr # references, allocate temporary ones to capture them so we @@ -1754,7 +2009,7 @@ sub background_psql my $replication = $params{replication}; my @psql_params = ( - 'psql', + $self->installed_command('psql'), '-XAtq', '-d', $self->connstr($dbname) @@ -1831,7 +2086,11 @@ sub interactive_psql local %ENV = $self->_get_env(); - my @psql_params = ('psql', '-XAt', '-d', $self->connstr($dbname)); + my @psql_params = ( + $self->installed_command('psql'), + '-XAt', + '-d', + $self->connstr($dbname)); push @psql_params, @{ $params{extra_params} } if defined $params{extra_params}; @@ -1888,6 +2147,14 @@ If given, it must be an array reference containing a list of regular expressions that must NOT match against the server log. They will be passed to C. +=item host => B + +If this parameter is set, this host is used for the connection attempt. + +=item port => B + +If this parameter is set, this port is used for the connection attempt. + =back =cut @@ -1938,7 +2205,9 @@ sub connect_ok my ($ret, $stdout, $stderr) = $self->psql( 'postgres', $sql, - extra_params => ['-w'], + no_password => 1, + host => $params{host}, + port => $params{port}, connstr => "$connstr", on_error_stop => 0); @@ -2067,7 +2336,13 @@ sub poll_query_until $expected = 't' unless defined($expected); # default value - my $cmd = [ 'psql', '-XAt', '-c', $query, '-d', $self->connstr($dbname) ]; + my $cmd = [ + $self->installed_command('psql'), + '-XAt', + '-c', + $query, + '-d', + $self->connstr($dbname) ]; my ($stdout, $stderr); my $max_attempts = 180 * 10; my $attempts = 0; @@ -2489,7 +2764,8 @@ sub pg_recvlogical_upto croak 'endpos must be specified' unless defined($endpos); my @cmd = ( - 'pg_recvlogical', '-S', $slot_name, '--dbname', + $self->installed_command('pg_recvlogical'), + '-S', $slot_name, '--dbname', $self->connstr($dbname)); push @cmd, '--endpos', $endpos; push @cmd, '-f', '-', '--no-loop', '--start'; -- 2.21.1 (Apple Git-122.3)