Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

On 5/14/21 10:26 PM, Michael Paquier wrote:

Hi,
(Adding Andrew in CC for the buildfarm and PostgresNode parts.)

$subject has been around for a couple of years now, with the following
threads:
/messages/by-id/20180126080026.GI17847@paquier.xyz
/messages/by-id/CAB7nPqRdaN1A1YNjxNL9T1jUEWct8ttqq29dNv8W_o37+e8wfA@mail.gmail.com

An advantage of moving to TAP is that we can then remove the support
for upgrades within the MSVC scripts, and also remove pg_upgrade's
test.sh that has accumulated tweaks that are solved by the TAP tests,
resulting in cleanup:
8 files changed, 230 insertions(+), 403 deletions(-)

Based on the past discussions, there were two obstacles preventing to
do this switch:
- Support for tests with older versions, something where the gap as
been closed thanks to Andrew's work in 4c4eaf3d.
- Buildfarm support, and I am not sure how things need to be extended
there.

Another thing to note is that HEAD uses oldbindir, bindir and libdir
to track the location of the old and new libraries and binaries. With
the infrastructure in place, once can define only an install path for
a PostgresNode, so this version uses only two variables:
- oldinstall, for the installation path of the version to upgrade
from.
- oldsrc, to point to the source of the old version.

It is not difficult to switch to one approach or the other, but
reducing the logic to a minimum number of variables is a good deal to
take IMO.

I have been testing this patch a bit with older versions, down to 12,
and that was logically working, and PostgresNode may need more to be
able to work with ~11, as documented in get_new_node(). And I have
not tested that with MSVC yet. Anyway, attached is a new patch to
make the discussion move on. Even if there is still work to be done
here, would people here still support this switch?

PostgresNode is currently able to create nodes suitable for upgrade down
to release 10. If we add '-w' to the 'pg_ctl start' flags that can
extend down to release 9.5. (Just tested) I think we should do that
forthwith. '-w' is now the default, but having it there explicitly does
no harm.

If people are interested in what's incompatible on older versions, they
can look at
<https://gitlab.com/adunstan/postgresnodeng/-/blob/master/PostgresNode.pm&gt;
starting at about line 2764.

I don't think this will work, though, unless there is enough data to
exercise pg_upgrade fairly thoroughly. The approach taken by both
test.sh and (somewhat more comprehensively) by the buildfarm cross
version upgrade module is to test a cluster where the regression tests
have been run. That might be more difficult when testing against older
versions, so I have published a snapshot of the dumps of each of the
versions we tests against in the buildfarm animal crake. These could be
loaded into PostgresNode instances and then an upgrade attempted. See
<https://gitlab.com/adunstan/pg-old-bin/-/tree/master/data&gt;. The data
goes back to 9.2. These compressed dumps are a couple of megabytes each,
not huge.

cheers

andrew

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

On Sat, May 15, 2021 at 02:22:24PM -0400, Andrew Dunstan wrote:

PostgresNode is currently able to create nodes suitable for upgrade down
to release 10. If we add '-w' to the 'pg_ctl start' flags that can
extend down to release 9.5. (Just tested) I think we should do that
forthwith. '-w' is now the default, but having it there explicitly does
no harm.

Agreed. When testing manually, I have personally never worked on any
patches that required binaries older than 9.4, so I would be fine if
the TAP tests are able to work easily down to 9.5, even if pg_upgrade
is supported down to 8.4.

If people are interested in what's incompatible on older versions, they
can look at
<https://gitlab.com/adunstan/postgresnodeng/-/blob/master/PostgresNode.pm&gt;
starting at about line 2764.

We should really have adjust_conf() at some point in the in-core
module.

I don't think this will work, though, unless there is enough data to
exercise pg_upgrade fairly thoroughly. The approach taken by both
test.sh and (somewhat more comprehensively) by the buildfarm cross
version upgrade module is to test a cluster where the regression tests
have been run.

Yeah, that's what my patch is doing with pg_regress, FWIW. This
requires regress.so from the old version.

That might be more difficult when testing against older
versions, so I have published a snapshot of the dumps of each of the
versions we tests against in the buildfarm animal crake. These could be
loaded into PostgresNode instances and then an upgrade attempted. See
<https://gitlab.com/adunstan/pg-old-bin/-/tree/master/data&gt;. The data
goes back to 9.2. These compressed dumps are a couple of megabytes each,
not huge.

I agree that this can be simpler in some cases. In your experience,
how much of an issue is it when it becomes necessary to keep around
binaries that rely on libraries older than what a system can support?
It is easy to counter issues in this area with OpenSSL and
non-necessary things, but we had in the past also cases where we had
code that conflicted with the kernel, aka 3e68686.

At the end of this exercise, what I think we should achieve is to:
1) Reduce the diff between the buildfarm code and the in-core code.
2) Get rid of test.sh.
3) Be able to run easily upgrade tests across major versions for
developers.

As of now, 3) requires some extra facilities depending on if this is
done by the buildfarm or the in-core tests:
1) Path to the source code of the old version. This is required once
it becomes necessary to find out src/test/regress/ for the schedule,
the tests to run and its regress.so. There is no need to do that if
you have a dump of the old instance.
2) Path to a custom dump to replace the run with pg_regress from 1).
3) Location of the old binaries, for pg_upgrade. When it comes to
PostgresNode, we require an install path, so we cannot use directly
the location of the binaries.

Looking at the code of the buildfarm, its code does something smarter
than what my patch or HEAD's test.sh does now, as these require the
path for the old source. The buildfarm code first scans for the
probin's used in the regression database and then updates any
references. What test.sh and my patch do is using the path to the old
source code and run a single UPDATE. The approach taken by the
buildfarm code is more portable, as a path to the old source code
becomes necessary only if running pg_regress manually. So, what about
doing the following thing?
1) Update the TAP test so as probin entries are updated in the same way
as the buildfarm.
2) Allow one to specify a path to a custom dump, or a path to the old
source code for pg_regress.

If we do that, then it should be possible to reduce the code footprint
in the buildfarm code, while still allowing people to test major
upgrades in the same old-fashioned way, right? That's assuming that
PostgresNode is made compatible down to 9.2, of course, as a first
step, as that's the range of the dumps you are keeping around for the
buildfarm.

Thoughts?
--
Michael

On 5/16/21 9:55 PM, Michael Paquier wrote:

On Sat, May 15, 2021 at 02:22:24PM -0400, Andrew Dunstan wrote:

PostgresNode is currently able to create nodes suitable for upgrade down
to release 10. If we add '-w' to the 'pg_ctl start' flags that can
extend down to release 9.5. (Just tested) I think we should do that
forthwith. '-w' is now the default, but having it there explicitly does
no harm.

Agreed. When testing manually, I have personally never worked on any
patches that required binaries older than 9.4, so I would be fine if
the TAP tests are able to work easily down to 9.5, even if pg_upgrade
is supported down to 8.4.

If people are interested in what's incompatible on older versions, they
can look at
<https://gitlab.com/adunstan/postgresnodeng/-/blob/master/PostgresNode.pm&gt;
starting at about line 2764.

We should really have adjust_conf() at some point in the in-core
module.

Yes, I'm going to be proposing a series of smallish patches including
these when the tree is branched (which I hope will be in a few weeks).

I don't think this will work, though, unless there is enough data to
exercise pg_upgrade fairly thoroughly. The approach taken by both
test.sh and (somewhat more comprehensively) by the buildfarm cross
version upgrade module is to test a cluster where the regression tests
have been run.

Yeah, that's what my patch is doing with pg_regress, FWIW. This
requires regress.so from the old version.

That might be more difficult when testing against older
versions, so I have published a snapshot of the dumps of each of the
versions we tests against in the buildfarm animal crake. These could be
loaded into PostgresNode instances and then an upgrade attempted. See
<https://gitlab.com/adunstan/pg-old-bin/-/tree/master/data&gt;. The data
goes back to 9.2. These compressed dumps are a couple of megabytes each,
not huge.

I agree that this can be simpler in some cases. In your experience,
how much of an issue is it when it becomes necessary to keep around
binaries that rely on libraries older than what a system can support?
It is easy to counter issues in this area with OpenSSL and
non-necessary things, but we had in the past also cases where we had
code that conflicted with the kernel, aka 3e68686.

That one at least isn't an issue. Old versions of postgres didn't have
pg_rewind.

At the end of this exercise, what I think we should achieve is to:
1) Reduce the diff between the buildfarm code and the in-core code.
2) Get rid of test.sh.
3) Be able to run easily upgrade tests across major versions for
developers.

As of now, 3) requires some extra facilities depending on if this is
done by the buildfarm or the in-core tests:
1) Path to the source code of the old version. This is required once
it becomes necessary to find out src/test/regress/ for the schedule,
the tests to run and its regress.so. There is no need to do that if
you have a dump of the old instance.
2) Path to a custom dump to replace the run with pg_regress from 1).
3) Location of the old binaries, for pg_upgrade. When it comes to
PostgresNode, we require an install path, so we cannot use directly
the location of the binaries.

Looking at the code of the buildfarm, its code does something smarter
than what my patch or HEAD's test.sh does now, as these require the
path for the old source. The buildfarm code first scans for the
probin's used in the regression database and then updates any
references. What test.sh and my patch do is using the path to the old
source code and run a single UPDATE. The approach taken by the
buildfarm code is more portable, as a path to the old source code
becomes necessary only if running pg_regress manually. So, what about
doing the following thing?
1) Update the TAP test so as probin entries are updated in the same way
as the buildfarm.
2) Allow one to specify a path to a custom dump, or a path to the old
source code for pg_regress.

If we do that, then it should be possible to reduce the code footprint
in the buildfarm code, while still allowing people to test major
upgrades in the same old-fashioned way, right? That's assuming that
PostgresNode is made compatible down to 9.2, of course, as a first
step, as that's the range of the dumps you are keeping around for the
buildfarm.

I'm intending to add some older dumps. -) But for now 9.2 is a good target.

cheers

andrew

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

On Mon, May 17, 2021 at 12:32:13PM -0400, Andrew Dunstan wrote:

On 5/16/21 9:55 PM, Michael Paquier wrote:
Yes, I'm going to be proposing a series of smallish patches including
these when the tree is branched (which I hope will be in a few weeks).

Thanks! That clearly needs to happen first. I'll help reviewing
these.

If we do that, then it should be possible to reduce the code footprint
in the buildfarm code, while still allowing people to test major
upgrades in the same old-fashioned way, right? That's assuming that
PostgresNode is made compatible down to 9.2, of course, as a first
step, as that's the range of the dumps you are keeping around for the
buildfarm.

I'm intending to add some older dumps. -) But for now 9.2 is a good target.

Makes sense. For now, I'll update this patch set so as it is possible
to use custom dumps, as an option in parallel of pg_regress when
specifying a different source code path. I'll also decouple the
business with probin updates and stick with the approach used by the
buildfarm code.
--
Michael

On Tue, May 18, 2021 at 10:49:39AM +0900, Michael Paquier wrote:

Makes sense. For now, I'll update this patch set so as it is possible
to use custom dumps, as an option in parallel of pg_regress when
specifying a different source code path. I'll also decouple the
business with probin updates and stick with the approach used by the
buildfarm code.

This has proved to not be that difficult. With the updated version
attached, pg_upgrade has two modes to set up the old instance used for
the upgrade with older binaries:
- With oldsrc and oldinstall set, pg_regress gets used, same way as
HEAD.
- With olddump and oldinstall set, an old dump is loaded instead in
the old instance before launching the upgrade.

oldsrc and olddump are exclusive options. Similarly to HEAD, the
dumps taken from the old and new instances generate diffs that can be
inspected manually. The updates of probin are done without any
dependencies to the source path of the old instance, copying from the
buildfarm.

While on it, I have fixed a couple of things that exist in test.sh but
were not reflected in this new script:
- Handling of postfix operations with ~13 clusters.
- Handling oldstyle_length for ~9.6 clusters.
- Handling of EXTRA_REGRESS_OPT.

This stuff still needs to be expanded depending on how PostgresNode is
made backward-compatible, but I'll wait for that to happen before
going further down here. I have also spent some time testing all that
with MSVC, and the installation paths used for pg_regress&co make the
script a tad more confusing, so I have dropped this part for now.

Thanks,
--
Michael

Hello Michael,

This patch needs the update from 201a76183 -- the function `get_new_node`
no longer exists.

Running check tests in the pg_upgrade folder fails for this reason.

Thank you,
Rachel

On Tue, Sep 7, 2021 at 2:06 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Sep 07, 2021 at 02:43:15PM -0700, Rachel Heaton wrote:

Running check tests in the pg_upgrade folder fails for this reason.

Thanks, rebased as attached. Andrew has posted another patch set that
completely reworks the shape of the modules by moving them into a
dedicated namespace, meaning that this is going to break again. I'll
see about that when we reach this point.
--
Michael

On Thu, May 20, 2021 at 03:07:56PM +0900, Michael Paquier wrote:

This stuff still needs to be expanded depending on how PostgresNode is
made backward-compatible, but I'll wait for that to happen before
going further down here. I have also spent some time testing all that
with MSVC, and the installation paths used for pg_regress&co make the
script a tad more confusing, so I have dropped this part for now.

Andrew, as this is a bit tied to the buildfarm code and any
simplifications that could happen there, do you have any comments
and/or suggestions for this patch?

This still applies on HEAD and it holds all the properties of the
existing test by using PostgresNodes that point to older installations
for the business with binaries and libraries business. There is one
part where pg_upgrade logs into src/test/regress/, which is not good,
but that should be easily fixable.
--
Michael

On 10/1/21 2:19 AM, Michael Paquier wrote:

On Thu, May 20, 2021 at 03:07:56PM +0900, Michael Paquier wrote:

This stuff still needs to be expanded depending on how PostgresNode is
made backward-compatible, but I'll wait for that to happen before
going further down here. I have also spent some time testing all that
with MSVC, and the installation paths used for pg_regress&co make the
script a tad more confusing, so I have dropped this part for now.

Andrew, as this is a bit tied to the buildfarm code and any
simplifications that could happen there, do you have any comments
and/or suggestions for this patch?

I haven't looked at the patch closely yet, but from a buildfarm POV I
think the only thing that needs to be done is to inhibit the buildfarm
client module if the TAP tests are present. The buildfarm code that runs
TAP tests should automatically detect and run the new test.

I've just counted and there are 116 animals reporting check-pg_upgrade,
so we'd better put that out pronto. It's a little early but I'll try to
push out a release containing code for it on Monday or Tuesday (it's a
one line addition).

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

I haven't looked at the patch closely yet, but from a buildfarm POV I
think the only thing that needs to be done is to inhibit the buildfarm
client module if the TAP tests are present. The buildfarm code that runs
TAP tests should automatically detect and run the new test.

I've just counted and there are 116 animals reporting check-pg_upgrade,
so we'd better put that out pronto. It's a little early but I'll try to
push out a release containing code for it on Monday or Tuesday (it's a
one line addition).

IIUC, the only problem for a non-updated animal would be that it'd
run the test twice? Or would it actually fail? If the latter,
we'd need to sit on the patch rather longer.

regards, tom lane

On 10/2/21 5:03 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I haven't looked at the patch closely yet, but from a buildfarm POV I
think the only thing that needs to be done is to inhibit the buildfarm
client module if the TAP tests are present. The buildfarm code that runs
TAP tests should automatically detect and run the new test.
I've just counted and there are 116 animals reporting check-pg_upgrade,
so we'd better put that out pronto. It's a little early but I'll try to
push out a release containing code for it on Monday or Tuesday (it's a
one line addition).

IIUC, the only problem for a non-updated animal would be that it'd
run the test twice? Or would it actually fail? If the latter,
we'd need to sit on the patch rather longer.

The patch removes test.sh, so yes it would break.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/2/21 5:03 PM, Tom Lane wrote:

IIUC, the only problem for a non-updated animal would be that it'd
run the test twice? Or would it actually fail? If the latter,
we'd need to sit on the patch rather longer.

The patch removes test.sh, so yes it would break.

Maybe we could leave test.sh in place for awhile? I'd rather
not cause a flag day for buildfarm owners. (Also, how do we
see this working in the back branches?)

regards, tom lane

On Sat, Oct 02, 2021 at 11:34:38PM -0400, Tom Lane wrote:

Maybe we could leave test.sh in place for awhile? I'd rather
not cause a flag day for buildfarm owners. (Also, how do we
see this working in the back branches?)

I would be fine with test.sh staying around for now.

If we do that, though, I think that we had better remove the support
for upgrades across different major versions in test.sh, and keep this
capability in the new script. I am not sure that a lot of people use
that to begin with, but it would be weird to support that with a
different configuration layer for both at the same time (test.sh uses
a combination of bin/ and lib/ paths, while TAP uses just installation
path to accomodate with what PostgresNode.pm is able to do). The
patch of this thread also adds support for the load of an old dump
instead of an installcheck run of the old instance, which is something
the buildfarm could use.

I also looked two days ago at a proposal to move all the
pg_upgrade-specific SQLs into a new, separate, file that makes use of
psql's \if to do the job encoded now in test.sh. I think that it
would be strange to duplicate this logic in a the pg_upgrade TAP test
and test.sh if we finish by keeping both around for now. So that's a
second item we had better deal with first, in my opinion:
/messages/by-id/YVa/se5gxr1PsXDy@paquier.xyz

Thoughts?
--
Michael

On 10/2/21 11:34 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/2/21 5:03 PM, Tom Lane wrote:

IIUC, the only problem for a non-updated animal would be that it'd
run the test twice? Or would it actually fail? If the latter,
we'd need to sit on the patch rather longer.

The patch removes test.sh, so yes it would break.

Maybe we could leave test.sh in place for awhile? I'd rather
not cause a flag day for buildfarm owners. (Also, how do we
see this working in the back branches?)

Actually, I was wrong. The module just does "make check" for non-MSVC.
For MSVC it calls vcregress.pl, which the patch doesn't touch (it
should, I think).

cheers

andrew

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

On 03.10.21 09:03, Michael Paquier wrote:

On Sat, Oct 02, 2021 at 11:34:38PM -0400, Tom Lane wrote:

Maybe we could leave test.sh in place for awhile? I'd rather
not cause a flag day for buildfarm owners. (Also, how do we
see this working in the back branches?)

I would be fine with test.sh staying around for now.

test.sh could be changed to invoke the TAP test.

On Sun, Oct 10, 2021 at 04:07:43PM +0200, Peter Eisentraut wrote:

On 03.10.21 09:03, Michael Paquier wrote:

On Sat, Oct 02, 2021 at 11:34:38PM -0400, Tom Lane wrote:

Maybe we could leave test.sh in place for awhile? I'd rather
not cause a flag day for buildfarm owners. (Also, how do we
see this working in the back branches?)

I would be fine with test.sh staying around for now.

test.sh could be changed to invoke the TAP test.

That would remove the possibility to run the tests of pg_upgrade with
--enable-tap-tests, which is the point I think Tom was making, because
TestUpgrade.pm in the buildfarm code just uses "make check" as of the
following:
$cmd = "cd $self->{pgsql}/src/bin/pg_upgrade && $make $instflags check";
--
Michael

On 10/10/21 10:07 AM, Peter Eisentraut wrote:

On 03.10.21 09:03, Michael Paquier wrote:

On Sat, Oct 02, 2021 at 11:34:38PM -0400, Tom Lane wrote:

Maybe we could leave test.sh in place for awhile?� I'd rather
not cause a flag day for buildfarm owners.� (Also, how do we
see this working in the back branches?)

I would be fine with test.sh staying around for now.

test.sh could be changed to invoke the TAP test.

Keeping test.sh is not necessary - I mis-remembered what the test module
does.

cheers

andrew

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

On Sun, Oct 03, 2021 at 08:22:57AM -0400, Andrew Dunstan wrote:

Actually, I was wrong. The module just does "make check" for non-MSVC.
For MSVC it calls vcregress.pl, which the patch doesn't touch (it
should, I think).

Yes, it should. And I'd like to do things so as we replace all the
internals of upgradecheck() by a call to tap_check(). The patch does
not work yet properly with MSVC, and there were some problems in
getting the invocation of pg_regress right as far as I recall. That's
why I have left this part for now. I don't see why we could not do
the MSVC part as an independent step though, getting rid of test.sh is
appealing enough in itself.
--
Michael

On Mon, Oct 11, 2021 at 09:04:47AM -0400, Andrew Dunstan wrote:

Keeping test.sh is not necessary - I mis-remembered what the test module
does.

So.. Are people fine to remove entirely test.sh at the end, requiring
the tests of pg_upgrade to have TAP installed? I'd rather raise the
bar here, as it would keep the code simpler in the tree in the long
term. Or am I misunderstanding something?
--
Michael