From: Jonathan S. Katz <jkatz@postgresql.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Noah Misch <noah@leadboat.com>
Cc: Andrew Dunstan <andrew@dunslane.net>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Shruthi Gowda <gowdashru@gmail.com>
Subject: Re: pg15b2: large objects lost on upgrade
Date: Tue, 2 Aug 2022 16:44:52 -0400
Message-ID: <286460a2-75ce-ab54-9da1-c817e7ae6f51@postgresql.org> (raw)
In-Reply-To: <06f74ad5-9e28-85ae-35ae-cbb6a2d756b8@postgresql.org>
References: <YuCQQtF2p3qWs7t+@momjian.us>
<3901843.1659116952@sss.pgh.pa.us>
<CA+TgmobzCRWG=_Tf0VXvF26pZbTxV-CKPNx6RdoWonKPETLvyQ@mail.gmail.com>
<CA+TgmoYKVbB9rt2UAvaFLG-qYCgRuhAr7wPLDwTT0VpbPKvj_w@mail.gmail.com>
<CA+TgmoZiBP2OD9POF0R2L=fib4ueOUusSVzmnO0_+kNzqSpnKw@mail.gmail.com>
<4152163.1659124804@sss.pgh.pa.us>
<CA+TgmoY=MfT2uc=N!4sAcHf3_teqYBNtmqVrVYv5oEK=4RfxATw@mail.gmail.com>
<CA+TgmoZX6Zz4KU2enOYyc+aQWA+S5ZaZRGo6BgxW7mNF4btqaw@mail.gmail.com>
<125444.1659136594@sss.pgh.pa.us>
<20220730054401.GB3644221@rfd.leadboat.com>
<351616.1659192034@sss.pgh.pa.us>
<635c8809-3525-7dae-a1a1-0d9069cd86bc@postgresql.org>
<2657259.1659460333@sss.pgh.pa.us>
<CA+TgmoastyZUAmJ6t953=yycQxu_S0x6A08QVARsbOVAWW=D2A@mail.gmail.com>
<c07d096c-a67a-9fb9-2001-7a97b36aa5a7@postgresql.org>
<2686054.1659469199@sss.pgh.pa.us>
<98286e9b-8fe6-554e-c23f-d67a3df2c30d@postgresql.org>
<2687956.1659469881@sss.pgh.pa.us>
<06f74ad5-9e28-85ae-35ae-cbb6a2d756b8@postgresql.org>
On 8/2/22 4:20 PM, Jonathan S. Katz wrote:
> On 8/2/22 3:51 PM, Tom Lane wrote:
>> "Jonathan S. Katz" <jkatz@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.
Please see attached patch that does the above. Tests pass on my local
environment (though I did not trigger autovacuum).
Jonathan
Attachments:pg_upgrade-test-gte-xid.patch (text/plain, 1.7K) downloaddiff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.plindex 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.
OpenPGP_signature (application/pgp-signature, 839B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jkatz@postgresql.org, tgl@sss.pgh.pa.us, robertmhaas@gmail.com, noah@leadboat.com, andrew@dunslane.net, bruce@momjian.us, michael@paquier.xyz, pryzby@telsasoft.com, gowdashru@gmail.com
Subject: Re: pg15b2: large objects lost on upgrade
In-Reply-To: <286460a2-75ce-ab54-9da1-c817e7ae6f51@postgresql.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox