public inbox for [email protected]
help / color / mirror / Atom feedRe: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
31+ messages / 7 participants
[nested] [flat]
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
@ 2022-02-16 04:58 Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-02-16 04:58 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Tue, Feb 15, 2022 at 01:02:41PM +0900, Michael Paquier wrote:
> Hmm. At the end of the day, I am wondering whether we should not give
> up entirely on the concept of running the regression tests on older
> branches in the TAP script of a newer branch. pg_regress needs to
> come from the old source tree, meaning that we would most likely need
> to maintain a set of compatibility tweaks that would most probably
> rot over the time, and the buildfarm only cares about the possibility
> to set up old instances by loading dumps rather than running
> pg_regress. This would also make the switch to TAP much easier (no
> need for the extra createdb or --locale AFAIK). So attempting to
> maintain all that is going to be a PITA in the long term, and there is
> nothing running that automatically anyway.
>
> There is also the extra requirement to adjust dump files, but that's
> independent of setting up the old instance to upgrade, and I don't
> really plan to tackle that as of this thread (note that the buildfarm
> client has extra tweaks regarding that).
>
> Any thoughts about that?
I have been looking at how much simplicity this brings, and I have to
admit that it is tempting to just support the loading of dumps when
setting up the old instance to upgrade from. We'd still need to do an
extra effort in terms of cleaning up the diffs for the dump of the old
instance with older versions once/if this is plugged into the
buildfarm, but that could be addressed later depending on the versions
that need to be covered.
--
Michael
Attachments:
[text/x-diff] v9-0001-Fix-sanity-check-for-PGHOST-ADDR-in-pg_upgrade-wi.patch (1.6K, ../../YgyEYkbf%[email protected]/2-v9-0001-Fix-sanity-check-for-PGHOST-ADDR-in-pg_upgrade-wi.patch)
download | inline diff:
From 7957ff3c2030a3a7df87ca0663a29deff69b385a Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 15 Feb 2022 12:05:28 +0900
Subject: [PATCH v9 1/2] Fix sanity check for PGHOST[ADDR] in pg_upgrade with
Windows paths
The checks currently done at the startup of pg_upgrade for PGHOST and
PGHOSTADDR to avoid any attempt to access to an external cluster would
fail when attempting to use Windows paths, or even temporary paths
prefixed by '@'. is_unixsock_path() is designed to detect such cases,
so use it rather than assuming that all valid paths are prefixed with a
slash.
Issue found while testing the tests of pg_upgrade through the CI on
Windows.
Based on an analysis from me and a solution from Andres Freund.
---
src/bin/pg_upgrade/server.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 7878d233de..265137e86b 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -11,6 +11,7 @@
#include "common/connect.h"
#include "fe_utils/string_utils.h"
+#include "libpq/pqcomm.h"
#include "pg_upgrade.h"
static PGconn *get_db_conn(ClusterInfo *cluster, const char *db_name);
@@ -368,7 +369,7 @@ check_pghost_envvar(void)
if (value && strlen(value) > 0 &&
/* check for 'local' host values */
(strcmp(value, "localhost") != 0 && strcmp(value, "127.0.0.1") != 0 &&
- strcmp(value, "::1") != 0 && value[0] != '/'))
+ strcmp(value, "::1") != 0 && !is_unixsock_path(value)))
pg_fatal("libpq environment variable %s has a non-local server value: %s\n",
option->envvar, value);
}
--
2.34.1
[text/x-diff] v9-0002-Switch-tests-of-pg_upgrade-to-use-TAP.patch (27.6K, ../../YgyEYkbf%[email protected]/3-v9-0002-Switch-tests-of-pg_upgrade-to-use-TAP.patch)
download | inline diff:
From ab8860e4bbc93d71bec3c875128dffe121bd282e Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Wed, 16 Feb 2022 13:50:35 +0900
Subject: [PATCH v9 2/2] Switch tests of pg_upgrade to use TAP
---
src/bin/pg_upgrade/Makefile | 21 +-
src/bin/pg_upgrade/TESTING | 85 ++------
src/bin/pg_upgrade/t/001_basic.pl | 9 +
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 255 ++++++++++++++++++++++
src/bin/pg_upgrade/test.sh | 279 -------------------------
src/tools/msvc/vcregress.pl | 92 +-------
6 files changed, 294 insertions(+), 447 deletions(-)
create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
delete mode 100644 src/bin/pg_upgrade/test.sh
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 49b94f0ac7..1f5d757548 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -28,6 +28,10 @@ OBJS = \
override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
+# required for 002_pg_upgrade.pl
+REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
+export REGRESS_SHLIB
+
all: pg_upgrade
pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
@@ -47,17 +51,8 @@ clean distclean maintainer-clean:
rm -rf delete_old_cluster.sh log/ tmp_check/ \
reindex_hash.sql
-# When $(MAKE) is present, make automatically infers that this is a
-# recursive make. which is not actually what we want here, as that
-# e.g. prevents output synchronization from working (as make thinks
-# that the subsidiary make knows how to deal with that itself, but
-# we're invoking a shell script that doesn't know). Referencing
-# $(MAKE) indirectly avoids that behaviour.
-# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
-NOTSUBMAKEMAKE=$(MAKE)
+check:
+ $(prove_check)
-check: test.sh all temp-install
- MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
-
-# installcheck is not supported because there's no meaningful way to test
-# pg_upgrade against a single already-running server
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 78b9747908..3718483a1c 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -2,78 +2,23 @@ THE SHORT VERSION
-----------------
On non-Windows machines, you can execute the testing process
-described below by running
+described below by running the following command in this directory:
make check
-in this directory. This will run the shell script test.sh, performing
-an upgrade from the version in this source tree to a new instance of
-the same version.
-To test an upgrade from a different version, you must have a built
-source tree for the old version as well as this version, and you
-must have done "make install" for both versions. Then do:
+This will run the TAP tests to run pg_upgrade, performing an upgrade
+from the version in this source tree to a new instance of the same
+version.
-export oldsrc=...somewhere/postgresql (old version's source tree)
-export oldbindir=...otherversion/bin (old version's installed bin dir)
-export bindir=...thisversion/bin (this version's installed bin dir)
-export libdir=...thisversion/lib (this version's installed lib dir)
-sh test.sh
+Testing an upgrade from a different version requires a dump to set up
+the contents of this instance, with its set of binaries. The following
+variables are available to control the test:
+export olddump=...somewhere/dump.sql (old version's dump)
+export oldinstall=...otherversion/ (old version's install base path)
-In this case, you will have to manually eyeball the resulting dump
-diff for version-specific differences, as explained below.
+Finally, the tests can be done by running
+ make check
-
-DETAILS
--------
-
-The most effective way to test pg_upgrade, aside from testing on user
-data, is by upgrading the PostgreSQL regression database.
-
-This testing process first requires the creation of a valid regression
-database dump. Such files contain most database features and are
-specific to each major version of Postgres.
-
-Here are the steps needed to create a regression database dump file:
-
-1) Create and populate the regression database in the old cluster.
- This database can be created by running 'make installcheck' from
- src/test/regress.
-
-2) Use pg_dump to dump out the regression database. Use the new
- cluster's pg_dump on the old database to minimize whitespace
- differences in the diff.
-
-3) Adjust the regression database dump file
-
- a) Perform the load/dump twice
- This fixes problems with the ordering of COPY columns for
- inherited tables.
-
- b) Change CREATE FUNCTION shared object paths to use '$libdir'
- The old and new cluster will have different shared object paths.
-
- c) Fix any wrapping format differences
- Commands like CREATE TRIGGER and ALTER TABLE sometimes have
- differences.
-
-Once the dump is created, it can be repeatedly loaded into the old
-database, upgraded, and dumped out of the new database, and then
-compared to the original version. To test the dump file, perform these
-steps:
-
-1) Create the old and new clusters in different directories.
-
-2) Copy the regression shared object files into the appropriate /lib
- directory for old and new clusters.
-
-3) Create the regression database in the old server.
-
-4) Load the dump file created above into the regression database;
- check for errors while loading.
-
-5) Upgrade the old database to the new major version, as outlined in
- the pg_upgrade manual section.
-
-6) Use pg_dump to dump out the regression database in the new cluster.
-
-7) Diff the regression database dump file with the regression dump
- file loaded into the old server.
+Using the test in this configuration is possible with any dump file.
+However, it is recommended to create a dump from the database "regression",
+created by running 'make installcheck' from src/test/regress as this covers
+most database features and are specific to each major version of Postgres.
diff --git a/src/bin/pg_upgrade/t/001_basic.pl b/src/bin/pg_upgrade/t/001_basic.pl
new file mode 100644
index 0000000000..75b0f98b08
--- /dev/null
+++ b/src/bin/pg_upgrade/t/001_basic.pl
@@ -0,0 +1,9 @@
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Utils;
+use Test::More tests => 8;
+
+program_help_ok('pg_upgrade');
+program_version_ok('pg_upgrade');
+program_options_handling_ok('pg_upgrade');
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
new file mode 100644
index 0000000000..7c314c9d26
--- /dev/null
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -0,0 +1,255 @@
+# Set of tests for pg_upgrade, including cross-version checks.
+use strict;
+use warnings;
+
+use Cwd qw(abs_path getcwd);
+use File::Basename qw(dirname);
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Generate a database with a name made of a range of ASCII characters.
+sub generate_db
+{
+ my ($node, $from_char, $to_char) = @_;
+
+ my $dbname = '';
+ for my $i ($from_char .. $to_char)
+ {
+ next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
+ $dbname = $dbname . sprintf('%c', $i);
+ }
+ $node->run_log(
+ [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
+ );
+}
+
+# The test of pg_upgrade consists in setting up an instance. This is the
+# source instance used for the upgrade. Then a new and fresh instance is
+# created, and is used as the target instance for the upgrade. Before
+# running an upgrade, a logical dump of the old instance is taken, and a
+# second logical dump of the new instance is taken after the upgrade.
+# The upgrade test passes if there are no differences in these two dumps.
+
+# Testing upgrades with an older instance of PostgreSQL requires setting up
+# two environment variables, as of:
+# - "olddump", to point to a dump file that will be used to set
+# up the old instance to upgrade from, the dump being restored in the
+# old cluster.
+# - "oldinstall", to point to the installation path of the old
+# instance.
+if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
+ || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
+{
+ # Not all variables are defined, so leave and die if test is
+ # done with an older installation.
+ die "olddump or oldinstall is undefined";
+}
+
+# Temporary location for the dumps taken
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
+# Initialize node to upgrade
+my $oldnode = PostgreSQL::Test::Cluster->new('old_node',
+ install_path => $ENV{oldinstall});
+
+$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+$oldnode->start;
+
+# The default location of the source code is the root of this directory
+# for the new and old nodes.
+my $srcdir = abs_path("../../..");
+
+# Set up the data of the old instance with pg_regress or an old dump.
+if (defined($ENV{olddump}))
+{
+ # Use the dump specified.
+ my $olddumpfile = $ENV{olddump};
+ die "no dump file found!" unless -e $olddumpfile;
+
+ # Load the dump, and we are done here.
+ $oldnode->command_ok(
+ [
+ 'psql', '-X', '-f', $olddumpfile,
+ '--port', $oldnode->port, '--host', $oldnode->host,
+ 'regression'
+ ]);
+}
+else
+{
+ # Default is to just use pg_regress to set up the old instance
+ # Creating databases with names covering most ASCII bytes
+ generate_db($oldnode, 1, 45);
+ generate_db($oldnode, 46, 90);
+ generate_db($oldnode, 91, 127);
+
+ # Grab any regression options that may be passed down by caller.
+ my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
+ my @extra_opts = split(/\s+/, $extra_opts_val);
+
+ # --dlpath is needed to be able to find the location of regress.so and
+ # any libraries the regression tests required. This needs to point to
+ # the old instance when using it. In the default case, fallback to
+ # what the caller provided for REGRESS_SHLIB.
+ my $dlpath =
+ dirname(PostgreSQL::Test::Utils::perl2host($ENV{REGRESS_SHLIB}));
+
+ # --outputdir points to the path where to place the output files.
+ my $outputdir =
+ PostgreSQL::Test::Utils::perl2host($PostgreSQL::Test::Utils::tmp_check);
+
+ # --inputdir points to the path of the input files.
+ my $inputdir =
+ PostgreSQL::Test::Utils::perl2host("$srcdir/src/test/regress");
+
+ my @regress_command = [
+ $ENV{PG_REGRESS},
+ '--dlpath',
+ $dlpath,
+ '--max-concurrent-tests',
+ '20',
+ '--bindir',
+ $oldnode->config_data('--bindir'),
+ '--host',
+ $oldnode->host,
+ '--port',
+ $oldnode->port,
+ '--schedule',
+ "$srcdir/src/test/regress/parallel_schedule",
+ '--outputdir',
+ $outputdir,
+ '--inputdir',
+ $inputdir
+ ];
+ @regress_command = (@regress_command, @extra_opts);
+
+ $oldnode->command_ok(@regress_command,
+ 'regression test run on old instance');
+}
+
+# Before dumping, get rid of objects not existing or not supported in later
+# versions. This depends on the version of the old server used, and matters
+# only if different versions are used for the dump.
+if (defined($ENV{oldinstall}))
+{
+ # Note that upgrade_adapt.sql from the new version is used, to
+ # cope with an upgrade to this version.
+ $oldnode->run_log(
+ [
+ 'psql', '-X', '-f',
+ PostgreSQL::Test::Utils::perl2host(
+ "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql"),
+ '--port',
+ $oldnode->port,
+ '--host',
+ $oldnode->host,
+ 'regression'
+ ]);
+}
+
+# Initialize a new node for the upgrade. This is done early so as it is
+# possible to know with which node's PATH the initial dump needs to be
+# taken.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my $newbindir = $newnode->config_data('--bindir');
+my $oldbindir = $oldnode->config_data('--bindir');
+
+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from the new node here.
+$newnode->command_ok(
+ [
+ 'pg_dumpall', '--no-sync',
+ '-d', $oldnode->connstr('postgres'),
+ '-f', "$tempdir/dump1.sql"
+ ],
+ 'dump before running pg_upgrade');
+
+# After dumping, update references to the old source tree's regress.so
+# to point to the new tree.
+if (defined($ENV{oldinstall}))
+{
+ # First, fetch all the references to libraries that are not part
+ # of the default path $libdir.
+ my $output = $oldnode->safe_psql('regression',
+ "SELECT DISTINCT probin::text FROM pg_proc WHERE probin NOT LIKE '\$libdir%';"
+ );
+ chomp($output);
+ my @libpaths = split("\n", $output);
+
+ my $dump_data = slurp_file("$tempdir/dump1.sql");
+
+ my $newregresssrc = "$srcdir/src/test/regress";
+ foreach (@libpaths)
+ {
+ my $libpath = $_;
+ $libpath = dirname($libpath);
+ $dump_data =~ s/$libpath/$newregresssrc/g;
+ }
+
+ open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump file";
+ print $fh $dump_data;
+ close $fh;
+
+ # This replaces any references to the old tree's regress.so
+ # the new tree's regress.so. Any references that do *not*
+ # match $libdir are switched so as this request does not
+ # depend on the path of the old source tree. This is useful
+ # when using an old dump. Do the operation on all the databases
+ # that allow connections so as this includes the regression
+ # database and anything the user has set up.
+ $output = $oldnode->safe_psql('postgres',
+ "SELECT datname FROM pg_database WHERE datallowconn;");
+ chomp($output);
+ my @datnames = split("\n", $output);
+ foreach (@datnames)
+ {
+ my $datname = $_;
+ $oldnode->safe_psql(
+ $datname, "UPDATE pg_proc SET probin =
+ regexp_replace(probin, '.*/', '$newregresssrc/')
+ WHERE probin NOT LIKE '\$libdir/%'");
+ }
+}
+
+# Upgrade the instance.
+$oldnode->stop;
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir, '-b', $oldbindir,
+ '-B', $newbindir, '-p', $oldnode->port,
+ '-P', $newnode->port
+ ],
+ 'run of pg_upgrade for new instance');
+$newnode->start;
+
+# Check if there are any logs coming from pg_upgrade, that would only be
+# retained on failure.
+my $log_path = $newnode->data_dir . "/pg_upgrade_output.d/log";
+if (-d $log_path)
+{
+ foreach my $log (glob("$log_path/*"))
+ {
+ note "###########################";
+ note "Contents of log file $log";
+ note "###########################";
+ my $log_contents = slurp_file($log);
+ print "$log_contents\n";
+ }
+}
+
+# Second dump from the upgraded instance.
+$newnode->run_log(
+ [
+ 'pg_dumpall', '--no-sync',
+ '-d', $newnode->connstr('postgres'),
+ '-f', "$tempdir/dump2.sql"
+ ]);
+
+# Compare the two dumps, there should be no differences.
+command_ok([ 'diff', '-q', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ],
+ 'old and new dump match after pg_upgrade');
+
+done_testing();
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
deleted file mode 100644
index ef328b3062..0000000000
--- a/src/bin/pg_upgrade/test.sh
+++ /dev/null
@@ -1,279 +0,0 @@
-#!/bin/sh
-
-# src/bin/pg_upgrade/test.sh
-#
-# Test driver for pg_upgrade. Initializes a new database cluster,
-# runs the regression tests (to put in some data), runs pg_dumpall,
-# runs pg_upgrade, runs pg_dumpall again, compares the dumps.
-#
-# Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
-# Portions Copyright (c) 1994, Regents of the University of California
-
-set -e
-
-: ${MAKE=make}
-
-# Guard against parallel make issues (see comments in pg_regress.c)
-unset MAKEFLAGS
-unset MAKELEVEL
-
-# Run a given "initdb" binary and overlay the regression testing
-# authentication configuration.
-standard_initdb() {
- # To increase coverage of non-standard segment size and group access
- # without increasing test runtime, run these tests with a custom setting.
- # Also, specify "-A trust" explicitly to suppress initdb's warning.
- # --allow-group-access and --wal-segsize have been added in v11.
- "$1" -N --wal-segsize 1 --allow-group-access -A trust
- if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
- then
- cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
- fi
- ../../test/regress/pg_regress --config-auth "$PGDATA"
-}
-
-# What flavor of host are we on?
-# Treat MINGW* (msys1) and MSYS* (msys2) the same.
-testhost=`uname -s | sed 's/^MSYS/MINGW/'`
-
-# Establish how the server will listen for connections
-case $testhost in
- MINGW*)
- LISTEN_ADDRESSES="localhost"
- PG_REGRESS_SOCKET_DIR=""
- PGHOST=localhost
- ;;
- *)
- LISTEN_ADDRESSES=""
- # Select a socket directory. The algorithm is from the "configure"
- # script; the outcome mimics pg_regress.c:make_temp_sockdir().
- if [ x"$PG_REGRESS_SOCKET_DIR" = x ]; then
- set +e
- dir=`(umask 077 &&
- mktemp -d /tmp/pg_upgrade_check-XXXXXX) 2>/dev/null`
- if [ ! -d "$dir" ]; then
- dir=/tmp/pg_upgrade_check-$$-$RANDOM
- (umask 077 && mkdir "$dir")
- if [ ! -d "$dir" ]; then
- echo "could not create socket temporary directory in \"/tmp\""
- exit 1
- fi
- fi
- set -e
- PG_REGRESS_SOCKET_DIR=$dir
- trap 'rm -rf "$PG_REGRESS_SOCKET_DIR"' 0
- trap 'exit 3' 1 2 13 15
- fi
- PGHOST=$PG_REGRESS_SOCKET_DIR
- ;;
-esac
-
-POSTMASTER_OPTS="-F -c listen_addresses=\"$LISTEN_ADDRESSES\" -k \"$PG_REGRESS_SOCKET_DIR\""
-export PGHOST
-
-# don't rely on $PWD here, as old shells don't set it
-temp_root=`pwd`/tmp_check
-rm -rf "$temp_root"
-mkdir "$temp_root"
-
-: ${oldbindir=$bindir}
-
-: ${oldsrc=../../..}
-oldsrc=`cd "$oldsrc" && pwd`
-newsrc=`cd ../../.. && pwd`
-
-# We need to make pg_regress use psql from the desired installation
-# (likely a temporary one), because otherwise the installcheck run
-# below would try to use psql from the proper installation directory
-# of the target version, which might be outdated or not exist. But
-# don't override anything else that's already in EXTRA_REGRESS_OPTS.
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$oldbindir'"
-export EXTRA_REGRESS_OPTS
-
-# While in normal cases this will already be set up, adding bindir to
-# path allows test.sh to be invoked with different versions as
-# described in ./TESTING
-PATH=$bindir:$PATH
-export PATH
-
-BASE_PGDATA="$temp_root/data"
-PGDATA="${BASE_PGDATA}.old"
-export PGDATA
-
-# Send installcheck outputs to a private directory. This avoids conflict when
-# check-world runs pg_upgrade check concurrently with src/test/regress check.
-# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs.
-outputdir="$temp_root/regress"
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
-export EXTRA_REGRESS_OPTS
-mkdir "$outputdir"
-
-# pg_regress --make-tablespacedir would take care of that in 14~, but this is
-# still required for older versions where this option is not supported.
-if [ "$newsrc" != "$oldsrc" ]; then
- mkdir "$outputdir"/testtablespace
- mkdir "$outputdir"/sql
- mkdir "$outputdir"/expected
-fi
-
-logdir=`pwd`/log
-rm -rf "$logdir"
-mkdir "$logdir"
-
-# Clear out any environment vars that might cause libpq to connect to
-# the wrong postmaster (cf pg_regress.c)
-#
-# Some shells, such as NetBSD's, return non-zero from unset if the variable
-# is already unset. Since we are operating under 'set -e', this causes the
-# script to fail. To guard against this, set them all to an empty string first.
-PGDATABASE=""; unset PGDATABASE
-PGUSER=""; unset PGUSER
-PGSERVICE=""; unset PGSERVICE
-PGSSLMODE=""; unset PGSSLMODE
-PGREQUIRESSL=""; unset PGREQUIRESSL
-PGCONNECT_TIMEOUT=""; unset PGCONNECT_TIMEOUT
-PGHOSTADDR=""; unset PGHOSTADDR
-
-# Select a non-conflicting port number, similarly to pg_regress.c
-PG_VERSION_NUM=`grep '#define PG_VERSION_NUM' "$newsrc"/src/include/pg_config.h | awk '{print $3}'`
-PGPORT=`expr $PG_VERSION_NUM % 16384 + 49152`
-export PGPORT
-
-i=0
-while psql -X postgres </dev/null 2>/dev/null
-do
- i=`expr $i + 1`
- if [ $i -eq 16 ]
- then
- echo port $PGPORT apparently in use
- exit 1
- fi
- PGPORT=`expr $PGPORT + 1`
- export PGPORT
-done
-
-# buildfarm may try to override port via EXTRA_REGRESS_OPTS ...
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --port=$PGPORT"
-export EXTRA_REGRESS_OPTS
-
-standard_initdb "$oldbindir"/initdb
-"$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
-
-# Create databases with names covering the ASCII bytes other than NUL, BEL,
-# LF, or CR. BEL would ring the terminal bell in the course of this test, and
-# it is not otherwise a special case. PostgreSQL doesn't support the rest.
-dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
- if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
-# Exercise backslashes adjacent to double quotes, a Windows special case.
-dbname1='\"\'$dbname1'\\"\\\'
-dbname2=`awk 'BEGIN { for (i = 46; i < 91; i++) printf "%c", i }' </dev/null`
-dbname3=`awk 'BEGIN { for (i = 91; i < 128; i++) printf "%c", i }' </dev/null`
-createdb "regression$dbname1" || createdb_status=$?
-createdb "regression$dbname2" || createdb_status=$?
-createdb "regression$dbname3" || createdb_status=$?
-
-# Extra options to apply to the dump. This may be changed later.
-extra_dump_options=""
-
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
- oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
-
- # Before dumping, tweak the database of the old instance depending
- # on its version.
- if [ "$newsrc" != "$oldsrc" ]; then
- # This SQL script has its own idea of the cleanup that needs to be
- # done on the cluster to-be-upgraded, and includes version checks.
- # Note that this uses the script stored on the new branch.
- psql -X -d regression -f "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql" \
- || psql_fix_sql_status=$?
-
- # Handling of --extra-float-digits gets messy after v12.
- # Note that this changes the dumps from the old and new
- # instances if involving an old cluster of v11 or older.
- if [ $oldpgversion -lt 120000 ]; then
- extra_dump_options="--extra-float-digits=0"
- fi
- fi
-
- pg_dumpall $extra_dump_options --no-sync \
- -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
-
- if [ "$newsrc" != "$oldsrc" ]; then
- # update references to old source tree's regress.so etc
- fix_sql=""
- case $oldpgversion in
- *)
- fix_sql="UPDATE pg_proc SET probin = replace(probin, '$oldsrc', '$newsrc') WHERE probin LIKE '$oldsrc%';"
- ;;
- esac
- psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
-
- mv "$temp_root"/dump1.sql "$temp_root"/dump1.sql.orig
- sed "s;$oldsrc;$newsrc;g" "$temp_root"/dump1.sql.orig >"$temp_root"/dump1.sql
- fi
-else
- make_installcheck_status=$?
-fi
-"$oldbindir"/pg_ctl -m fast stop
-if [ -n "$createdb_status" ]; then
- exit 1
-fi
-if [ -n "$make_installcheck_status" ]; then
- exit 1
-fi
-if [ -n "$psql_fix_sql_status" ]; then
- exit 1
-fi
-if [ -n "$pg_dumpall1_status" ]; then
- echo "pg_dumpall of pre-upgrade database cluster failed"
- exit 1
-fi
-
-PGDATA="$BASE_PGDATA"
-
-standard_initdb 'initdb'
-
-pg_upgrade $PG_UPGRADE_OPTS --no-sync -d "${PGDATA}.old" -D "$PGDATA" -b "$oldbindir" -p "$PGPORT" -P "$PGPORT"
-
-# make sure all directories and files have group permissions, on Unix hosts
-# Windows hosts don't support Unix-y permissions.
-case $testhost in
- MINGW*|CYGWIN*) ;;
- *) if [ `find "$PGDATA" -type f ! -perm 640 | wc -l` -ne 0 ]; then
- echo "files in PGDATA with permission != 640";
- exit 1;
- fi ;;
-esac
-
-case $testhost in
- MINGW*|CYGWIN*) ;;
- *) if [ `find "$PGDATA" -type d ! -perm 750 | wc -l` -ne 0 ]; then
- echo "directories in PGDATA with permission != 750";
- exit 1;
- fi ;;
-esac
-
-pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
-
-pg_dumpall $extra_dump_options --no-sync \
- -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
-pg_ctl -m fast stop
-
-if [ -n "$pg_dumpall2_status" ]; then
- echo "pg_dumpall of post-upgrade database cluster failed"
- exit 1
-fi
-
-case $testhost in
- MINGW*) MSYS2_ARG_CONV_EXCL=/c cmd /c delete_old_cluster.bat ;;
- *) sh ./delete_old_cluster.sh ;;
-esac
-
-if diff "$temp_root"/dump1.sql "$temp_root"/dump2.sql >/dev/null; then
- echo PASSED
- exit 0
-else
- echo "Files $temp_root/dump1.sql and $temp_root/dump2.sql differ"
- echo "dumps were not identical"
- exit 1
-fi
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index a994626239..39d1e49721 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -285,6 +285,10 @@ sub bincheck
foreach my $dir (@bin_dirs)
{
next unless -d "$dir/t";
+ # Do not consider pg_upgrade, as it is handled by
+ # upgradecheck.
+ next if ($dir =~ "/pg_upgrade/");
+
my $status = tap_check($dir);
$mstat ||= $status;
}
@@ -589,91 +593,9 @@ sub generate_db
sub upgradecheck
{
- my $status;
- my $cwd = getcwd();
-
- # Much of this comes from the pg_upgrade test.sh script,
- # but it only covers the --install case, and not the case
- # where the old and new source or bin dirs are different.
- # i.e. only this version to this version check. That's
- # what pg_upgrade's "make check" does.
-
- $ENV{PGHOST} = 'localhost';
- $ENV{PGPORT} ||= 50432;
- my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check";
- rmtree($tmp_root);
- mkdir $tmp_root || die $!;
- my $upg_tmp_install = "$tmp_root/install"; # unshared temp install
- print "Setting up temp install\n\n";
- Install($upg_tmp_install, "all", $config);
-
- # Install does a chdir, so change back after that
- chdir $cwd;
- my ($bindir, $libdir, $oldsrc, $newsrc) =
- ("$upg_tmp_install/bin", "$upg_tmp_install/lib", $topdir, $topdir);
- $ENV{PATH} = "$bindir;$ENV{PATH}";
- my $data = "$tmp_root/data";
- $ENV{PGDATA} = "$data.old";
- my $outputdir = "$tmp_root/regress";
- my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
- mkdir "$outputdir" || die $!;
-
- my $logdir = "$topdir/src/bin/pg_upgrade/log";
- rmtree($logdir);
- mkdir $logdir || die $!;
- print "\nRunning initdb on old cluster\n\n";
- standard_initdb() or exit 1;
- print "\nStarting old cluster\n\n";
- my @args = ('pg_ctl', 'start', '-l', "$logdir/postmaster1.log");
- system(@args) == 0 or exit 1;
-
- print "\nCreating databases with names covering most ASCII bytes\n\n";
- generate_db("\\\"\\", 1, 45, "\\\\\"\\\\\\");
- generate_db('', 46, 90, '');
- generate_db('', 91, 127, '');
-
- print "\nSetting up data for upgrading\n\n";
- installcheck_internal('parallel', @EXTRA_REGRESS_OPTS);
-
- # now we can chdir into the source dir
- chdir "$topdir/src/bin/pg_upgrade";
- print "\nDumping old cluster\n\n";
- @args = ('pg_dumpall', '-f', "$tmp_root/dump1.sql");
- system(@args) == 0 or exit 1;
- print "\nStopping old cluster\n\n";
- system("pg_ctl stop") == 0 or exit 1;
- $ENV{PGDATA} = "$data";
- print "\nSetting up new cluster\n\n";
- standard_initdb() or exit 1;
- print "\nRunning pg_upgrade\n\n";
- @args = (
- 'pg_upgrade', '-d', "$data.old", '-D', $data, '-b', $bindir,
- '--no-sync');
- system(@args) == 0 or exit 1;
- print "\nStarting new cluster\n\n";
- @args = ('pg_ctl', '-l', "$logdir/postmaster2.log", 'start');
- system(@args) == 0 or exit 1;
- print "\nDumping new cluster\n\n";
- @args = ('pg_dumpall', '-f', "$tmp_root/dump2.sql");
- system(@args) == 0 or exit 1;
- print "\nStopping new cluster\n\n";
- system("pg_ctl stop") == 0 or exit 1;
- print "\nDeleting old cluster\n\n";
- system(".\\delete_old_cluster.bat") == 0 or exit 1;
- print "\nComparing old and new cluster dumps\n\n";
-
- @args = ('diff', '-q', "$tmp_root/dump1.sql", "$tmp_root/dump2.sql");
- system(@args);
- $status = $?;
- if (!$status)
- {
- print "PASSED\n";
- }
- else
- {
- print "dumps not identical!\n";
- exit(1);
- }
+ InstallTemp();
+ my $mstat = tap_check("$topdir/src/bin/pg_upgrade");
+ exit $mstat if $mstat;
return;
}
--
2.34.1
[application/pgp-signature] signature.asc (833B, ../../YgyEYkbf%[email protected]/4-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-03-02 06:57 ` Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-03-02 06:57 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Wed, Feb 16, 2022 at 01:58:10PM +0900, Michael Paquier wrote:
> I have been looking at how much simplicity this brings, and I have to
> admit that it is tempting to just support the loading of dumps when
> setting up the old instance to upgrade from. We'd still need to do an
> extra effort in terms of cleaning up the diffs for the dump of the old
> instance with older versions once/if this is plugged into the
> buildfarm, but that could be addressed later depending on the versions
> that need to be covered.
The bug related to the detection of Windows and temporary paths for
pg_upgrade's server.c has been fixed as of dc57366, so attached is the
remaining rebased piece as perl2host has been recently removed.
Do others have an opinion about a backpatch of the bugfix? Nobody has
complained about that since pg_upgrade exists, so I have just done the
change on HEAD.
--
Michael
Attachments:
[text/x-diff] v10-0001-Switch-tests-of-pg_upgrade-to-use-TAP.patch (27.5K, ../../[email protected]/2-v10-0001-Switch-tests-of-pg_upgrade-to-use-TAP.patch)
download | inline diff:
From 66b6961866d215344aa836963e5bdbfb237ed606 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Wed, 2 Mar 2022 15:55:32 +0900
Subject: [PATCH v10] Switch tests of pg_upgrade to use TAP
---
src/bin/pg_upgrade/Makefile | 21 +-
src/bin/pg_upgrade/TESTING | 85 ++------
src/bin/pg_upgrade/t/001_basic.pl | 9 +
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 251 ++++++++++++++++++++++
src/bin/pg_upgrade/test.sh | 279 -------------------------
src/tools/msvc/vcregress.pl | 92 +-------
6 files changed, 290 insertions(+), 447 deletions(-)
create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
delete mode 100644 src/bin/pg_upgrade/test.sh
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 49b94f0ac7..1f5d757548 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -28,6 +28,10 @@ OBJS = \
override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
+# required for 002_pg_upgrade.pl
+REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
+export REGRESS_SHLIB
+
all: pg_upgrade
pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
@@ -47,17 +51,8 @@ clean distclean maintainer-clean:
rm -rf delete_old_cluster.sh log/ tmp_check/ \
reindex_hash.sql
-# When $(MAKE) is present, make automatically infers that this is a
-# recursive make. which is not actually what we want here, as that
-# e.g. prevents output synchronization from working (as make thinks
-# that the subsidiary make knows how to deal with that itself, but
-# we're invoking a shell script that doesn't know). Referencing
-# $(MAKE) indirectly avoids that behaviour.
-# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
-NOTSUBMAKEMAKE=$(MAKE)
+check:
+ $(prove_check)
-check: test.sh all temp-install
- MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
-
-# installcheck is not supported because there's no meaningful way to test
-# pg_upgrade against a single already-running server
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 78b9747908..3718483a1c 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -2,78 +2,23 @@ THE SHORT VERSION
-----------------
On non-Windows machines, you can execute the testing process
-described below by running
+described below by running the following command in this directory:
make check
-in this directory. This will run the shell script test.sh, performing
-an upgrade from the version in this source tree to a new instance of
-the same version.
-To test an upgrade from a different version, you must have a built
-source tree for the old version as well as this version, and you
-must have done "make install" for both versions. Then do:
+This will run the TAP tests to run pg_upgrade, performing an upgrade
+from the version in this source tree to a new instance of the same
+version.
-export oldsrc=...somewhere/postgresql (old version's source tree)
-export oldbindir=...otherversion/bin (old version's installed bin dir)
-export bindir=...thisversion/bin (this version's installed bin dir)
-export libdir=...thisversion/lib (this version's installed lib dir)
-sh test.sh
+Testing an upgrade from a different version requires a dump to set up
+the contents of this instance, with its set of binaries. The following
+variables are available to control the test:
+export olddump=...somewhere/dump.sql (old version's dump)
+export oldinstall=...otherversion/ (old version's install base path)
-In this case, you will have to manually eyeball the resulting dump
-diff for version-specific differences, as explained below.
+Finally, the tests can be done by running
+ make check
-
-DETAILS
--------
-
-The most effective way to test pg_upgrade, aside from testing on user
-data, is by upgrading the PostgreSQL regression database.
-
-This testing process first requires the creation of a valid regression
-database dump. Such files contain most database features and are
-specific to each major version of Postgres.
-
-Here are the steps needed to create a regression database dump file:
-
-1) Create and populate the regression database in the old cluster.
- This database can be created by running 'make installcheck' from
- src/test/regress.
-
-2) Use pg_dump to dump out the regression database. Use the new
- cluster's pg_dump on the old database to minimize whitespace
- differences in the diff.
-
-3) Adjust the regression database dump file
-
- a) Perform the load/dump twice
- This fixes problems with the ordering of COPY columns for
- inherited tables.
-
- b) Change CREATE FUNCTION shared object paths to use '$libdir'
- The old and new cluster will have different shared object paths.
-
- c) Fix any wrapping format differences
- Commands like CREATE TRIGGER and ALTER TABLE sometimes have
- differences.
-
-Once the dump is created, it can be repeatedly loaded into the old
-database, upgraded, and dumped out of the new database, and then
-compared to the original version. To test the dump file, perform these
-steps:
-
-1) Create the old and new clusters in different directories.
-
-2) Copy the regression shared object files into the appropriate /lib
- directory for old and new clusters.
-
-3) Create the regression database in the old server.
-
-4) Load the dump file created above into the regression database;
- check for errors while loading.
-
-5) Upgrade the old database to the new major version, as outlined in
- the pg_upgrade manual section.
-
-6) Use pg_dump to dump out the regression database in the new cluster.
-
-7) Diff the regression database dump file with the regression dump
- file loaded into the old server.
+Using the test in this configuration is possible with any dump file.
+However, it is recommended to create a dump from the database "regression",
+created by running 'make installcheck' from src/test/regress as this covers
+most database features and are specific to each major version of Postgres.
diff --git a/src/bin/pg_upgrade/t/001_basic.pl b/src/bin/pg_upgrade/t/001_basic.pl
new file mode 100644
index 0000000000..75b0f98b08
--- /dev/null
+++ b/src/bin/pg_upgrade/t/001_basic.pl
@@ -0,0 +1,9 @@
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Utils;
+use Test::More tests => 8;
+
+program_help_ok('pg_upgrade');
+program_version_ok('pg_upgrade');
+program_options_handling_ok('pg_upgrade');
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
new file mode 100644
index 0000000000..7e8b48497b
--- /dev/null
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -0,0 +1,251 @@
+# Set of tests for pg_upgrade, including cross-version checks.
+use strict;
+use warnings;
+
+use Cwd qw(abs_path getcwd);
+use File::Basename qw(dirname);
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Generate a database with a name made of a range of ASCII characters.
+sub generate_db
+{
+ my ($node, $from_char, $to_char) = @_;
+
+ my $dbname = '';
+ for my $i ($from_char .. $to_char)
+ {
+ next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
+ $dbname = $dbname . sprintf('%c', $i);
+ }
+ $node->run_log(
+ [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
+ );
+}
+
+# The test of pg_upgrade consists in setting up an instance. This is the
+# source instance used for the upgrade. Then a new and fresh instance is
+# created, and is used as the target instance for the upgrade. Before
+# running an upgrade, a logical dump of the old instance is taken, and a
+# second logical dump of the new instance is taken after the upgrade.
+# The upgrade test passes if there are no differences in these two dumps.
+
+# Testing upgrades with an older instance of PostgreSQL requires setting up
+# two environment variables, as of:
+# - "olddump", to point to a dump file that will be used to set
+# up the old instance to upgrade from, the dump being restored in the
+# old cluster.
+# - "oldinstall", to point to the installation path of the old
+# instance.
+if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
+ || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
+{
+ # Not all variables are defined, so leave and die if test is
+ # done with an older installation.
+ die "olddump or oldinstall is undefined";
+}
+
+# Temporary location for the dumps taken
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
+# Initialize node to upgrade
+my $oldnode = PostgreSQL::Test::Cluster->new('old_node',
+ install_path => $ENV{oldinstall});
+
+$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+$oldnode->start;
+
+# The default location of the source code is the root of this directory
+# for the new and old nodes.
+my $srcdir = abs_path("../../..");
+
+# Set up the data of the old instance with pg_regress or an old dump.
+if (defined($ENV{olddump}))
+{
+ # Use the dump specified.
+ my $olddumpfile = $ENV{olddump};
+ die "no dump file found!" unless -e $olddumpfile;
+
+ # Load the dump, and we are done here.
+ $oldnode->command_ok(
+ [
+ 'psql', '-X', '-f', $olddumpfile,
+ '--port', $oldnode->port, '--host', $oldnode->host,
+ 'regression'
+ ]);
+}
+else
+{
+ # Default is to just use pg_regress to set up the old instance
+ # Creating databases with names covering most ASCII bytes
+ generate_db($oldnode, 1, 45);
+ generate_db($oldnode, 46, 90);
+ generate_db($oldnode, 91, 127);
+
+ # Grab any regression options that may be passed down by caller.
+ my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
+ my @extra_opts = split(/\s+/, $extra_opts_val);
+
+ # --dlpath is needed to be able to find the location of regress.so and
+ # any libraries the regression tests required. This needs to point to
+ # the old instance when using it. In the default case, fallback to
+ # what the caller provided for REGRESS_SHLIB.
+ my $dlpath = dirname($ENV{REGRESS_SHLIB});
+
+ # --outputdir points to the path where to place the output files.
+ my $outputdir = $PostgreSQL::Test::Utils::tmp_check;
+
+ # --inputdir points to the path of the input files.
+ my $inputdir = "$srcdir/src/test/regress";
+
+ my @regress_command = [
+ $ENV{PG_REGRESS},
+ '--dlpath',
+ $dlpath,
+ '--max-concurrent-tests',
+ '20',
+ '--bindir',
+ $oldnode->config_data('--bindir'),
+ '--host',
+ $oldnode->host,
+ '--port',
+ $oldnode->port,
+ '--schedule',
+ "$srcdir/src/test/regress/parallel_schedule",
+ '--outputdir',
+ $outputdir,
+ '--inputdir',
+ $inputdir
+ ];
+ @regress_command = (@regress_command, @extra_opts);
+
+ $oldnode->command_ok(@regress_command,
+ 'regression test run on old instance');
+}
+
+# Before dumping, get rid of objects not existing or not supported in later
+# versions. This depends on the version of the old server used, and matters
+# only if different versions are used for the dump.
+if (defined($ENV{oldinstall}))
+{
+ # Note that upgrade_adapt.sql from the new version is used, to
+ # cope with an upgrade to this version.
+ $oldnode->run_log(
+ [
+ 'psql', '-X', '-f',
+ "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
+ '--port',
+ $oldnode->port,
+ '--host',
+ $oldnode->host,
+ 'regression'
+ ]);
+}
+
+# Initialize a new node for the upgrade. This is done early so as it is
+# possible to know with which node's PATH the initial dump needs to be
+# taken.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my $newbindir = $newnode->config_data('--bindir');
+my $oldbindir = $oldnode->config_data('--bindir');
+
+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from the new node here.
+$newnode->command_ok(
+ [
+ 'pg_dumpall', '--no-sync',
+ '-d', $oldnode->connstr('postgres'),
+ '-f', "$tempdir/dump1.sql"
+ ],
+ 'dump before running pg_upgrade');
+
+# After dumping, update references to the old source tree's regress.so
+# to point to the new tree.
+if (defined($ENV{oldinstall}))
+{
+ # First, fetch all the references to libraries that are not part
+ # of the default path $libdir.
+ my $output = $oldnode->safe_psql('regression',
+ "SELECT DISTINCT probin::text FROM pg_proc WHERE probin NOT LIKE '\$libdir%';"
+ );
+ chomp($output);
+ my @libpaths = split("\n", $output);
+
+ my $dump_data = slurp_file("$tempdir/dump1.sql");
+
+ my $newregresssrc = "$srcdir/src/test/regress";
+ foreach (@libpaths)
+ {
+ my $libpath = $_;
+ $libpath = dirname($libpath);
+ $dump_data =~ s/$libpath/$newregresssrc/g;
+ }
+
+ open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump file";
+ print $fh $dump_data;
+ close $fh;
+
+ # This replaces any references to the old tree's regress.so
+ # the new tree's regress.so. Any references that do *not*
+ # match $libdir are switched so as this request does not
+ # depend on the path of the old source tree. This is useful
+ # when using an old dump. Do the operation on all the databases
+ # that allow connections so as this includes the regression
+ # database and anything the user has set up.
+ $output = $oldnode->safe_psql('postgres',
+ "SELECT datname FROM pg_database WHERE datallowconn;");
+ chomp($output);
+ my @datnames = split("\n", $output);
+ foreach (@datnames)
+ {
+ my $datname = $_;
+ $oldnode->safe_psql(
+ $datname, "UPDATE pg_proc SET probin =
+ regexp_replace(probin, '.*/', '$newregresssrc/')
+ WHERE probin NOT LIKE '\$libdir/%'");
+ }
+}
+
+# Upgrade the instance.
+$oldnode->stop;
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir, '-b', $oldbindir,
+ '-B', $newbindir, '-p', $oldnode->port,
+ '-P', $newnode->port
+ ],
+ 'run of pg_upgrade for new instance');
+$newnode->start;
+
+# Check if there are any logs coming from pg_upgrade, that would only be
+# retained on failure.
+my $log_path = $newnode->data_dir . "/pg_upgrade_output.d/log";
+if (-d $log_path)
+{
+ foreach my $log (glob("$log_path/*"))
+ {
+ note "###########################";
+ note "Contents of log file $log";
+ note "###########################";
+ my $log_contents = slurp_file($log);
+ print "$log_contents\n";
+ }
+}
+
+# Second dump from the upgraded instance.
+$newnode->run_log(
+ [
+ 'pg_dumpall', '--no-sync',
+ '-d', $newnode->connstr('postgres'),
+ '-f', "$tempdir/dump2.sql"
+ ]);
+
+# Compare the two dumps, there should be no differences.
+command_ok([ 'diff', '-q', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ],
+ 'old and new dump match after pg_upgrade');
+
+done_testing();
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
deleted file mode 100644
index ef328b3062..0000000000
--- a/src/bin/pg_upgrade/test.sh
+++ /dev/null
@@ -1,279 +0,0 @@
-#!/bin/sh
-
-# src/bin/pg_upgrade/test.sh
-#
-# Test driver for pg_upgrade. Initializes a new database cluster,
-# runs the regression tests (to put in some data), runs pg_dumpall,
-# runs pg_upgrade, runs pg_dumpall again, compares the dumps.
-#
-# Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
-# Portions Copyright (c) 1994, Regents of the University of California
-
-set -e
-
-: ${MAKE=make}
-
-# Guard against parallel make issues (see comments in pg_regress.c)
-unset MAKEFLAGS
-unset MAKELEVEL
-
-# Run a given "initdb" binary and overlay the regression testing
-# authentication configuration.
-standard_initdb() {
- # To increase coverage of non-standard segment size and group access
- # without increasing test runtime, run these tests with a custom setting.
- # Also, specify "-A trust" explicitly to suppress initdb's warning.
- # --allow-group-access and --wal-segsize have been added in v11.
- "$1" -N --wal-segsize 1 --allow-group-access -A trust
- if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
- then
- cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
- fi
- ../../test/regress/pg_regress --config-auth "$PGDATA"
-}
-
-# What flavor of host are we on?
-# Treat MINGW* (msys1) and MSYS* (msys2) the same.
-testhost=`uname -s | sed 's/^MSYS/MINGW/'`
-
-# Establish how the server will listen for connections
-case $testhost in
- MINGW*)
- LISTEN_ADDRESSES="localhost"
- PG_REGRESS_SOCKET_DIR=""
- PGHOST=localhost
- ;;
- *)
- LISTEN_ADDRESSES=""
- # Select a socket directory. The algorithm is from the "configure"
- # script; the outcome mimics pg_regress.c:make_temp_sockdir().
- if [ x"$PG_REGRESS_SOCKET_DIR" = x ]; then
- set +e
- dir=`(umask 077 &&
- mktemp -d /tmp/pg_upgrade_check-XXXXXX) 2>/dev/null`
- if [ ! -d "$dir" ]; then
- dir=/tmp/pg_upgrade_check-$$-$RANDOM
- (umask 077 && mkdir "$dir")
- if [ ! -d "$dir" ]; then
- echo "could not create socket temporary directory in \"/tmp\""
- exit 1
- fi
- fi
- set -e
- PG_REGRESS_SOCKET_DIR=$dir
- trap 'rm -rf "$PG_REGRESS_SOCKET_DIR"' 0
- trap 'exit 3' 1 2 13 15
- fi
- PGHOST=$PG_REGRESS_SOCKET_DIR
- ;;
-esac
-
-POSTMASTER_OPTS="-F -c listen_addresses=\"$LISTEN_ADDRESSES\" -k \"$PG_REGRESS_SOCKET_DIR\""
-export PGHOST
-
-# don't rely on $PWD here, as old shells don't set it
-temp_root=`pwd`/tmp_check
-rm -rf "$temp_root"
-mkdir "$temp_root"
-
-: ${oldbindir=$bindir}
-
-: ${oldsrc=../../..}
-oldsrc=`cd "$oldsrc" && pwd`
-newsrc=`cd ../../.. && pwd`
-
-# We need to make pg_regress use psql from the desired installation
-# (likely a temporary one), because otherwise the installcheck run
-# below would try to use psql from the proper installation directory
-# of the target version, which might be outdated or not exist. But
-# don't override anything else that's already in EXTRA_REGRESS_OPTS.
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$oldbindir'"
-export EXTRA_REGRESS_OPTS
-
-# While in normal cases this will already be set up, adding bindir to
-# path allows test.sh to be invoked with different versions as
-# described in ./TESTING
-PATH=$bindir:$PATH
-export PATH
-
-BASE_PGDATA="$temp_root/data"
-PGDATA="${BASE_PGDATA}.old"
-export PGDATA
-
-# Send installcheck outputs to a private directory. This avoids conflict when
-# check-world runs pg_upgrade check concurrently with src/test/regress check.
-# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs.
-outputdir="$temp_root/regress"
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
-export EXTRA_REGRESS_OPTS
-mkdir "$outputdir"
-
-# pg_regress --make-tablespacedir would take care of that in 14~, but this is
-# still required for older versions where this option is not supported.
-if [ "$newsrc" != "$oldsrc" ]; then
- mkdir "$outputdir"/testtablespace
- mkdir "$outputdir"/sql
- mkdir "$outputdir"/expected
-fi
-
-logdir=`pwd`/log
-rm -rf "$logdir"
-mkdir "$logdir"
-
-# Clear out any environment vars that might cause libpq to connect to
-# the wrong postmaster (cf pg_regress.c)
-#
-# Some shells, such as NetBSD's, return non-zero from unset if the variable
-# is already unset. Since we are operating under 'set -e', this causes the
-# script to fail. To guard against this, set them all to an empty string first.
-PGDATABASE=""; unset PGDATABASE
-PGUSER=""; unset PGUSER
-PGSERVICE=""; unset PGSERVICE
-PGSSLMODE=""; unset PGSSLMODE
-PGREQUIRESSL=""; unset PGREQUIRESSL
-PGCONNECT_TIMEOUT=""; unset PGCONNECT_TIMEOUT
-PGHOSTADDR=""; unset PGHOSTADDR
-
-# Select a non-conflicting port number, similarly to pg_regress.c
-PG_VERSION_NUM=`grep '#define PG_VERSION_NUM' "$newsrc"/src/include/pg_config.h | awk '{print $3}'`
-PGPORT=`expr $PG_VERSION_NUM % 16384 + 49152`
-export PGPORT
-
-i=0
-while psql -X postgres </dev/null 2>/dev/null
-do
- i=`expr $i + 1`
- if [ $i -eq 16 ]
- then
- echo port $PGPORT apparently in use
- exit 1
- fi
- PGPORT=`expr $PGPORT + 1`
- export PGPORT
-done
-
-# buildfarm may try to override port via EXTRA_REGRESS_OPTS ...
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --port=$PGPORT"
-export EXTRA_REGRESS_OPTS
-
-standard_initdb "$oldbindir"/initdb
-"$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
-
-# Create databases with names covering the ASCII bytes other than NUL, BEL,
-# LF, or CR. BEL would ring the terminal bell in the course of this test, and
-# it is not otherwise a special case. PostgreSQL doesn't support the rest.
-dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
- if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
-# Exercise backslashes adjacent to double quotes, a Windows special case.
-dbname1='\"\'$dbname1'\\"\\\'
-dbname2=`awk 'BEGIN { for (i = 46; i < 91; i++) printf "%c", i }' </dev/null`
-dbname3=`awk 'BEGIN { for (i = 91; i < 128; i++) printf "%c", i }' </dev/null`
-createdb "regression$dbname1" || createdb_status=$?
-createdb "regression$dbname2" || createdb_status=$?
-createdb "regression$dbname3" || createdb_status=$?
-
-# Extra options to apply to the dump. This may be changed later.
-extra_dump_options=""
-
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
- oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
-
- # Before dumping, tweak the database of the old instance depending
- # on its version.
- if [ "$newsrc" != "$oldsrc" ]; then
- # This SQL script has its own idea of the cleanup that needs to be
- # done on the cluster to-be-upgraded, and includes version checks.
- # Note that this uses the script stored on the new branch.
- psql -X -d regression -f "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql" \
- || psql_fix_sql_status=$?
-
- # Handling of --extra-float-digits gets messy after v12.
- # Note that this changes the dumps from the old and new
- # instances if involving an old cluster of v11 or older.
- if [ $oldpgversion -lt 120000 ]; then
- extra_dump_options="--extra-float-digits=0"
- fi
- fi
-
- pg_dumpall $extra_dump_options --no-sync \
- -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
-
- if [ "$newsrc" != "$oldsrc" ]; then
- # update references to old source tree's regress.so etc
- fix_sql=""
- case $oldpgversion in
- *)
- fix_sql="UPDATE pg_proc SET probin = replace(probin, '$oldsrc', '$newsrc') WHERE probin LIKE '$oldsrc%';"
- ;;
- esac
- psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
-
- mv "$temp_root"/dump1.sql "$temp_root"/dump1.sql.orig
- sed "s;$oldsrc;$newsrc;g" "$temp_root"/dump1.sql.orig >"$temp_root"/dump1.sql
- fi
-else
- make_installcheck_status=$?
-fi
-"$oldbindir"/pg_ctl -m fast stop
-if [ -n "$createdb_status" ]; then
- exit 1
-fi
-if [ -n "$make_installcheck_status" ]; then
- exit 1
-fi
-if [ -n "$psql_fix_sql_status" ]; then
- exit 1
-fi
-if [ -n "$pg_dumpall1_status" ]; then
- echo "pg_dumpall of pre-upgrade database cluster failed"
- exit 1
-fi
-
-PGDATA="$BASE_PGDATA"
-
-standard_initdb 'initdb'
-
-pg_upgrade $PG_UPGRADE_OPTS --no-sync -d "${PGDATA}.old" -D "$PGDATA" -b "$oldbindir" -p "$PGPORT" -P "$PGPORT"
-
-# make sure all directories and files have group permissions, on Unix hosts
-# Windows hosts don't support Unix-y permissions.
-case $testhost in
- MINGW*|CYGWIN*) ;;
- *) if [ `find "$PGDATA" -type f ! -perm 640 | wc -l` -ne 0 ]; then
- echo "files in PGDATA with permission != 640";
- exit 1;
- fi ;;
-esac
-
-case $testhost in
- MINGW*|CYGWIN*) ;;
- *) if [ `find "$PGDATA" -type d ! -perm 750 | wc -l` -ne 0 ]; then
- echo "directories in PGDATA with permission != 750";
- exit 1;
- fi ;;
-esac
-
-pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
-
-pg_dumpall $extra_dump_options --no-sync \
- -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
-pg_ctl -m fast stop
-
-if [ -n "$pg_dumpall2_status" ]; then
- echo "pg_dumpall of post-upgrade database cluster failed"
- exit 1
-fi
-
-case $testhost in
- MINGW*) MSYS2_ARG_CONV_EXCL=/c cmd /c delete_old_cluster.bat ;;
- *) sh ./delete_old_cluster.sh ;;
-esac
-
-if diff "$temp_root"/dump1.sql "$temp_root"/dump2.sql >/dev/null; then
- echo PASSED
- exit 0
-else
- echo "Files $temp_root/dump1.sql and $temp_root/dump2.sql differ"
- echo "dumps were not identical"
- exit 1
-fi
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index e2b0db0879..a5f39f02d3 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -286,6 +286,10 @@ sub bincheck
foreach my $dir (@bin_dirs)
{
next unless -d "$dir/t";
+ # Do not consider pg_upgrade, as it is handled by
+ # upgradecheck.
+ next if ($dir =~ "/pg_upgrade/");
+
my $status = tap_check($dir);
$mstat ||= $status;
}
@@ -590,91 +594,9 @@ sub generate_db
sub upgradecheck
{
- my $status;
- my $cwd = getcwd();
-
- # Much of this comes from the pg_upgrade test.sh script,
- # but it only covers the --install case, and not the case
- # where the old and new source or bin dirs are different.
- # i.e. only this version to this version check. That's
- # what pg_upgrade's "make check" does.
-
- $ENV{PGHOST} = 'localhost';
- $ENV{PGPORT} ||= 50432;
- my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check";
- rmtree($tmp_root);
- mkdir $tmp_root || die $!;
- my $upg_tmp_install = "$tmp_root/install"; # unshared temp install
- print "Setting up temp install\n\n";
- Install($upg_tmp_install, "all", $config);
-
- # Install does a chdir, so change back after that
- chdir $cwd;
- my ($bindir, $libdir, $oldsrc, $newsrc) =
- ("$upg_tmp_install/bin", "$upg_tmp_install/lib", $topdir, $topdir);
- $ENV{PATH} = "$bindir;$ENV{PATH}";
- my $data = "$tmp_root/data";
- $ENV{PGDATA} = "$data.old";
- my $outputdir = "$tmp_root/regress";
- my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
- mkdir "$outputdir" || die $!;
-
- my $logdir = "$topdir/src/bin/pg_upgrade/log";
- rmtree($logdir);
- mkdir $logdir || die $!;
- print "\nRunning initdb on old cluster\n\n";
- standard_initdb() or exit 1;
- print "\nStarting old cluster\n\n";
- my @args = ('pg_ctl', 'start', '-l', "$logdir/postmaster1.log");
- system(@args) == 0 or exit 1;
-
- print "\nCreating databases with names covering most ASCII bytes\n\n";
- generate_db("\\\"\\", 1, 45, "\\\\\"\\\\\\");
- generate_db('', 46, 90, '');
- generate_db('', 91, 127, '');
-
- print "\nSetting up data for upgrading\n\n";
- installcheck_internal('parallel', @EXTRA_REGRESS_OPTS);
-
- # now we can chdir into the source dir
- chdir "$topdir/src/bin/pg_upgrade";
- print "\nDumping old cluster\n\n";
- @args = ('pg_dumpall', '-f', "$tmp_root/dump1.sql");
- system(@args) == 0 or exit 1;
- print "\nStopping old cluster\n\n";
- system("pg_ctl stop") == 0 or exit 1;
- $ENV{PGDATA} = "$data";
- print "\nSetting up new cluster\n\n";
- standard_initdb() or exit 1;
- print "\nRunning pg_upgrade\n\n";
- @args = (
- 'pg_upgrade', '-d', "$data.old", '-D', $data, '-b', $bindir,
- '--no-sync');
- system(@args) == 0 or exit 1;
- print "\nStarting new cluster\n\n";
- @args = ('pg_ctl', '-l', "$logdir/postmaster2.log", 'start');
- system(@args) == 0 or exit 1;
- print "\nDumping new cluster\n\n";
- @args = ('pg_dumpall', '-f', "$tmp_root/dump2.sql");
- system(@args) == 0 or exit 1;
- print "\nStopping new cluster\n\n";
- system("pg_ctl stop") == 0 or exit 1;
- print "\nDeleting old cluster\n\n";
- system(".\\delete_old_cluster.bat") == 0 or exit 1;
- print "\nComparing old and new cluster dumps\n\n";
-
- @args = ('diff', '-q', "$tmp_root/dump1.sql", "$tmp_root/dump2.sql");
- system(@args);
- $status = $?;
- if (!$status)
- {
- print "PASSED\n";
- }
- else
- {
- print "dumps not identical!\n";
- exit(1);
- }
+ InstallTemp();
+ my $mstat = tap_check("$topdir/src/bin/pg_upgrade");
+ exit $mstat if $mstat;
return;
}
--
2.35.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-03-02 08:07 ` Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Andres Freund @ 2022-03-02 08:07 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
Hi,
On 2022-03-02 15:57:23 +0900, Michael Paquier wrote:
> Do others have an opinion about a backpatch of the bugfix? Nobody has
> complained about that since pg_upgrade exists, so I have just done the
> change on HEAD.
WFM.
> +++ b/src/bin/pg_upgrade/t/001_basic.pl
> @@ -0,0 +1,9 @@
> +use strict;
> +use warnings;
> +
> +use PostgreSQL::Test::Utils;
> +use Test::More tests => 8;
Outdated.
> +program_help_ok('pg_upgrade');
> +program_version_ok('pg_upgrade');
> +program_options_handling_ok('pg_upgrade');
Unrelated. But I kinda wish we'd do this in a saner manner than copying this
test into every binary. E.g. by ensuring that all tools installed in the temp
install are tested or such.
> +# The test of pg_upgrade consists in setting up an instance. This is the
> +# source instance used for the upgrade. Then a new and fresh instance is
> +# created, and is used as the target instance for the upgrade.
This seems a bit repetitive. Lots of "instance".
> Before
> +# running an upgrade, a logical dump of the old instance is taken, and a
> +# second logical dump of the new instance is taken after the upgrade.
> +# The upgrade test passes if there are no differences in these two dumps.
> +
> +# Testing upgrades with an older instance of PostgreSQL requires setting up
> +# two environment variables, as of:
> +# - "olddump", to point to a dump file that will be used to set
> +# up the old instance to upgrade from, the dump being restored in the
> +# old cluster.
> +# - "oldinstall", to point to the installation path of the old
> +# instance.
> +if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
> + || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
Odd indentation. Spaces between parens?
> +$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
I'd copy the comments from test.sh wrt --wal-segsize,
--allow-group-access.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
@ 2022-03-03 05:03 ` Michael Paquier <[email protected]>
2022-03-03 13:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andrew Dunstan <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 2 replies; 31+ messages in thread
From: Michael Paquier @ 2022-03-03 05:03 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Wed, Mar 02, 2022 at 12:07:29AM -0800, Andres Freund wrote:
>> +++ b/src/bin/pg_upgrade/t/001_basic.pl
>> @@ -0,0 +1,9 @@
>> +use strict;
>> +use warnings;
>> +
>> +use PostgreSQL::Test::Utils;
>> +use Test::More tests => 8;
>
> Outdated.
Fixed.
>> +program_help_ok('pg_upgrade');
>> +program_version_ok('pg_upgrade');
>> +program_options_handling_ok('pg_upgrade');
>
> Unrelated. But I kinda wish we'd do this in a saner manner than copying this
> test into every binary. E.g. by ensuring that all tools installed in the temp
> install are tested or such.
Perhaps. I am sticking with the existing style for now.
>> +# The test of pg_upgrade consists in setting up an instance. This is the
>> +# source instance used for the upgrade. Then a new and fresh instance is
>> +# created, and is used as the target instance for the upgrade.
>
> This seems a bit repetitive. Lots of "instance".
Indeed. I have reworked the whole, rather than just those three
sentences.
>> +if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
>> + || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
>
> Odd indentation. Spaces between parens?
Well, perltidy tells me that this is right.
>> +$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
>
> I'd copy the comments from test.sh wrt --wal-segsize,
> --allow-group-access.
Done.
--
Michael
Attachments:
[text/x-diff] v11-0001-Switch-tests-of-pg_upgrade-to-use-TAP.patch (27.5K, ../../[email protected]/2-v11-0001-Switch-tests-of-pg_upgrade-to-use-TAP.patch)
download | inline diff:
From ba2cf7a7adf9cf50406d8bbcaa12167137ba29c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 3 Mar 2022 14:01:56 +0900
Subject: [PATCH v11] Switch tests of pg_upgrade to use TAP
---
src/bin/pg_upgrade/Makefile | 21 +-
src/bin/pg_upgrade/TESTING | 85 ++------
src/bin/pg_upgrade/t/001_basic.pl | 11 +
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 248 ++++++++++++++++++++++
src/bin/pg_upgrade/test.sh | 279 -------------------------
src/tools/msvc/vcregress.pl | 92 +-------
6 files changed, 289 insertions(+), 447 deletions(-)
create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
delete mode 100644 src/bin/pg_upgrade/test.sh
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 49b94f0ac7..1f5d757548 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -28,6 +28,10 @@ OBJS = \
override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
+# required for 002_pg_upgrade.pl
+REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
+export REGRESS_SHLIB
+
all: pg_upgrade
pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
@@ -47,17 +51,8 @@ clean distclean maintainer-clean:
rm -rf delete_old_cluster.sh log/ tmp_check/ \
reindex_hash.sql
-# When $(MAKE) is present, make automatically infers that this is a
-# recursive make. which is not actually what we want here, as that
-# e.g. prevents output synchronization from working (as make thinks
-# that the subsidiary make knows how to deal with that itself, but
-# we're invoking a shell script that doesn't know). Referencing
-# $(MAKE) indirectly avoids that behaviour.
-# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
-NOTSUBMAKEMAKE=$(MAKE)
+check:
+ $(prove_check)
-check: test.sh all temp-install
- MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
-
-# installcheck is not supported because there's no meaningful way to test
-# pg_upgrade against a single already-running server
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index 78b9747908..3718483a1c 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -2,78 +2,23 @@ THE SHORT VERSION
-----------------
On non-Windows machines, you can execute the testing process
-described below by running
+described below by running the following command in this directory:
make check
-in this directory. This will run the shell script test.sh, performing
-an upgrade from the version in this source tree to a new instance of
-the same version.
-To test an upgrade from a different version, you must have a built
-source tree for the old version as well as this version, and you
-must have done "make install" for both versions. Then do:
+This will run the TAP tests to run pg_upgrade, performing an upgrade
+from the version in this source tree to a new instance of the same
+version.
-export oldsrc=...somewhere/postgresql (old version's source tree)
-export oldbindir=...otherversion/bin (old version's installed bin dir)
-export bindir=...thisversion/bin (this version's installed bin dir)
-export libdir=...thisversion/lib (this version's installed lib dir)
-sh test.sh
+Testing an upgrade from a different version requires a dump to set up
+the contents of this instance, with its set of binaries. The following
+variables are available to control the test:
+export olddump=...somewhere/dump.sql (old version's dump)
+export oldinstall=...otherversion/ (old version's install base path)
-In this case, you will have to manually eyeball the resulting dump
-diff for version-specific differences, as explained below.
+Finally, the tests can be done by running
+ make check
-
-DETAILS
--------
-
-The most effective way to test pg_upgrade, aside from testing on user
-data, is by upgrading the PostgreSQL regression database.
-
-This testing process first requires the creation of a valid regression
-database dump. Such files contain most database features and are
-specific to each major version of Postgres.
-
-Here are the steps needed to create a regression database dump file:
-
-1) Create and populate the regression database in the old cluster.
- This database can be created by running 'make installcheck' from
- src/test/regress.
-
-2) Use pg_dump to dump out the regression database. Use the new
- cluster's pg_dump on the old database to minimize whitespace
- differences in the diff.
-
-3) Adjust the regression database dump file
-
- a) Perform the load/dump twice
- This fixes problems with the ordering of COPY columns for
- inherited tables.
-
- b) Change CREATE FUNCTION shared object paths to use '$libdir'
- The old and new cluster will have different shared object paths.
-
- c) Fix any wrapping format differences
- Commands like CREATE TRIGGER and ALTER TABLE sometimes have
- differences.
-
-Once the dump is created, it can be repeatedly loaded into the old
-database, upgraded, and dumped out of the new database, and then
-compared to the original version. To test the dump file, perform these
-steps:
-
-1) Create the old and new clusters in different directories.
-
-2) Copy the regression shared object files into the appropriate /lib
- directory for old and new clusters.
-
-3) Create the regression database in the old server.
-
-4) Load the dump file created above into the regression database;
- check for errors while loading.
-
-5) Upgrade the old database to the new major version, as outlined in
- the pg_upgrade manual section.
-
-6) Use pg_dump to dump out the regression database in the new cluster.
-
-7) Diff the regression database dump file with the regression dump
- file loaded into the old server.
+Using the test in this configuration is possible with any dump file.
+However, it is recommended to create a dump from the database "regression",
+created by running 'make installcheck' from src/test/regress as this covers
+most database features and are specific to each major version of Postgres.
diff --git a/src/bin/pg_upgrade/t/001_basic.pl b/src/bin/pg_upgrade/t/001_basic.pl
new file mode 100644
index 0000000000..40458f10b6
--- /dev/null
+++ b/src/bin/pg_upgrade/t/001_basic.pl
@@ -0,0 +1,11 @@
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+program_help_ok('pg_upgrade');
+program_version_ok('pg_upgrade');
+program_options_handling_ok('pg_upgrade');
+
+done_testing();
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
new file mode 100644
index 0000000000..7a256844da
--- /dev/null
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -0,0 +1,248 @@
+# Set of tests for pg_upgrade, including cross-version checks.
+use strict;
+use warnings;
+
+use Cwd qw(abs_path getcwd);
+use File::Basename qw(dirname);
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Generate a database with a name made of a range of ASCII characters.
+sub generate_db
+{
+ my ($node, $from_char, $to_char) = @_;
+
+ my $dbname = '';
+ for my $i ($from_char .. $to_char)
+ {
+ next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
+ $dbname = $dbname . sprintf('%c', $i);
+ }
+ $node->run_log(
+ [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
+ );
+}
+
+# The test of pg_upgrade consists requires two clusters, an old one and
+# and a new one, that gets upgraded. Before running the upgrade, a logical
+# dump of the old cluster is taken, and a second logical dump of the new one
+# is taken after the upgrade. The upgrade test passes if there are no
+# differences in these two dumps.
+
+# Testing upgrades with an older version of PostgreSQL requires setting up
+# two environment variables, as of:
+# - "olddump", to point to a dump file that will be used to set up the old
+# instance to upgrade from, the dump being restored in the old cluster.
+# - "oldinstall", to point to the installation path of the old cluster.
+if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
+ || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
+{
+ # Not all variables are defined, so leave and die if test is
+ # done with an older installation.
+ die "olddump or oldinstall is undefined";
+}
+
+# Temporary location for the dumps taken
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
+# Initialize node to upgrade
+my $oldnode = PostgreSQL::Test::Cluster->new('old_node',
+ install_path => $ENV{oldinstall});
+
+# To increase coverage of non-standard segment size and group access without
+# increasing test runtime, run these tests with a custom setting.
+# --allow-group-access and --wal-segsize have been added in v11.
+$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+$oldnode->start;
+
+# The default location of the source code is the root of this directory.
+my $srcdir = abs_path("../../..");
+
+# Set up the data of the old instance with a dump or pg_regress.
+if (defined($ENV{olddump}))
+{
+ # Use the dump specified.
+ my $olddumpfile = $ENV{olddump};
+ die "no dump file found!" unless -e $olddumpfile;
+
+ # Load the dump, and we are done here.
+ $oldnode->command_ok(
+ [
+ 'psql', '-X', '-f', $olddumpfile,
+ '--port', $oldnode->port, '--host', $oldnode->host,
+ 'regression'
+ ]);
+}
+else
+{
+ # Default is to just use pg_regress to set up the old instance
+ # Creating databases with names covering most ASCII bytes
+ generate_db($oldnode, 1, 45);
+ generate_db($oldnode, 46, 90);
+ generate_db($oldnode, 91, 127);
+
+ # Grab any regression options that may be passed down by caller.
+ my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
+ my @extra_opts = split(/\s+/, $extra_opts_val);
+
+ # --dlpath is needed to be able to find the location of regress.so and
+ # any libraries the regression tests required. This needs to point to
+ # the old cluster when using it. In the default case, fallback to
+ # what the caller provided for REGRESS_SHLIB.
+ my $dlpath = dirname($ENV{REGRESS_SHLIB});
+
+ # --outputdir points to the path where to place the output files.
+ my $outputdir = $PostgreSQL::Test::Utils::tmp_check;
+
+ # --inputdir points to the path of the input files.
+ my $inputdir = "$srcdir/src/test/regress";
+
+ my @regress_command = [
+ $ENV{PG_REGRESS},
+ @extra_opts,
+ '--dlpath',
+ $dlpath,
+ '--max-concurrent-tests',
+ '20',
+ '--bindir',
+ $oldnode->config_data('--bindir'),
+ '--host',
+ $oldnode->host,
+ '--port',
+ $oldnode->port,
+ '--schedule',
+ "$srcdir/src/test/regress/parallel_schedule",
+ '--outputdir',
+ $outputdir,
+ '--inputdir',
+ $inputdir
+ ];
+
+ $oldnode->command_ok(@regress_command,
+ 'regression test run on old instance');
+}
+
+# Before dumping, get rid of objects not existing or not supported in later
+# versions. This depends on the version of the old server used, and matters
+# only if different versions are used for the dump.
+if (defined($ENV{oldinstall}))
+{
+ # Note that upgrade_adapt.sql from the new version is used, to
+ # cope with an upgrade to this version.
+ $oldnode->run_log(
+ [
+ 'psql', '-X',
+ '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
+ '--port', $oldnode->port,
+ '--host', $oldnode->host,
+ 'regression'
+ ]);
+}
+
+# Initialize a new node for the upgrade. This is done early so as it is
+# possible to know with which node's PATH the initial dump needs to be
+# taken.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+$newnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my $newbindir = $newnode->config_data('--bindir');
+my $oldbindir = $oldnode->config_data('--bindir');
+
+# Take a dump before performing the upgrade as a base comparison. Note
+# that we need to use pg_dumpall from the new node here.
+$newnode->command_ok(
+ [
+ 'pg_dumpall', '--no-sync',
+ '-d', $oldnode->connstr('postgres'),
+ '-f', "$tempdir/dump1.sql"
+ ],
+ 'dump before running pg_upgrade');
+
+# After dumping, update references to the old source tree's regress.so
+# to point to the new tree.
+if (defined($ENV{oldinstall}))
+{
+ # First, fetch all the references to libraries that are not part
+ # of the default path $libdir.
+ my $output = $oldnode->safe_psql('regression',
+ "SELECT DISTINCT probin::text FROM pg_proc WHERE probin NOT LIKE '\$libdir%';"
+ );
+ chomp($output);
+ my @libpaths = split("\n", $output);
+
+ my $dump_data = slurp_file("$tempdir/dump1.sql");
+
+ my $newregresssrc = "$srcdir/src/test/regress";
+ foreach (@libpaths)
+ {
+ my $libpath = $_;
+ $libpath = dirname($libpath);
+ $dump_data =~ s/$libpath/$newregresssrc/g;
+ }
+
+ open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump file";
+ print $fh $dump_data;
+ close $fh;
+
+ # This replaces any references to the old tree's regress.so
+ # the new tree's regress.so. Any references that do *not*
+ # match $libdir are switched so as this request does not
+ # depend on the path of the old source tree. This is useful
+ # when using an old dump. Do the operation on all the databases
+ # that allow connections so as this includes the regression
+ # database and anything the user has set up.
+ $output = $oldnode->safe_psql('postgres',
+ "SELECT datname FROM pg_database WHERE datallowconn;");
+ chomp($output);
+ my @datnames = split("\n", $output);
+ foreach (@datnames)
+ {
+ my $datname = $_;
+ $oldnode->safe_psql(
+ $datname, "UPDATE pg_proc SET probin =
+ regexp_replace(probin, '.*/', '$newregresssrc/')
+ WHERE probin NOT LIKE '\$libdir/%'");
+ }
+}
+
+# Upgrade the instance.
+$oldnode->stop;
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir, '-b', $oldbindir,
+ '-B', $newbindir, '-p', $oldnode->port,
+ '-P', $newnode->port
+ ],
+ 'run of pg_upgrade for new instance');
+$newnode->start;
+
+# Check if there are any logs coming from pg_upgrade, that would only be
+# retained on failure.
+my $log_path = $newnode->data_dir . "/pg_upgrade_output.d/log";
+if (-d $log_path)
+{
+ foreach my $log (glob("$log_path/*"))
+ {
+ note "###########################";
+ note "Contents of log file $log";
+ note "###########################";
+ my $log_contents = slurp_file($log);
+ print "$log_contents\n";
+ }
+}
+
+# Second dump from the upgraded instance.
+$newnode->run_log(
+ [
+ 'pg_dumpall', '--no-sync',
+ '-d', $newnode->connstr('postgres'),
+ '-f', "$tempdir/dump2.sql"
+ ]);
+
+# Compare the two dumps, there should be no differences.
+command_ok([ 'diff', '-q', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ],
+ 'old and new dump match after pg_upgrade');
+
+done_testing();
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
deleted file mode 100644
index ef328b3062..0000000000
--- a/src/bin/pg_upgrade/test.sh
+++ /dev/null
@@ -1,279 +0,0 @@
-#!/bin/sh
-
-# src/bin/pg_upgrade/test.sh
-#
-# Test driver for pg_upgrade. Initializes a new database cluster,
-# runs the regression tests (to put in some data), runs pg_dumpall,
-# runs pg_upgrade, runs pg_dumpall again, compares the dumps.
-#
-# Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
-# Portions Copyright (c) 1994, Regents of the University of California
-
-set -e
-
-: ${MAKE=make}
-
-# Guard against parallel make issues (see comments in pg_regress.c)
-unset MAKEFLAGS
-unset MAKELEVEL
-
-# Run a given "initdb" binary and overlay the regression testing
-# authentication configuration.
-standard_initdb() {
- # To increase coverage of non-standard segment size and group access
- # without increasing test runtime, run these tests with a custom setting.
- # Also, specify "-A trust" explicitly to suppress initdb's warning.
- # --allow-group-access and --wal-segsize have been added in v11.
- "$1" -N --wal-segsize 1 --allow-group-access -A trust
- if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
- then
- cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
- fi
- ../../test/regress/pg_regress --config-auth "$PGDATA"
-}
-
-# What flavor of host are we on?
-# Treat MINGW* (msys1) and MSYS* (msys2) the same.
-testhost=`uname -s | sed 's/^MSYS/MINGW/'`
-
-# Establish how the server will listen for connections
-case $testhost in
- MINGW*)
- LISTEN_ADDRESSES="localhost"
- PG_REGRESS_SOCKET_DIR=""
- PGHOST=localhost
- ;;
- *)
- LISTEN_ADDRESSES=""
- # Select a socket directory. The algorithm is from the "configure"
- # script; the outcome mimics pg_regress.c:make_temp_sockdir().
- if [ x"$PG_REGRESS_SOCKET_DIR" = x ]; then
- set +e
- dir=`(umask 077 &&
- mktemp -d /tmp/pg_upgrade_check-XXXXXX) 2>/dev/null`
- if [ ! -d "$dir" ]; then
- dir=/tmp/pg_upgrade_check-$$-$RANDOM
- (umask 077 && mkdir "$dir")
- if [ ! -d "$dir" ]; then
- echo "could not create socket temporary directory in \"/tmp\""
- exit 1
- fi
- fi
- set -e
- PG_REGRESS_SOCKET_DIR=$dir
- trap 'rm -rf "$PG_REGRESS_SOCKET_DIR"' 0
- trap 'exit 3' 1 2 13 15
- fi
- PGHOST=$PG_REGRESS_SOCKET_DIR
- ;;
-esac
-
-POSTMASTER_OPTS="-F -c listen_addresses=\"$LISTEN_ADDRESSES\" -k \"$PG_REGRESS_SOCKET_DIR\""
-export PGHOST
-
-# don't rely on $PWD here, as old shells don't set it
-temp_root=`pwd`/tmp_check
-rm -rf "$temp_root"
-mkdir "$temp_root"
-
-: ${oldbindir=$bindir}
-
-: ${oldsrc=../../..}
-oldsrc=`cd "$oldsrc" && pwd`
-newsrc=`cd ../../.. && pwd`
-
-# We need to make pg_regress use psql from the desired installation
-# (likely a temporary one), because otherwise the installcheck run
-# below would try to use psql from the proper installation directory
-# of the target version, which might be outdated or not exist. But
-# don't override anything else that's already in EXTRA_REGRESS_OPTS.
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$oldbindir'"
-export EXTRA_REGRESS_OPTS
-
-# While in normal cases this will already be set up, adding bindir to
-# path allows test.sh to be invoked with different versions as
-# described in ./TESTING
-PATH=$bindir:$PATH
-export PATH
-
-BASE_PGDATA="$temp_root/data"
-PGDATA="${BASE_PGDATA}.old"
-export PGDATA
-
-# Send installcheck outputs to a private directory. This avoids conflict when
-# check-world runs pg_upgrade check concurrently with src/test/regress check.
-# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs.
-outputdir="$temp_root/regress"
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
-export EXTRA_REGRESS_OPTS
-mkdir "$outputdir"
-
-# pg_regress --make-tablespacedir would take care of that in 14~, but this is
-# still required for older versions where this option is not supported.
-if [ "$newsrc" != "$oldsrc" ]; then
- mkdir "$outputdir"/testtablespace
- mkdir "$outputdir"/sql
- mkdir "$outputdir"/expected
-fi
-
-logdir=`pwd`/log
-rm -rf "$logdir"
-mkdir "$logdir"
-
-# Clear out any environment vars that might cause libpq to connect to
-# the wrong postmaster (cf pg_regress.c)
-#
-# Some shells, such as NetBSD's, return non-zero from unset if the variable
-# is already unset. Since we are operating under 'set -e', this causes the
-# script to fail. To guard against this, set them all to an empty string first.
-PGDATABASE=""; unset PGDATABASE
-PGUSER=""; unset PGUSER
-PGSERVICE=""; unset PGSERVICE
-PGSSLMODE=""; unset PGSSLMODE
-PGREQUIRESSL=""; unset PGREQUIRESSL
-PGCONNECT_TIMEOUT=""; unset PGCONNECT_TIMEOUT
-PGHOSTADDR=""; unset PGHOSTADDR
-
-# Select a non-conflicting port number, similarly to pg_regress.c
-PG_VERSION_NUM=`grep '#define PG_VERSION_NUM' "$newsrc"/src/include/pg_config.h | awk '{print $3}'`
-PGPORT=`expr $PG_VERSION_NUM % 16384 + 49152`
-export PGPORT
-
-i=0
-while psql -X postgres </dev/null 2>/dev/null
-do
- i=`expr $i + 1`
- if [ $i -eq 16 ]
- then
- echo port $PGPORT apparently in use
- exit 1
- fi
- PGPORT=`expr $PGPORT + 1`
- export PGPORT
-done
-
-# buildfarm may try to override port via EXTRA_REGRESS_OPTS ...
-EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --port=$PGPORT"
-export EXTRA_REGRESS_OPTS
-
-standard_initdb "$oldbindir"/initdb
-"$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
-
-# Create databases with names covering the ASCII bytes other than NUL, BEL,
-# LF, or CR. BEL would ring the terminal bell in the course of this test, and
-# it is not otherwise a special case. PostgreSQL doesn't support the rest.
-dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
- if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
-# Exercise backslashes adjacent to double quotes, a Windows special case.
-dbname1='\"\'$dbname1'\\"\\\'
-dbname2=`awk 'BEGIN { for (i = 46; i < 91; i++) printf "%c", i }' </dev/null`
-dbname3=`awk 'BEGIN { for (i = 91; i < 128; i++) printf "%c", i }' </dev/null`
-createdb "regression$dbname1" || createdb_status=$?
-createdb "regression$dbname2" || createdb_status=$?
-createdb "regression$dbname3" || createdb_status=$?
-
-# Extra options to apply to the dump. This may be changed later.
-extra_dump_options=""
-
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
- oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
-
- # Before dumping, tweak the database of the old instance depending
- # on its version.
- if [ "$newsrc" != "$oldsrc" ]; then
- # This SQL script has its own idea of the cleanup that needs to be
- # done on the cluster to-be-upgraded, and includes version checks.
- # Note that this uses the script stored on the new branch.
- psql -X -d regression -f "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql" \
- || psql_fix_sql_status=$?
-
- # Handling of --extra-float-digits gets messy after v12.
- # Note that this changes the dumps from the old and new
- # instances if involving an old cluster of v11 or older.
- if [ $oldpgversion -lt 120000 ]; then
- extra_dump_options="--extra-float-digits=0"
- fi
- fi
-
- pg_dumpall $extra_dump_options --no-sync \
- -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
-
- if [ "$newsrc" != "$oldsrc" ]; then
- # update references to old source tree's regress.so etc
- fix_sql=""
- case $oldpgversion in
- *)
- fix_sql="UPDATE pg_proc SET probin = replace(probin, '$oldsrc', '$newsrc') WHERE probin LIKE '$oldsrc%';"
- ;;
- esac
- psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
-
- mv "$temp_root"/dump1.sql "$temp_root"/dump1.sql.orig
- sed "s;$oldsrc;$newsrc;g" "$temp_root"/dump1.sql.orig >"$temp_root"/dump1.sql
- fi
-else
- make_installcheck_status=$?
-fi
-"$oldbindir"/pg_ctl -m fast stop
-if [ -n "$createdb_status" ]; then
- exit 1
-fi
-if [ -n "$make_installcheck_status" ]; then
- exit 1
-fi
-if [ -n "$psql_fix_sql_status" ]; then
- exit 1
-fi
-if [ -n "$pg_dumpall1_status" ]; then
- echo "pg_dumpall of pre-upgrade database cluster failed"
- exit 1
-fi
-
-PGDATA="$BASE_PGDATA"
-
-standard_initdb 'initdb'
-
-pg_upgrade $PG_UPGRADE_OPTS --no-sync -d "${PGDATA}.old" -D "$PGDATA" -b "$oldbindir" -p "$PGPORT" -P "$PGPORT"
-
-# make sure all directories and files have group permissions, on Unix hosts
-# Windows hosts don't support Unix-y permissions.
-case $testhost in
- MINGW*|CYGWIN*) ;;
- *) if [ `find "$PGDATA" -type f ! -perm 640 | wc -l` -ne 0 ]; then
- echo "files in PGDATA with permission != 640";
- exit 1;
- fi ;;
-esac
-
-case $testhost in
- MINGW*|CYGWIN*) ;;
- *) if [ `find "$PGDATA" -type d ! -perm 750 | wc -l` -ne 0 ]; then
- echo "directories in PGDATA with permission != 750";
- exit 1;
- fi ;;
-esac
-
-pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w
-
-pg_dumpall $extra_dump_options --no-sync \
- -f "$temp_root"/dump2.sql || pg_dumpall2_status=$?
-pg_ctl -m fast stop
-
-if [ -n "$pg_dumpall2_status" ]; then
- echo "pg_dumpall of post-upgrade database cluster failed"
- exit 1
-fi
-
-case $testhost in
- MINGW*) MSYS2_ARG_CONV_EXCL=/c cmd /c delete_old_cluster.bat ;;
- *) sh ./delete_old_cluster.sh ;;
-esac
-
-if diff "$temp_root"/dump1.sql "$temp_root"/dump2.sql >/dev/null; then
- echo PASSED
- exit 0
-else
- echo "Files $temp_root/dump1.sql and $temp_root/dump2.sql differ"
- echo "dumps were not identical"
- exit 1
-fi
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index e2b0db0879..a5f39f02d3 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -286,6 +286,10 @@ sub bincheck
foreach my $dir (@bin_dirs)
{
next unless -d "$dir/t";
+ # Do not consider pg_upgrade, as it is handled by
+ # upgradecheck.
+ next if ($dir =~ "/pg_upgrade/");
+
my $status = tap_check($dir);
$mstat ||= $status;
}
@@ -590,91 +594,9 @@ sub generate_db
sub upgradecheck
{
- my $status;
- my $cwd = getcwd();
-
- # Much of this comes from the pg_upgrade test.sh script,
- # but it only covers the --install case, and not the case
- # where the old and new source or bin dirs are different.
- # i.e. only this version to this version check. That's
- # what pg_upgrade's "make check" does.
-
- $ENV{PGHOST} = 'localhost';
- $ENV{PGPORT} ||= 50432;
- my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check";
- rmtree($tmp_root);
- mkdir $tmp_root || die $!;
- my $upg_tmp_install = "$tmp_root/install"; # unshared temp install
- print "Setting up temp install\n\n";
- Install($upg_tmp_install, "all", $config);
-
- # Install does a chdir, so change back after that
- chdir $cwd;
- my ($bindir, $libdir, $oldsrc, $newsrc) =
- ("$upg_tmp_install/bin", "$upg_tmp_install/lib", $topdir, $topdir);
- $ENV{PATH} = "$bindir;$ENV{PATH}";
- my $data = "$tmp_root/data";
- $ENV{PGDATA} = "$data.old";
- my $outputdir = "$tmp_root/regress";
- my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
- mkdir "$outputdir" || die $!;
-
- my $logdir = "$topdir/src/bin/pg_upgrade/log";
- rmtree($logdir);
- mkdir $logdir || die $!;
- print "\nRunning initdb on old cluster\n\n";
- standard_initdb() or exit 1;
- print "\nStarting old cluster\n\n";
- my @args = ('pg_ctl', 'start', '-l', "$logdir/postmaster1.log");
- system(@args) == 0 or exit 1;
-
- print "\nCreating databases with names covering most ASCII bytes\n\n";
- generate_db("\\\"\\", 1, 45, "\\\\\"\\\\\\");
- generate_db('', 46, 90, '');
- generate_db('', 91, 127, '');
-
- print "\nSetting up data for upgrading\n\n";
- installcheck_internal('parallel', @EXTRA_REGRESS_OPTS);
-
- # now we can chdir into the source dir
- chdir "$topdir/src/bin/pg_upgrade";
- print "\nDumping old cluster\n\n";
- @args = ('pg_dumpall', '-f', "$tmp_root/dump1.sql");
- system(@args) == 0 or exit 1;
- print "\nStopping old cluster\n\n";
- system("pg_ctl stop") == 0 or exit 1;
- $ENV{PGDATA} = "$data";
- print "\nSetting up new cluster\n\n";
- standard_initdb() or exit 1;
- print "\nRunning pg_upgrade\n\n";
- @args = (
- 'pg_upgrade', '-d', "$data.old", '-D', $data, '-b', $bindir,
- '--no-sync');
- system(@args) == 0 or exit 1;
- print "\nStarting new cluster\n\n";
- @args = ('pg_ctl', '-l', "$logdir/postmaster2.log", 'start');
- system(@args) == 0 or exit 1;
- print "\nDumping new cluster\n\n";
- @args = ('pg_dumpall', '-f', "$tmp_root/dump2.sql");
- system(@args) == 0 or exit 1;
- print "\nStopping new cluster\n\n";
- system("pg_ctl stop") == 0 or exit 1;
- print "\nDeleting old cluster\n\n";
- system(".\\delete_old_cluster.bat") == 0 or exit 1;
- print "\nComparing old and new cluster dumps\n\n";
-
- @args = ('diff', '-q', "$tmp_root/dump1.sql", "$tmp_root/dump2.sql");
- system(@args);
- $status = $?;
- if (!$status)
- {
- print "PASSED\n";
- }
- else
- {
- print "dumps not identical!\n";
- exit(1);
- }
+ InstallTemp();
+ my $mstat = tap_check("$topdir/src/bin/pg_upgrade");
+ exit $mstat if $mstat;
return;
}
--
2.35.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-03-03 13:16 ` Andrew Dunstan <[email protected]>
1 sibling, 0 replies; 31+ messages in thread
From: Andrew Dunstan @ 2022-03-03 13:16 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Postgres hackers <[email protected]>
On 3/3/22 00:03, Michael Paquier wrote:
>>> +if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
>>> + || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
>> Odd indentation. Spaces between parens?
> Well, perltidy tells me that this is right.
>
>
Yeah, I haven't found a way to make it stop doing that :-(
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-03-31 04:10 ` Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-03-31 04:10 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Thu, Mar 03, 2022 at 02:03:38PM +0900, Michael Paquier wrote:
> Indeed. I have reworked the whole, rather than just those three
> sentences.
So, any particular feelings about this patch? This has been around
for a couple of months/years now, so it could be a good time to do the
switch now rather than wait an extra year, or even the beginning of
the next release cycle. And the buildfarm is already able to handle
that in its code based on the last release, by skipping the upgrade
check if it finds a pg_upgrade/t/ subdirectory.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-03-31 05:00 ` Tom Lane <[email protected]>
2022-03-31 05:25 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
0 siblings, 2 replies; 31+ messages in thread
From: Tom Lane @ 2022-03-31 05:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
Michael Paquier <[email protected]> writes:
> So, any particular feelings about this patch? This has been around
> for a couple of months/years now, so it could be a good time to do the
> switch now rather than wait an extra year, or even the beginning of
> the next release cycle. And the buildfarm is already able to handle
> that in its code based on the last release, by skipping the upgrade
> check if it finds a pg_upgrade/t/ subdirectory.
There's still about a third of the buildfarm running older
client releases --- I count
2 REL_8
2 REL_10
13 REL_11
6 REL_12
16 REL_13.1
89 REL_14
How well does this patch work with pre-14 buildfarm clients?
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
@ 2022-03-31 05:25 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 31+ messages in thread
From: Tom Lane @ 2022-03-31 05:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
I wrote:
> There's still about a third of the buildfarm running older
> client releases --- I count
> 2 REL_8
> 2 REL_10
> 13 REL_11
> 6 REL_12
> 16 REL_13.1
> 89 REL_14
Wait a minute ... actually, what's most relevant here is
the population running TAP tests, which seems to be
2 REL_8
4 REL_11
1 REL_12
7 REL_13.1
53 REL_14
So there are still some people we'd have to nag if it doesn't
work pre-v14, but fewer than I thought --- specifically,
the owners of
butterflyfish
copperhead
eelpout
elver
halibut
kittiwake
mantid
marabou
massasauga
myna
snakefly
snapper
spurfowl
tadarida
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
@ 2022-03-31 05:36 ` Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Andres Freund @ 2022-03-31 05:36 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On 2022-03-31 01:00:14 -0400, Tom Lane wrote:
> How well does this patch work with pre-14 buildfarm clients?
Looks to me like it'll just run the test twice, once via TestUpgrade, once via
taptest. It's possible that there could be trouble somehow due to duplicated
log files or something?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
@ 2022-03-31 07:56 ` Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-03-31 07:56 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Wed, Mar 30, 2022 at 10:36:16PM -0700, Andres Freund wrote:
> On 2022-03-31 01:00:14 -0400, Tom Lane wrote:
> > How well does this patch work with pre-14 buildfarm clients?
>
> Looks to me like it'll just run the test twice, once via TestUpgrade, once via
> taptest. It's possible that there could be trouble somehow due to duplicated
> log files or something?
Hmm. TestUpgrade.pm also uses tmp_check/, and the TAP tests would
remove this path before running. Still, all the contents of the logs
would be printed out before moving to the next tests at the end of
check-pg_upgrade. It does not seem like this double run is going to
be an issue on this side.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-03-31 13:49 ` Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Tom Lane @ 2022-03-31 13:49 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
Michael Paquier <[email protected]> writes:
> On Wed, Mar 30, 2022 at 10:36:16PM -0700, Andres Freund wrote:
>> On 2022-03-31 01:00:14 -0400, Tom Lane wrote:
>>> How well does this patch work with pre-14 buildfarm clients?
>> Looks to me like it'll just run the test twice, once via TestUpgrade, once via
>> taptest. It's possible that there could be trouble somehow due to duplicated
>> log files or something?
> Hmm. TestUpgrade.pm also uses tmp_check/, and the TAP tests would
> remove this path before running. Still, all the contents of the logs
> would be printed out before moving to the next tests at the end of
> check-pg_upgrade. It does not seem like this double run is going to
> be an issue on this side.
Well, let's go ahead with it and see what happens. If it's too
much of a mess we can always revert.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
@ 2022-04-01 01:16 ` Michael Paquier <[email protected]>
2022-04-01 03:42 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-04-01 03:54 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
0 siblings, 3 replies; 31+ messages in thread
From: Michael Paquier @ 2022-04-01 01:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Thu, Mar 31, 2022 at 09:49:50AM -0400, Tom Lane wrote:
> Well, let's go ahead with it and see what happens. If it's too
> much of a mess we can always revert.
Okay, done after an extra round of self-review. I have finished by
tweaking a couple of comments, and adjusted further TESTING to explain
what needs to be done to have a dump compatible with the test. Let's
now see what goes wrong.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-04-01 03:42 ` Noah Misch <[email protected]>
2022-04-01 03:51 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Justin Pryzby <[email protected]>
2022-04-01 04:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2 siblings, 2 replies; 31+ messages in thread
From: Noah Misch @ 2022-04-01 03:42 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 09:49:50AM -0400, Tom Lane wrote:
> > Well, let's go ahead with it and see what happens. If it's too
> > much of a mess we can always revert.
>
> Okay, done after an extra round of self-review. I have finished by
> tweaking a couple of comments, and adjusted further TESTING to explain
> what needs to be done to have a dump compatible with the test. Let's
> now see what goes wrong.
The REL_14 buildfarm client did not grab logs from the first failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse&dt=2022-04-01%2001%3A39%3A04
The failure looked like this:
# Running: diff -q /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump1.sql /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump2.sql
/usr/bin/diff: illegal option -- q
usage: diff [-bitw] [-c | -e | -f | -h | -n | -u] file1 file2
diff [-bitw] [-C number | -U number] file1 file2
diff [-bitw] [-D string] file1 file2
diff [-bitw] [-c | -e | -f | -h | -n | -u] [-l] [-r] [-s] [-S name] directory1 directory2
not ok 4 - old and new dump match after pg_upgrade
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-04-01 03:42 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-04-01 03:51 ` Justin Pryzby <[email protected]>
2022-04-01 06:01 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Justin Pryzby @ 2022-04-01 03:51 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; [email protected]
On Thu, Mar 31, 2022 at 08:42:41PM -0700, Noah Misch wrote:
> On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> > On Thu, Mar 31, 2022 at 09:49:50AM -0400, Tom Lane wrote:
> > > Well, let's go ahead with it and see what happens. If it's too
> > > much of a mess we can always revert.
> >
> > Okay, done after an extra round of self-review. I have finished by
> > tweaking a couple of comments, and adjusted further TESTING to explain
> > what needs to be done to have a dump compatible with the test. Let's
> > now see what goes wrong.
>
> The REL_14 buildfarm client did not grab logs from the first failure:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse&dt=2022-04-01%2001%3A39%3A04
>
> The failure looked like this:
>
> # Running: diff -q /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump1.sql /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump2.sql
> /usr/bin/diff: illegal option -- q
> usage: diff [-bitw] [-c | -e | -f | -h | -n | -u] file1 file2
> diff [-bitw] [-C number | -U number] file1 file2
> diff [-bitw] [-D string] file1 file2
> diff [-bitw] [-c | -e | -f | -h | -n | -u] [-l] [-r] [-s] [-S name] directory1 directory2
> not ok 4 - old and new dump match after pg_upgrade
Is diff -q defined somewhere ? I can't find it in postgres sources nor in
sources for bf client.
Maybe your bf member could use git diff --exit-code --quiet ?
--
Justin
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-04-01 03:42 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-04-01 03:51 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Justin Pryzby <[email protected]>
@ 2022-04-01 06:01 ` Michael Paquier <[email protected]>
2022-04-01 11:53 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-04-01 06:01 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Noah Misch <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; [email protected]
On Thu, Mar 31, 2022 at 10:51:59PM -0500, Justin Pryzby wrote:
> Is diff -q defined somewhere ? I can't find it in postgres sources nor in
> sources for bf client.
322becb has added such a call, at the end of 002_pg_upgrade.pl.
vcregress.pl also has one before this commit.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YkaVQo44WFTAF2%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-04-01 03:42 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-04-01 03:51 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Justin Pryzby <[email protected]>
2022-04-01 06:01 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-04-01 11:53 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Michael Paquier @ 2022-04-01 11:53 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Noah Misch <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; [email protected]
On Fri, Apr 01, 2022 at 03:01:38PM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 10:51:59PM -0500, Justin Pryzby wrote:
>> Is diff -q defined somewhere ? I can't find it in postgres sources nor in
>> sources for bf client.
>
> 322becb has added such a call, at the end of 002_pg_upgrade.pl.
> vcregress.pl also has one before this commit.
The Windows animals seem to be in good shape, except hamerkop that
dies on "vcregress upgradecheck" when the TAP tests are disabled
causing the buildfarm client to stop. My idea to use upgradecheck
leads to more code than just moving the test to bincheck so let's
reuse the suggestion from Andres upthread and disable completely
upgradecheck, keeping the target around only for compatibility. The
attached does that, and the test of pg_upgrade would go through
bincheck instead.
It is late here, I'll try to get that patched up tomorrow.
--
Michael
Attachments:
[text/x-diff] win32-upgradecheck.patch (1.5K, ../../[email protected]/2-win32-upgradecheck.patch)
download | inline diff:
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 43d05bde4e..18101e7a70 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -470,7 +470,6 @@ $ENV{CONFIG}="Debug";
<userinput>vcregress isolationcheck</userinput>
<userinput>vcregress bincheck</userinput>
<userinput>vcregress recoverycheck</userinput>
-<userinput>vcregress upgradecheck</userinput>
</screen>
To change the schedule used (default is parallel), append it to the
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index e896820ac5..fbfc781508 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -106,7 +106,7 @@ my %command = (
ISOLATIONCHECK => \&isolationcheck,
BINCHECK => \&bincheck,
RECOVERYCHECK => \&recoverycheck,
- UPGRADECHECK => \&upgradecheck,
+ UPGRADECHECK => \&upgradecheck, # no-op
TAPTEST => \&taptest,);
my $proc = $command{$what};
@@ -286,9 +286,6 @@ sub bincheck
foreach my $dir (@bin_dirs)
{
next unless -d "$dir/t";
- # Do not consider pg_upgrade, as it is handled by
- # upgradecheck.
- next if ($dir =~ "/pg_upgrade/");
my $status = tap_check($dir);
$mstat ||= $status;
@@ -520,9 +517,9 @@ sub generate_db
sub upgradecheck
{
- InstallTemp();
- my $mstat = tap_check("$topdir/src/bin/pg_upgrade");
- exit $mstat if $mstat;
+ # pg_upgrade is now handled by bincheck, but keep this
+ # target for backward compatibility.
+ print "upgradecheck disabled\n";
return;
}
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-04-01 03:42 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-04-01 04:00 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 31+ messages in thread
From: Michael Paquier @ 2022-04-01 04:00 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Thu, Mar 31, 2022 at 08:42:41PM -0700, Noah Misch wrote:
> The failure looked like this:
>
> # Running: diff -q /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump1.sql /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_lPFv/dump2.sql
> /usr/bin/diff: illegal option -- q
> usage: diff [-bitw] [-c | -e | -f | -h | -n | -u] file1 file2
> diff [-bitw] [-C number | -U number] file1 file2
> diff [-bitw] [-D string] file1 file2
> diff [-bitw] [-c | -e | -f | -h | -n | -u] [-l] [-r] [-s] [-S name] directory1 directory2
> not ok 4 - old and new dump match after pg_upgrade
Ah, thanks for the information! So the problem was that the first
commit of the patch took the diff command from the MSVC scripts, and
there is no -q on Solaris 11.3. Using File::Compare should be enough
to fix the problem, then. Hopefully.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YkZ41Kx16%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-04-01 03:54 ` Michael Paquier <[email protected]>
2 siblings, 0 replies; 31+ messages in thread
From: Michael Paquier @ 2022-04-01 03:54 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> Okay, done after an extra round of self-review. I have finished by
> tweaking a couple of comments, and adjusted further TESTING to explain
> what needs to be done to have a dump compatible with the test. Let's
> now see what goes wrong.
So, the first reports are published, and the buildfarm is rather cool
on the matter. wrasse is the only buildfarm member that has reported
a failure, complaining that the dumps generated do not match. I am
not completely sure what's going on there, so I have applied an extra
patch to get more information from the logs on failures, and switched
the test to use File::Compare::compare() to check if the dumps match.
This last part feels safer in the long run, anyway. There should be a
diff command as previous runs used test.sh, so perhaps this is an
issue with its perl. The next report should tell more.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-05-02 04:27 ` Noah Misch <[email protected]>
2022-05-02 07:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2 siblings, 2 replies; 31+ messages in thread
From: Noah Misch @ 2022-05-02 04:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 09:49:50AM -0400, Tom Lane wrote:
> > Well, let's go ahead with it and see what happens. If it's too
> > much of a mess we can always revert.
>
> Okay, done after an extra round of self-review.
commit 322becb wrote:
> --- /dev/null
> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> +# Generate a database with a name made of a range of ASCII characters.
> +sub generate_db
> +{
> + my ($node, $from_char, $to_char) = @_;
> +
> + my $dbname = '';
> + for my $i ($from_char .. $to_char)
> + {
> + next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
> + $dbname = $dbname . sprintf('%c', $i);
> + }
> + $node->run_log(
> + [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
> + );
Nothing checks the command result, so the test file passes even if each of
these createdb calls fails. Other run_log() calls in this file have the same
problem. This particular one should be command_ok() or similar.
--host and --port are redundant in a PostgreSQL::Test::Cluster::run_log call,
because that call puts equivalent configuration in the environment. Other
calls in the file have the same redundant operands. (No other test file has
redundant --host or --port.)
> + # Grab any regression options that may be passed down by caller.
> + my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
Typo: s/_OPT/_OPTS/
> + my @extra_opts = split(/\s+/, $extra_opts_val);
src/test/recovery/t/027_stream_regress.pl and the makefiles treat
EXTRA_REGRESS_OPTS as a shell fragment. To be compatible, use the
src/test/recovery/t/027_stream_regress.pl approach. Affected usage patetrns
are not very important, but since the tree has code for it, you may as well
borrow that code. These examples witness the difference:
EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/bin/pg_upgrade check PROVE_TESTS=t/002_pg_upgrade.pl
# log has: /home/nm/src/pg/postgresql/src/bin/pg_upgrade/../../../src/test/regress/pg_regress: unrecognized option '--nosuc"'
EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/test/recovery check PROVE_TESTS=t/027_stream_regress.pl
# log has: /home/nm/src/pg/postgresql/src/test/recovery/../../../src/test/regress/pg_regress: unrecognized option '--nosuc h'
> --- a/src/bin/pg_upgrade/test.sh
> +++ /dev/null
> -# Create databases with names covering the ASCII bytes other than NUL, BEL,
> -# LF, or CR. BEL would ring the terminal bell in the course of this test, and
> -# it is not otherwise a special case. PostgreSQL doesn't support the rest.
> -dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
> - if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
> -# Exercise backslashes adjacent to double quotes, a Windows special case.
> -dbname1='\"\'$dbname1'\\"\\\'
This rewrite dropped the exercise of backslashes adjacent to double quotes.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-05-02 07:00 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 31+ messages in thread
From: Michael Paquier @ 2022-05-02 07:00 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Sun, May 01, 2022 at 09:27:18PM -0700, Noah Misch wrote:
> commit 322becb wrote:
Thanks, Noah. I am out this week, but I should be able to address all
your points at the beginning of next week. I have added an open item
for now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-05-09 03:18 ` Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-05-09 03:18 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Sun, May 01, 2022 at 09:27:18PM -0700, Noah Misch wrote:
> On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
>> commit 322becb wrote:
>
> Nothing checks the command result, so the test file passes even if each of
> these createdb calls fails. Other run_log() calls in this file have the same
> problem. This particular one should be command_ok() or similar.
All of them could rely on command_ok(), as they should never fail, so
switched this way.
> --host and --port are redundant in a PostgreSQL::Test::Cluster::run_log call,
> because that call puts equivalent configuration in the environment. Other
> calls in the file have the same redundant operands. (No other test file has
> redundant --host or --port.)
Right. Removed all that.
>> + # Grab any regression options that may be passed down by caller.
>> + my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
>
> Typo: s/_OPT/_OPTS/
Oops, fixed.
>> + my @extra_opts = split(/\s+/, $extra_opts_val);
>
> src/test/recovery/t/027_stream_regress.pl and the makefiles treat
> EXTRA_REGRESS_OPTS as a shell fragment. To be compatible, use the
> src/test/recovery/t/027_stream_regress.pl approach. Affected usage patetrns
> are not very important, but since the tree has code for it, you may as well
> borrow that code. These examples witness the difference:
So the pattern of EXTRA_REGRESS_OPTS being used in the Makefiles is
the decision point here. Makes sense.
>> -# Create databases with names covering the ASCII bytes other than NUL, BEL,
>> -# LF, or CR. BEL would ring the terminal bell in the course of this test, and
>> -# it is not otherwise a special case. PostgreSQL doesn't support the rest.
>> -dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
>> - if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
>> -# Exercise backslashes adjacent to double quotes, a Windows special case.
>> -dbname1='\"\'$dbname1'\\"\\\'
>
> This rewrite dropped the exercise of backslashes adjacent to double quotes.
Damn, thanks. If I am reading that right, this could be done with the
following addition in generate_db(), adding double quotes surrounded
by backslashes before and after the database name:
$dbname = '\\"\\' . $dbname . '\\"\\';
All these fixes lead me to the attached patch. Does that look fine to
you?
Thanks,
--
Michael
Attachments:
[text/x-diff] upgrade-tap-fixes-noah.patch (3.3K, ../../[email protected]/2-upgrade-tap-fixes-noah.patch)
download | inline diff:
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 05bf161843..4002982dc0 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -21,9 +21,11 @@ sub generate_db
next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
$dbname = $dbname . sprintf('%c', $i);
}
- $node->run_log(
- [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
- );
+
+ # Exercise backslashes adjacent to double quotes, a Windows special
+ # case.
+ $dbname = '\\"\\' . $dbname . '\\"\\';
+ $node->command_ok([ 'createdb', $dbname ]);
}
# The test of pg_upgrade requires two clusters, an old one and a new one
@@ -70,12 +72,7 @@ if (defined($ENV{olddump}))
# Load the dump using the "postgres" database as "regression" does
# not exist yet, and we are done here.
- $oldnode->command_ok(
- [
- 'psql', '-X', '-f', $olddumpfile,
- '--port', $oldnode->port, '--host', $oldnode->host,
- 'postgres'
- ]);
+ $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ]);
}
else
{
@@ -87,8 +84,7 @@ else
generate_db($oldnode, 91, 127);
# Grab any regression options that may be passed down by caller.
- my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
- my @extra_opts = split(/\s+/, $extra_opts_val);
+ my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";
# --dlpath is needed to be able to find the location of regress.so
# and any libraries the regression tests require.
@@ -100,19 +96,19 @@ else
# --inputdir points to the path of the input files.
my $inputdir = "$srcdir/src/test/regress";
- my @regress_command = [
- $ENV{PG_REGRESS}, @extra_opts,
- '--dlpath', $dlpath,
- '--max-concurrent-tests', '20',
- '--bindir=', '--host',
- $oldnode->host, '--port',
- $oldnode->port, '--schedule',
- "$srcdir/src/test/regress/parallel_schedule", '--outputdir',
- $outputdir, '--inputdir',
- $inputdir
- ];
-
- my $rc = run_log(@regress_command);
+ my $rc =
+ system($ENV{PG_REGRESS}
+ . "$extra_opts "
+ . "--dlpath=\"$dlpath\" "
+ . "--bindir= "
+ . "--host="
+ . $oldnode->host . " "
+ . "--port="
+ . $oldnode->port . " "
+ . "--schedule=$srcdir/src/test/regress/parallel_schedule "
+ . "--max-concurrent-tests=20 "
+ . "--inputdir=\"$inputdir\" "
+ . "--outputdir=\"$outputdir\"");
if ($rc != 0)
{
# Dump out the regression diffs file, if there is one
@@ -133,12 +129,10 @@ if (defined($ENV{oldinstall}))
{
# Note that upgrade_adapt.sql from the new version is used, to
# cope with an upgrade to this version.
- $oldnode->run_log(
+ $oldnode->command_ok(
[
- 'psql', '-X',
- '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
- '--port', $oldnode->port,
- '--host', $oldnode->host,
+ 'psql', '-X',
+ '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
'regression'
]);
}
@@ -232,7 +226,7 @@ if (-d $log_path)
}
# Second dump from the upgraded instance.
-$newnode->run_log(
+$newnode->command_ok(
[
'pg_dumpall', '--no-sync',
'-d', $newnode->connstr('postgres'),
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-05-11 01:29 ` Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-05-11 01:29 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Mon, May 09, 2022 at 12:18:39PM +0900, Michael Paquier wrote:
> All these fixes lead me to the attached patch.
I have applied this stuff as of 7dd3ee5, in time for beta1, and closed
the open item. One difference is that I've added one backslash
surrounding the double quote at the beginning *and* the end of the
database name in the patch. However, the original case was different,
with:
- At the beginning of the database name, one backslash before and
after the double quote.
- At the end of the database name, two backslaces before the double
quote and three after the double quote.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-05-11 05:32 ` Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Noah Misch @ 2022-05-11 05:32 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Wed, May 11, 2022 at 10:29:44AM +0900, Michael Paquier wrote:
> On Mon, May 09, 2022 at 12:18:39PM +0900, Michael Paquier wrote:
> > All these fixes lead me to the attached patch.
>
> I have applied this stuff as of 7dd3ee5, in time for beta1, and closed
> the open item. One difference is that I've added one backslash
> surrounding the double quote at the beginning *and* the end of the
> database name in the patch. However, the original case was different,
> with:
> - At the beginning of the database name, one backslash before and
> after the double quote.
> - At the end of the database name, two backslaces before the double
> quote and three after the double quote.
Why did you discontinue testing the longstanding test database name?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-05-12 05:27 ` Michael Paquier <[email protected]>
2022-05-14 08:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-05-12 05:27 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Tue, May 10, 2022 at 10:32:55PM -0700, Noah Misch wrote:
> Why did you discontinue testing the longstanding test database name?
I am not sure what you mean here. Are you saying that the test should
be changed to prefix each database name by "regression", as it was the
case in test.sh? Or do you mean that the backslash/double-quote
business should only apply to the first database name and not the
other two, implying that the new generate_db() in 002_pg_upgrade.pl
had better have a $prefix and a $suffix like it was originally
written?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-05-14 08:27 ` Noah Misch <[email protected]>
2022-05-16 05:30 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Noah Misch @ 2022-05-14 08:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Thu, May 12, 2022 at 02:27:30PM +0900, Michael Paquier wrote:
> On Tue, May 10, 2022 at 10:32:55PM -0700, Noah Misch wrote:
> > On Wed, May 11, 2022 at 10:29:44AM +0900, Michael Paquier wrote:
> > > On Mon, May 09, 2022 at 12:18:39PM +0900, Michael Paquier wrote:
> > > > All these fixes lead me to the attached patch.
> > >
> > > I have applied this stuff as of 7dd3ee5, in time for beta1, and closed
> > > the open item. One difference is that I've added one backslash
> > > surrounding the double quote at the beginning *and* the end of the
> > > database name in the patch. However, the original case was different,
> > > with:
> > > - At the beginning of the database name, one backslash before and
> > > after the double quote.
> > > - At the end of the database name, two backslaces before the double
> > > quote and three after the double quote.
Here, you describe differences between test.sh and your rewrite of test.sh.
> > Why did you discontinue testing the longstanding test database name?
>
> I am not sure what you mean here.
Here, I requested the rationale for the differences you had just described.
You made a choice to stop testing one list of database names and start testing
a different list of database names. Why?
> Are you saying that the test should
> be changed to prefix each database name by "regression", as it was the
> case in test.sh? Or do you mean that the backslash/double-quote
> business should only apply to the first database name and not the
> other two, implying that the new generate_db() in 002_pg_upgrade.pl
> had better have a $prefix and a $suffix like it was originally
> written?
No, I wasn't saying any of those. (Later, I may say one or more of those.)
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-14 08:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-05-16 05:30 ` Michael Paquier <[email protected]>
2022-05-18 08:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-05-16 05:30 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Sat, May 14, 2022 at 01:27:28AM -0700, Noah Misch wrote:
> Here, I requested the rationale for the differences you had just described.
> You made a choice to stop testing one list of database names and start testing
> a different list of database names. Why?
Because the shape of the new names does not change the test coverage
("regression" prefix or the addition of the double quotes with
backslashes for all the database names), while keeping the code a bit
simpler. If you think that the older names are more adapted, I have
no objections to use them, FWIW, which is something like the patch
attached would achieve.
This uses the same convention as vcregress.pl before 322becb, but not
the one of test.sh where "regression" was appended to the database
names.
--
Michael
Attachments:
[text/x-diff] upgrade-tap-fixes-noah-2.patch (1.5K, ../../[email protected]/2-upgrade-tap-fixes-noah-2.patch)
download | inline diff:
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 8372a85e6e..33a75991d8 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -13,18 +13,16 @@ use Test::More;
# Generate a database with a name made of a range of ASCII characters.
sub generate_db
{
- my ($node, $from_char, $to_char) = @_;
+ my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
- my $dbname = '';
+ my $dbname = $prefix;
for my $i ($from_char .. $to_char)
{
next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
$dbname = $dbname . sprintf('%c', $i);
}
- # Exercise backslashes adjacent to double quotes, a Windows special
- # case.
- $dbname = '\\"\\' . $dbname . '\\\\"\\\\\\';
+ $dbname .= $suffix;
$node->command_ok([ 'createdb', $dbname ]);
}
@@ -79,10 +77,12 @@ else
{
# Default is to use pg_regress to set up the old instance.
- # Create databases with names covering most ASCII bytes
- generate_db($oldnode, 1, 45);
- generate_db($oldnode, 46, 90);
- generate_db($oldnode, 91, 127);
+ # Create databases with names covering most ASCII bytes. The
+ # first name exercises backslashes adjacent to double quotes, a
+ # Windows special case.
+ generate_db($oldnode, "\\\"\\", 1, 45, "\\\\\"\\\\\\");
+ generate_db($oldnode, '', 46, 90, '');
+ generate_db($oldnode, '', 91, 127, '');
# Grab any regression options that may be passed down by caller.
my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-14 08:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-16 05:30 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-05-18 08:03 ` Noah Misch <[email protected]>
2022-05-18 09:20 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Noah Misch @ 2022-05-18 08:03 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Mon, May 16, 2022 at 02:30:00PM +0900, Michael Paquier wrote:
> On Sat, May 14, 2022 at 01:27:28AM -0700, Noah Misch wrote:
> > Here, I requested the rationale for the differences you had just described.
> > You made a choice to stop testing one list of database names and start testing
> > a different list of database names. Why?
>
> Because the shape of the new names does not change the test coverage
> ("regression" prefix or the addition of the double quotes with
> backslashes for all the database names), while keeping the code a bit
> simpler. If you think that the older names are more adapted, I have
> no objections to use them, FWIW, which is something like the patch
> attached would achieve.
>
> This uses the same convention as vcregress.pl before 322becb, but not
> the one of test.sh where "regression" was appended to the database
> names.
I would have picked the test.sh names, both because test.sh was the senior
implementation and because doing so avoids warnings under
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS. See the warnings here:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2022-05-18%2000%3A59%3A...
More-notable line from that same log:
sh: /Users/buildfarm/bf-data/HEAD/pgsql.build/src/bin/pg_upgrade/../../../src/test/regress/pg_regress--port=5678: No such file or directory
Commit 7dd3ee5 adopted much of the 027_stream_regress.pl approach to running
pg_regress, but it didn't grab the "is($rc, 0, 'regression tests pass')"
needed to make defects like that report a failure.
> --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> @@ -13,18 +13,16 @@ use Test::More;
> # Generate a database with a name made of a range of ASCII characters.
> sub generate_db
> {
> - my ($node, $from_char, $to_char) = @_;
> + my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
>
> - my $dbname = '';
> + my $dbname = $prefix;
> for my $i ($from_char .. $to_char)
> {
> next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
> $dbname = $dbname . sprintf('%c', $i);
> }
>
> - # Exercise backslashes adjacent to double quotes, a Windows special
> - # case.
> - $dbname = '\\"\\' . $dbname . '\\\\"\\\\\\';
> + $dbname .= $suffix;
> $node->command_ok([ 'createdb', $dbname ]);
> }
>
> @@ -79,10 +77,12 @@ else
> {
> # Default is to use pg_regress to set up the old instance.
>
> - # Create databases with names covering most ASCII bytes
> - generate_db($oldnode, 1, 45);
> - generate_db($oldnode, 46, 90);
> - generate_db($oldnode, 91, 127);
> + # Create databases with names covering most ASCII bytes. The
> + # first name exercises backslashes adjacent to double quotes, a
> + # Windows special case.
> + generate_db($oldnode, "\\\"\\", 1, 45, "\\\\\"\\\\\\");
> + generate_db($oldnode, '', 46, 90, '');
> + generate_db($oldnode, '', 91, 127, '');
Does this pass on Windows? I'm 65% confident that released IPC::Run can't
handle this input due to https://github.com/toddr/IPC-Run/issues/142. If it's
passing for you on Windows, then disregard.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-14 08:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-16 05:30 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-18 08:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-05-18 09:20 ` Michael Paquier <[email protected]>
2022-05-21 01:28 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2022-05-18 09:20 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Wed, May 18, 2022 at 01:03:15AM -0700, Noah Misch wrote:
> On Mon, May 16, 2022 at 02:30:00PM +0900, Michael Paquier wrote:
>> Because the shape of the new names does not change the test coverage
>> ("regression" prefix or the addition of the double quotes with
>> backslashes for all the database names), while keeping the code a bit
>> simpler. If you think that the older names are more adapted, I have
>> no objections to use them, FWIW, which is something like the patch
>> attached would achieve.
>>
>> This uses the same convention as vcregress.pl before 322becb, but not
>> the one of test.sh where "regression" was appended to the database
>> names.
>
> I would have picked the test.sh names, both because test.sh was the senior
> implementation and because doing so avoids warnings under
> -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS. See the warnings here:
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2022-05-18%2000%3A59%3A...
Yes, I saw that. This did not bother me much as the TAP tests run in
isolation, but I am fine to stick to your option and silence these.
> More-notable line from that same log:
> sh: /Users/buildfarm/bf-data/HEAD/pgsql.build/src/bin/pg_upgrade/../../../src/test/regress/pg_regress--port=5678: No such file or directory
So you are using EXTRA_REGRESS_OPTS, then, and a space is missing from
the first argument of the command used to make that work properly.
> Commit 7dd3ee5 adopted much of the 027_stream_regress.pl approach to running
> pg_regress, but it didn't grab the "is($rc, 0, 'regression tests pass')"
> needed to make defects like that report a failure.
Okay, added this one.
>> + generate_db($oldnode, "\\\"\\", 1, 45, "\\\\\"\\\\\\");
>> + generate_db($oldnode, '', 46, 90, '');
>> + generate_db($oldnode, '', 91, 127, '');
>
> Does this pass on Windows? I'm 65% confident that released IPC::Run can't
> handle this input due to https://github.com/toddr/IPC-Run/issues/142. If it's
> passing for you on Windows, then disregard.
Hmm. The CI has been passing for me with this name pattern in place,
as of https://github.com/michaelpq/postgres/tree/upgrade_tap_fixes.
Attached is an updated patch to address your concerns.
--
Michael
Attachments:
[text/x-diff] upgrade-tap-fixes-noah-3.patch (1.9K, ../../[email protected]/2-upgrade-tap-fixes-noah-3.patch)
download | inline diff:
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 8372a85e6e..86fe1b4d09 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -13,18 +13,16 @@ use Test::More;
# Generate a database with a name made of a range of ASCII characters.
sub generate_db
{
- my ($node, $from_char, $to_char) = @_;
+ my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
- my $dbname = '';
+ my $dbname = $prefix;
for my $i ($from_char .. $to_char)
{
next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
$dbname = $dbname . sprintf('%c', $i);
}
- # Exercise backslashes adjacent to double quotes, a Windows special
- # case.
- $dbname = '\\"\\' . $dbname . '\\\\"\\\\\\';
+ $dbname .= $suffix;
$node->command_ok([ 'createdb', $dbname ]);
}
@@ -79,10 +77,12 @@ else
{
# Default is to use pg_regress to set up the old instance.
- # Create databases with names covering most ASCII bytes
- generate_db($oldnode, 1, 45);
- generate_db($oldnode, 46, 90);
- generate_db($oldnode, 91, 127);
+ # Create databases with names covering most ASCII bytes. The
+ # first name exercises backslashes adjacent to double quotes, a
+ # Windows special case.
+ generate_db($oldnode, 'regression\\"\\', 1, 45, '\\\\"\\\\\\');
+ generate_db($oldnode, 'regression', 46, 90, '');
+ generate_db($oldnode, 'regression', 91, 127, '');
# Grab any regression options that may be passed down by caller.
my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";
@@ -99,7 +99,7 @@ else
my $rc =
system($ENV{PG_REGRESS}
- . "$extra_opts "
+ . " $extra_opts "
. "--dlpath=\"$dlpath\" "
. "--bindir= "
. "--host="
@@ -121,6 +121,7 @@ else
print "=== EOF ===\n";
}
}
+ is($rc, 0, 'regression tests pass');
}
# Before dumping, get rid of objects not existing or not supported in later
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-14 08:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-16 05:30 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-18 08:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-18 09:20 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
@ 2022-05-21 01:28 ` Noah Misch <[email protected]>
2022-05-21 03:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Noah Misch @ 2022-05-21 01:28 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Wed, May 18, 2022 at 06:20:08PM +0900, Michael Paquier wrote:
> Attached is an updated patch to address your concerns.
Looks successful.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 08:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-03 05:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 04:10 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 05:00 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-03-31 05:36 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]>
2022-03-31 07:56 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-31 13:49 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]>
2022-04-01 01:16 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-02 04:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-09 03:18 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 01:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-11 05:32 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-12 05:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-14 08:27 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-16 05:30 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-18 08:03 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
2022-05-18 09:20 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-05-21 01:28 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Noah Misch <[email protected]>
@ 2022-05-21 03:03 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Michael Paquier @ 2022-05-21 03:03 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Thomas Munro <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]>
On Fri, May 20, 2022 at 06:28:01PM -0700, Noah Misch wrote:
> Looks successful.
Thanks a lot for confirming. I have applied that on HEAD, then.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YohWm%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH v6 1/2] Track IO times in pg_stat_io
@ 2023-03-06 15:41 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Melanie Plageman @ 2023-03-06 15:41 UTC (permalink / raw)
Add IO timing for reads, writes, extends, and fsyncs to pg_stat_io.
Reviewed-by: Bertrand Drouvot <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ay5iKmnbXZ3DsauViF3eMxu4m1oNnJXqV_HyqYeg55Ww%40mai...
---
src/include/catalog/pg_proc.dat | 6 +-
src/include/pgstat.h | 14 +++-
src/backend/catalog/system_views.sql | 4 +
src/backend/storage/buffer/bufmgr.c | 56 ++++++--------
src/backend/storage/buffer/localbuf.c | 6 +-
src/backend/storage/smgr/md.c | 27 ++++---
src/backend/utils/activity/pgstat.c | 77 ++++++++++++++++++-
src/backend/utils/activity/pgstat_io.c | 101 ++++++++++++++++++++-----
src/backend/utils/adt/pgstatfuncs.c | 48 ++++++++++--
doc/src/sgml/monitoring.sgml | 59 +++++++++++++++
src/test/regress/expected/rules.out | 6 +-
11 files changed, 326 insertions(+), 78 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 5cf87aeb2c4..4148ec09ae7 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5744,9 +5744,9 @@
proname => 'pg_stat_get_io', provolatile => 'v',
prorows => '30', proretset => 't',
proparallel => 'r', prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,text,text,int8,int8,int8,int8,int8,int8,int8,timestamptz}',
- proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{backend_type,io_object,io_context,reads,writes,extends,op_bytes,evictions,reuses,fsyncs,stats_reset}',
+ proallargtypes => '{text,text,text,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,float8,timestamptz}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{backend_type,io_object,io_context,reads,read_time,writes,write_time,extends,extend_time,op_bytes,evictions,reuses,fsyncs,fsync_time,stats_reset}',
prosrc => 'pg_stat_get_io' },
{ oid => '1136', descr => 'statistics: information about WAL activity',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 1e418b682b5..288a8c7c0ef 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -304,7 +304,12 @@ typedef enum IOOp
typedef struct PgStat_BktypeIO
{
- PgStat_Counter data[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
+ PgStat_Counter counts[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
+ union
+ {
+ instr_time pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
+ PgStat_Counter times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
+ };
} PgStat_BktypeIO;
typedef struct PgStat_IO
@@ -466,6 +471,11 @@ extern TimestampTz pgstat_get_stat_snapshot_timestamp(bool *have_snapshot);
extern PgStat_Kind pgstat_get_kind_from_str(char *kind_str);
extern bool pgstat_have_entry(PgStat_Kind kind, Oid dboid, Oid objoid);
+extern instr_time pgstat_io_start(void);
+
+extern void pgstat_io_end(instr_time io_start, IOObject io_object,
+ IOContext io_context, IOOp io_op);
+
/*
* Functions in pgstat_archiver.c
@@ -498,6 +508,7 @@ extern PgStat_CheckpointerStats *pgstat_fetch_stat_checkpointer(void);
extern bool pgstat_bktype_io_stats_valid(PgStat_BktypeIO *context_ops,
BackendType bktype);
extern void pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op);
+extern void pgstat_count_io_time(IOObject io_object, IOContext io_context, IOOp io_op, instr_time time);
extern PgStat_IO *pgstat_fetch_stat_io(void);
extern const char *pgstat_get_io_context_name(IOContext io_context);
extern const char *pgstat_get_io_object_name(IOObject io_object);
@@ -507,6 +518,7 @@ extern bool pgstat_tracks_io_object(BackendType bktype,
IOObject io_object, IOContext io_context);
extern bool pgstat_tracks_io_op(BackendType bktype, IOObject io_object,
IOContext io_context, IOOp io_op);
+extern IOOp pgstat_tracks_io_time(IOOp io_op);
/*
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 34ca0e739f5..39391bc2fc0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1123,12 +1123,16 @@ SELECT
b.io_object,
b.io_context,
b.reads,
+ b.read_time,
b.writes,
+ b.write_time,
b.extends,
+ b.extend_time,
b.op_bytes,
b.evictions,
b.reuses,
b.fsyncs,
+ b.fsync_time,
b.stats_reset
FROM pg_stat_get_io() b;
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 0a05577b68d..64857f1ff68 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1000,12 +1000,17 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
if (isExtend)
{
+ instr_time io_start;
+
/* new buffers are zero-filled */
MemSet((char *) bufBlock, 0, BLCKSZ);
+
+ io_start = pgstat_io_start();
+
/* don't set checksum for all-zero page */
smgrextend(smgr, forkNum, blockNum, bufBlock, false);
- pgstat_count_io_op(io_object, io_context, IOOP_EXTEND);
+ pgstat_io_end(io_start, io_object, io_context, IOOP_EXTEND);
/*
* NB: we're *not* doing a ScheduleBufferTagForWriteback here;
@@ -1024,25 +1029,13 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
MemSet((char *) bufBlock, 0, BLCKSZ);
else
{
- instr_time io_start,
- io_time;
+ instr_time io_start;
- if (track_io_timing)
- INSTR_TIME_SET_CURRENT(io_start);
- else
- INSTR_TIME_SET_ZERO(io_start);
+ io_start = pgstat_io_start();
smgrread(smgr, forkNum, blockNum, bufBlock);
- pgstat_count_io_op(io_object, io_context, IOOP_READ);
-
- if (track_io_timing)
- {
- INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));
- INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);
- }
+ pgstat_io_end(io_start, io_object, io_context, IOOP_READ);
/* check for garbage data */
if (!PageIsVerifiedExtended((Page) bufBlock, blockNum,
@@ -2848,6 +2841,7 @@ BufferGetTag(Buffer buffer, RelFileLocator *rlocator, ForkNumber *forknum,
*blknum = bufHdr->tag.blockNum;
}
+
/*
* FlushBuffer
* Physically write out a shared buffer.
@@ -2873,8 +2867,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
{
XLogRecPtr recptr;
ErrorContextCallback errcallback;
- instr_time io_start,
- io_time;
+ instr_time io_start;
Block bufBlock;
char *bufToWrite;
uint32 buf_state;
@@ -2949,10 +2942,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
*/
bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum);
- if (track_io_timing)
- INSTR_TIME_SET_CURRENT(io_start);
- else
- INSTR_TIME_SET_ZERO(io_start);
+ io_start = pgstat_io_start();
/*
* bufToWrite is either the shared buffer or a copy, as appropriate.
@@ -2981,17 +2971,7 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object,
* When a strategy is not in use, the write can only be a "regular" write
* of a dirty shared buffer (IOCONTEXT_NORMAL IOOP_WRITE).
*/
- pgstat_count_io_op(IOOBJECT_RELATION, io_context, IOOP_WRITE);
-
- if (track_io_timing)
- {
- INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time));
- INSTR_TIME_ADD(pgBufferUsage.blk_write_time, io_time);
- }
-
- pgBufferUsage.shared_blks_written++;
+ pgstat_io_end(io_start, IOOBJECT_RELATION, io_context, IOOP_WRITE);
/*
* Mark the buffer as clean (unless BM_JUST_DIRTIED has become set) and
@@ -3597,6 +3577,7 @@ FlushRelationBuffers(Relation rel)
for (i = 0; i < NLocBuffer; i++)
{
uint32 buf_state;
+ instr_time io_start;
bufHdr = GetLocalBufferDescriptor(i);
if (BufTagMatchesRelFileLocator(&bufHdr->tag, &rel->rd_locator) &&
@@ -3616,6 +3597,12 @@ FlushRelationBuffers(Relation rel)
PageSetChecksumInplace(localpage, bufHdr->tag.blockNum);
+ /*
+ * TODO: is it okay that this formerly didn't count
+ * pgBufferUsage or pgStatBlockReadTime
+ */
+ io_start = pgstat_io_start();
+
smgrwrite(RelationGetSmgr(rel),
BufTagGetForkNum(&bufHdr->tag),
bufHdr->tag.blockNum,
@@ -3625,13 +3612,14 @@ FlushRelationBuffers(Relation rel)
buf_state &= ~(BM_DIRTY | BM_JUST_DIRTIED);
pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
- pgstat_count_io_op(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_WRITE);
+ pgstat_io_end(io_start, IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_WRITE);
/* Pop the error context stack */
error_context_stack = errcallback.previous;
}
}
+
return;
}
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 5325ddb663d..af45a72934a 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -220,6 +220,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
*/
if (buf_state & BM_DIRTY)
{
+ instr_time io_start;
SMgrRelation oreln;
Page localpage = (char *) LocalBufHdrGetBlock(bufHdr);
@@ -228,6 +229,8 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
PageSetChecksumInplace(localpage, bufHdr->tag.blockNum);
+ io_start = pgstat_io_start();
+
/* And write... */
smgrwrite(oreln,
BufTagGetForkNum(&bufHdr->tag),
@@ -239,8 +242,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
buf_state &= ~BM_DIRTY;
pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
- pgstat_count_io_op(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_WRITE);
- pgBufferUsage.local_blks_written++;
+ pgstat_io_end(io_start, IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_WRITE);
}
/*
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 352958e1feb..2440211f03d 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1030,6 +1030,17 @@ register_dirty_segment(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg)
if (!RegisterSyncRequest(&tag, SYNC_REQUEST, false /* retryOnError */ ))
{
+ instr_time io_start = pgstat_io_start();
+
+ ereport(DEBUG1,
+ (errmsg_internal("could not forward fsync request because request queue is full")));
+
+ if (FileSync(seg->mdfd_vfd, WAIT_EVENT_DATA_FILE_SYNC) < 0)
+ ereport(data_sync_elevel(ERROR),
+ (errcode_for_file_access(),
+ errmsg("could not fsync file \"%s\": %m",
+ FilePathName(seg->mdfd_vfd))));
+
/*
* We have no way of knowing if the current IOContext is
* IOCONTEXT_NORMAL or IOCONTEXT_[BULKREAD, BULKWRITE, VACUUM] at this
@@ -1041,16 +1052,7 @@ register_dirty_segment(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg)
* IOCONTEXT_NORMAL is likely clearer when investigating the number of
* backend fsyncs.
*/
- pgstat_count_io_op(IOOBJECT_RELATION, IOCONTEXT_NORMAL, IOOP_FSYNC);
-
- ereport(DEBUG1,
- (errmsg_internal("could not forward fsync request because request queue is full")));
-
- if (FileSync(seg->mdfd_vfd, WAIT_EVENT_DATA_FILE_SYNC) < 0)
- ereport(data_sync_elevel(ERROR),
- (errcode_for_file_access(),
- errmsg("could not fsync file \"%s\": %m",
- FilePathName(seg->mdfd_vfd))));
+ pgstat_io_end(io_start, IOOBJECT_RELATION, IOCONTEXT_NORMAL, IOOP_FSYNC);
}
}
@@ -1399,6 +1401,7 @@ int
mdsyncfiletag(const FileTag *ftag, char *path)
{
SMgrRelation reln = smgropen(ftag->rlocator, InvalidBackendId);
+ instr_time io_start;
File file;
bool need_to_close;
int result,
@@ -1425,6 +1428,8 @@ mdsyncfiletag(const FileTag *ftag, char *path)
need_to_close = true;
}
+ io_start = pgstat_io_start();
+
/* Sync the file. */
result = FileSync(file, WAIT_EVENT_DATA_FILE_SYNC);
save_errno = errno;
@@ -1432,7 +1437,7 @@ mdsyncfiletag(const FileTag *ftag, char *path)
if (need_to_close)
FileClose(file);
- pgstat_count_io_op(IOOBJECT_RELATION, IOCONTEXT_NORMAL, IOOP_FSYNC);
+ pgstat_io_end(io_start, IOOBJECT_RELATION, IOCONTEXT_NORMAL, IOOP_FSYNC);
errno = save_errno;
return result;
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b125802b215..9f5f041d7c2 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -95,9 +95,11 @@
#include "access/transam.h"
#include "access/xact.h"
+#include "executor/instrument.h"
#include "lib/dshash.h"
#include "pgstat.h"
#include "port/atomics.h"
+#include "storage/bufmgr.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/lwlock.h"
@@ -108,7 +110,6 @@
#include "utils/pgstat_internal.h"
#include "utils/timestamp.h"
-
/* ----------
* Timer definitions.
*
@@ -906,6 +907,80 @@ pgstat_have_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
return pgstat_get_entry_ref(kind, dboid, objoid, false, NULL) != NULL;
}
+instr_time
+pgstat_io_start(void)
+{
+ instr_time io_start;
+
+ if (track_io_timing)
+ INSTR_TIME_SET_CURRENT(io_start);
+ else
+ INSTR_TIME_SET_ZERO(io_start);
+
+ return io_start;
+}
+
+void
+pgstat_io_end(instr_time io_start, IOObject io_object,
+ IOContext io_context, IOOp io_op)
+{
+ instr_time io_time;
+
+ if (track_io_timing)
+ {
+ INSTR_TIME_SET_CURRENT(io_time);
+ INSTR_TIME_SUBTRACT(io_time, io_start);
+ pgstat_count_io_time(io_object, io_context, io_op, io_time);
+
+ if (io_op == IOOP_WRITE)
+ {
+ if (io_object == IOOBJECT_RELATION)
+ {
+ /* TODO: AFAICT, pgstat_count_buffer_write_time is only called */
+ /* for shared buffers whereas pgstat_count_buffer_read_time is */
+ /* called for temp relations and shared buffers. */
+ /*
+ * is this intentional and should I match current behavior or
+ * not?
+ */
+ pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time));
+ INSTR_TIME_ADD(pgBufferUsage.blk_write_time, io_time);
+ }
+ }
+ else if (io_op == IOOP_READ)
+ {
+ pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));
+ if (io_object == IOOBJECT_RELATION)
+ {
+ INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);
+ }
+ }
+ }
+
+ pgstat_count_io_op(io_object, io_context, io_op);
+
+ if (io_op == IOOP_WRITE)
+ {
+ if (io_object == IOOBJECT_RELATION)
+ pgBufferUsage.shared_blks_written++;
+ else if (io_object == IOOBJECT_TEMP_RELATION)
+ pgBufferUsage.local_blks_written++;
+ }
+
+ /*
+ * TODO: this is normally done later in ReadBuffer_common() is it okay to
+ * do here?
+ */
+ else if (io_op == IOOP_READ)
+ {
+ if (io_object == IOOBJECT_RELATION)
+ pgBufferUsage.shared_blks_read++;
+ else if (io_object == IOOBJECT_TEMP_RELATION)
+ pgBufferUsage.local_blks_read++;
+ }
+}
+
+
/*
* Ensure snapshot for fixed-numbered 'kind' exists.
*
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index af5d5546101..fe09cbf16d4 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -25,36 +25,47 @@ bool have_iostats = false;
/*
* Check that stats have not been counted for any combination of IOObject,
- * IOContext, and IOOp which are not tracked for the passed-in BackendType. The
- * passed-in PgStat_BktypeIO must contain stats from the BackendType specified
- * by the second parameter. Caller is responsible for locking the passed-in
- * PgStat_BktypeIO, if needed.
+ * IOContext, and IOOp which are not tracked for the passed-in BackendType. If
+ * the IOOp is not counted for this combination but IO time is otherwise
+ * tracked for this IOOp, check that IO time has not been counted for this
+ * combination. If stats are tracked for this combination and IO times are
+ * non-zero, counts should be non-zero.
+ *
+ * The passed-in PgStat_BktypeIO must contain stats from the BackendType
+ * specified by the second parameter. Caller is responsible for locking the
+ * passed-in PgStat_BktypeIO, if needed.
*/
bool
pgstat_bktype_io_stats_valid(PgStat_BktypeIO *backend_io,
BackendType bktype)
{
- bool bktype_tracked = pgstat_tracks_io_bktype(bktype);
-
for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
{
for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
{
- /*
- * Don't bother trying to skip to the next loop iteration if
- * pgstat_tracks_io_object() would return false here. We still
- * need to validate that each counter is zero anyway.
- */
for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
{
- /* No stats, so nothing to validate */
- if (backend_io->data[io_object][io_context][io_op] == 0)
- continue;
+ /* we do track it */
+ if (pgstat_tracks_io_op(bktype, io_object, io_context, io_op))
+ {
+ /* ensure that if IO times are non-zero, counts are > 0 */
+ if (backend_io->times[io_object][io_context][io_op] != 0 &&
+ backend_io->counts[io_object][io_context][io_op] <= 0)
+ return false;
- /* There are stats and there shouldn't be */
- if (!bktype_tracked ||
- !pgstat_tracks_io_op(bktype, io_object, io_context, io_op))
+ continue;
+ }
+
+ /* we don't track it, and it is not 0 */
+ if (backend_io->counts[io_object][io_context][io_op] != 0)
return false;
+
+ /* we don't track this IOOp, so make sure its IO time is zero */
+ if (pgstat_tracks_io_time(io_op) > -1)
+ {
+ if (backend_io->times[io_object][io_context][io_op] != 0)
+ return false;
+ }
}
}
}
@@ -70,7 +81,21 @@ pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op)
Assert((unsigned int) io_op < IOOP_NUM_TYPES);
Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
- PendingIOStats.data[io_object][io_context][io_op]++;
+ PendingIOStats.counts[io_object][io_context][io_op]++;
+
+ have_iostats = true;
+}
+
+void
+pgstat_count_io_time(IOObject io_object, IOContext io_context, IOOp io_op, instr_time time)
+{
+ Assert(io_object < IOOBJECT_NUM_TYPES);
+ Assert(io_context < IOCONTEXT_NUM_TYPES);
+ Assert(io_op < IOOP_NUM_TYPES);
+ Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
+ Assert(pgstat_tracks_io_time(io_op) != -1);
+
+ INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op], time);
have_iostats = true;
}
@@ -114,8 +139,17 @@ pgstat_flush_io(bool nowait)
for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
{
for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
- bktype_shstats->data[io_object][io_context][io_op] +=
- PendingIOStats.data[io_object][io_context][io_op];
+ {
+ instr_time time;
+
+ bktype_shstats->counts[io_object][io_context][io_op] +=
+ PendingIOStats.counts[io_object][io_context][io_op];
+
+ time = PendingIOStats.pending_times[io_object][io_context][io_op];
+
+ bktype_shstats->times[io_object][io_context][io_op] +=
+ INSTR_TIME_GET_MICROSEC(time);
+ }
}
}
@@ -384,3 +418,30 @@ pgstat_tracks_io_op(BackendType bktype, IOObject io_object,
return true;
}
+
+/*
+ * PgStat_BktypeIO->times contains IO times for IOOps. For simplicity this
+ * array has a spot for every IOOp. pgstat_tracks_io_time() is the source of
+ * truth for which IOOps have corresponding IO times.
+ */
+IOOp
+pgstat_tracks_io_time(IOOp io_op)
+{
+ switch (io_op)
+ {
+ case IOOP_READ:
+ return IOOP_READ;
+ case IOOP_WRITE:
+ return IOOP_WRITE;
+ case IOOP_EXTEND:
+ return IOOP_EXTEND;
+ case IOOP_FSYNC:
+ return IOOP_FSYNC;
+ case IOOP_EVICT:
+ case IOOP_REUSE:
+ return -1;
+ }
+
+ elog(ERROR, "unrecognized IOOp value: %d", io_op);
+ pg_unreachable();
+}
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 35c6d465553..60fdda1a147 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1255,12 +1255,16 @@ typedef enum io_stat_col
IO_COL_IO_OBJECT,
IO_COL_IO_CONTEXT,
IO_COL_READS,
+ IO_COL_READ_TIME,
IO_COL_WRITES,
+ IO_COL_WRITE_TIME,
IO_COL_EXTENDS,
+ IO_COL_EXTEND_TIME,
IO_COL_CONVERSION,
IO_COL_EVICTIONS,
IO_COL_REUSES,
IO_COL_FSYNCS,
+ IO_COL_FSYNC_TIME,
IO_COL_RESET_TIME,
IO_NUM_COLUMNS,
} io_stat_col;
@@ -1292,6 +1296,28 @@ pgstat_get_io_op_index(IOOp io_op)
pg_unreachable();
}
+/*
+ * Get the number of the column containing IO times for the specified IOOp. If
+ * the specified IOOp is one for which IO time is not tracked, return -1. Note
+ * that this function assumes that IO time for an IOOp is displayed in the view
+ * in the column directly after the IOOp counts.
+ */
+static io_stat_col
+pgstat_get_io_time_index(IOOp io_op)
+{
+ if (pgstat_tracks_io_time(io_op) == -1)
+ return -1;
+
+ return pgstat_get_io_op_index(io_op) + 1;
+}
+
+static inline
+PgStat_Counter
+pg_stat_micro_to_millisecs(PgStat_Counter val_microsec)
+{
+ return val_microsec * 0.001;
+}
+
Datum
pg_stat_get_io(PG_FUNCTION_ARGS)
{
@@ -1359,20 +1385,32 @@ pg_stat_get_io(PG_FUNCTION_ARGS)
for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
{
- int col_idx = pgstat_get_io_op_index(io_op);
+ PgStat_Counter count = bktype_stats->counts[io_obj][io_context][io_op];
+ int i = pgstat_get_io_op_index(io_op);
/*
* Some combinations of BackendType and IOOp, of IOContext
* and IOOp, and of IOObject and IOOp are not tracked. Set
* these cells in the view NULL.
*/
- nulls[col_idx] = !pgstat_tracks_io_op(bktype, io_obj, io_context, io_op);
+ if (pgstat_tracks_io_op(bktype, io_obj, io_context, io_op))
+ values[i] = Int64GetDatum(count);
+ else
+ nulls[i] = true;
+ }
- if (nulls[col_idx])
+ for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
+ {
+ PgStat_Counter time = bktype_stats->times[io_obj][io_context][io_op];
+ int i = pgstat_get_io_time_index(io_op);
+
+ if (i == -1)
continue;
- values[col_idx] =
- Int64GetDatum(bktype_stats->data[io_obj][io_context][io_op]);
+ if (!nulls[pgstat_get_io_op_index(io_op)])
+ values[i] = Float8GetDatum(pg_stat_micro_to_millisecs(time));
+ else
+ nulls[i] = true;
}
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6249bb50d02..ad3667f2585 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3814,6 +3814,18 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</entry>
</row>
+ <row>
+ <entry role="catalog_table_entry">
+ <para role="column_definition">
+ <structfield>read_time</structfield> <type>double precision</type>
+ </para>
+ <para>
+ Time spent in read operations in milliseconds (if <xref
+ linkend="guc-track-io-timing"/> is enabled, otherwise zero)
+ </para>
+ </entry>
+ </row>
+
<row>
<entry role="catalog_table_entry">
<para role="column_definition">
@@ -3826,6 +3838,18 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</entry>
</row>
+ <row>
+ <entry role="catalog_table_entry">
+ <para role="column_definition">
+ <structfield>write_time</structfield> <type>double precision</type>
+ </para>
+ <para>
+ Time spent in write operations in milliseconds (if <xref
+ linkend="guc-track-io-timing"/> is enabled, otherwise zero)
+ </para>
+ </entry>
+ </row>
+
<row>
<entry role="catalog_table_entry">
<para role="column_definition">
@@ -3838,6 +3862,18 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</entry>
</row>
+ <row>
+ <entry role="catalog_table_entry">
+ <para role="column_definition">
+ <structfield>extend_time</structfield> <type>double precision</type>
+ </para>
+ <para>
+ Time spent in extend operations in milliseconds (if <xref
+ linkend="guc-track-io-timing"/> is enabled, otherwise zero)
+ </para>
+ </entry>
+ </row>
+
<row>
<entry role="catalog_table_entry">
<para role="column_definition">
@@ -3902,6 +3938,18 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</entry>
</row>
+ <row>
+ <entry role="catalog_table_entry">
+ <para role="column_definition">
+ <structfield>fsync_time</structfield> <type>double precision</type>
+ </para>
+ <para>
+ Time spent in fsync operations in milliseconds (if <xref
+ linkend="guc-track-io-timing"/> is enabled, otherwise zero)
+ </para>
+ </entry>
+ </row>
+
<row>
<entry role="catalog_table_entry">
<para role="column_definition">
@@ -3967,6 +4015,17 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</itemizedlist>
</para>
+ <note>
+ <para>
+ Columns tracking I/O time will only be non-zero when <xref
+ linkend="guc-track-io-timing"/> is enabled. The user should be careful when
+ using these columns in combination with their corresponding operations to
+ ensure that <varname>track_io_timing</varname> was enabled for the entire
+ time since the last reset.
+ </para>
+ </note>
+
+
</sect2>
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index e953d1f5159..54348513145 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1880,14 +1880,18 @@ pg_stat_io| SELECT backend_type,
io_object,
io_context,
reads,
+ read_time,
writes,
+ write_time,
extends,
+ extend_time,
op_bytes,
evictions,
reuses,
fsyncs,
+ fsync_time,
stats_reset
- FROM pg_stat_get_io() b(backend_type, io_object, io_context, reads, writes, extends, op_bytes, evictions, reuses, fsyncs, stats_reset);
+ FROM pg_stat_get_io() b(backend_type, io_object, io_context, reads, read_time, writes, write_time, extends, extend_time, op_bytes, evictions, reuses, fsyncs, fsync_time, stats_reset);
pg_stat_progress_analyze| SELECT s.pid,
s.datid,
d.datname,
--
2.38.0
--6bjylyod4nohcyqb
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0002-pgstat-io-increment-pgstat_io-hackery.patch"
^ permalink raw reply [nested|flat] 31+ messages in thread
end of thread, other threads:[~2023-03-06 15:41 UTC | newest]
Thread overview: 31+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-16 04:58 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]>
2022-03-02 06:57 ` Michael Paquier <[email protected]>
2022-03-02 08:07 ` Andres Freund <[email protected]>
2022-03-03 05:03 ` Michael Paquier <[email protected]>
2022-03-03 13:16 ` Andrew Dunstan <[email protected]>
2022-03-31 04:10 ` Michael Paquier <[email protected]>
2022-03-31 05:00 ` Tom Lane <[email protected]>
2022-03-31 05:25 ` Tom Lane <[email protected]>
2022-03-31 05:36 ` Andres Freund <[email protected]>
2022-03-31 07:56 ` Michael Paquier <[email protected]>
2022-03-31 13:49 ` Tom Lane <[email protected]>
2022-04-01 01:16 ` Michael Paquier <[email protected]>
2022-04-01 03:42 ` Noah Misch <[email protected]>
2022-04-01 03:51 ` Justin Pryzby <[email protected]>
2022-04-01 06:01 ` Michael Paquier <[email protected]>
2022-04-01 11:53 ` Michael Paquier <[email protected]>
2022-04-01 04:00 ` Michael Paquier <[email protected]>
2022-04-01 03:54 ` Michael Paquier <[email protected]>
2022-05-02 04:27 ` Noah Misch <[email protected]>
2022-05-02 07:00 ` Michael Paquier <[email protected]>
2022-05-09 03:18 ` Michael Paquier <[email protected]>
2022-05-11 01:29 ` Michael Paquier <[email protected]>
2022-05-11 05:32 ` Noah Misch <[email protected]>
2022-05-12 05:27 ` Michael Paquier <[email protected]>
2022-05-14 08:27 ` Noah Misch <[email protected]>
2022-05-16 05:30 ` Michael Paquier <[email protected]>
2022-05-18 08:03 ` Noah Misch <[email protected]>
2022-05-18 09:20 ` Michael Paquier <[email protected]>
2022-05-21 01:28 ` Noah Misch <[email protected]>
2022-05-21 03:03 ` Michael Paquier <[email protected]>
2023-03-06 15:41 [PATCH v6 1/2] Track IO times in pg_stat_io Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox