Re: pg15b2: large objects lost on upgrade

[Jonathan S. Katz]

On 8/2/22 4:20 PM, Jonathan S. Katz wrote:

On 8/2/22 3:51 PM, Tom Lane wrote:

"Jonathan S. Katz" <jk...@postgresql.org> writes:

On 8/2/22 3:39 PM, Tom Lane wrote:

I am not in favor of disabling autovacuum in the test: ordinary
users are not going to do that while pg_upgrade'ing, so it'd make
the test less representative of real-world usage, which seems like
a bad idea.  We could either drop this particular check again, or
weaken it to allow new relfrozenxid >= old relfrozenxid, likewise
relminxid.

The test does look helpful and it would catch regressions. Loosely
quoting Robert on a different point upthread, we don't want to turn off
the alarm just because it's spuriously going off.
I think the weakened check is OK (and possibly mimics the real-world
where autovacuum runs), unless you see a major drawback to it?

I also think that ">=" is a sufficient requirement.  It'd be a
bit painful to test if we had to cope with potential XID wraparound,
but we know that these installations haven't been around nearly
long enough for that, so a plain ">=" test ought to be good enough.
(Replacing the simple "eq" code with something that can handle that
doesn't look like much fun, though.)

...if these systems are hitting XID wraparound, we have another issue to worry about. I started modifying the test to support this behavior, but thought that because 1. we want to ensure the OID is still equal and 2. in the examples you showed, both relfrozenxid or relminxid could increment, we may want to have the individual checks on each column. I may be able to conjure something up that does the above, but it's been a minute since I wrote anything in Perl.

Jonathan

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl 
b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 4cbc75644c..e2b739beff 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -226,6 +226,8 @@ ORDER BY c.oid::regclass::text
 EOM
 $horizon_query =~ s/\s+/ /g; # run it together on one line
 my $horizon1 = $oldnode->safe_psql('regression', $horizon_query);
+chomp($horizon1);
+my @horizon1_values = split(/\|/, $horizon1);
 
 # In a VPATH build, we'll be started in the source directory, but we want
 # to run pg_upgrade in the build directory so that any files generated finish
@@ -313,6 +315,8 @@ $newnode->command_ok(
 
 # And record the horizons from the upgraded cluster as well.
 my $horizon2 = $newnode->safe_psql('regression', $horizon_query);
+chomp($horizon2);
+my @horizon2_values = split(/\|/, $horizon2);
 
 # Compare the two dumps, there should be no differences.
 my $compare_res = compare("$tempdir/dump1.sql", "$tempdir/dump2.sql");
@@ -331,8 +335,13 @@ if ($compare_res != 0)
        print "=== EOF ===\n";
 }
 
-# Compare the horizons, there should be no differences.
-my $horizons_ok = $horizon1 eq $horizon2;
+# Compare the horizons. There should be no change in the OID, so we will test
+# for equality. However, because autovacuum may have run between the two
+# horizions, we will check if the updated horizon XIDs are greater than or
+# equal to the originals.
+my $horizons_ok = $horizon2_values[0] eq $horizon1_values[0] &&
+       int($horizon2_values[1]) >= int($horizon1_values[1]) &&
+       int($horizon2_values[2]) >= int($horizon1_values[2]);
 ok($horizons_ok, 'old and new horizons match after pg_upgrade');
 
 # Provide more context if the horizons do not match.

Please see attached patch that does the above. Tests pass on my local environment (though I did not trigger autovacuum).

OpenPGP_signature
Description: OpenPGP digital signature