Horton

Re: pg15b2: large objects lost on upgrade

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Jonathan S. Katz" <jkatz@postgresql.org>, Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>, Noah Misch <noah@leadboat.com>, Andrew Dunstan <andrew@dunslane.net>, Bruce Momjian <bruce@momjian.us>, Michael Paquier <michael@paquier.xyz>, Justin Pryzby <pryzby@telsasoft.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, Shruthi Gowda <gowdashru@gmail.com>
Date: 2022-08-04T14:08:02Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Revert recent changes to 002_pg_upgrade.pl.

  2. Revise test case added in 43746996399541ecb5c7b188725a5f097c15ceae.

  3. Use TRUNCATE to preserve relfilenode for pg_largeobject + index.

  4. Preserve relfilenode of pg_largeobject and its index across pg_upgrade.

  5. Have VACUUM warn on relfrozenxid "in the future".

  6. Set relfrozenxid to oldest extant XID seen by VACUUM.

  7. pg_upgrade: Preserve relfilenodes and tablespace OIDs.

  8. Perform a lot more sanity checks when freezing tuples.

Attachments

On Wed, Aug 3, 2022 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Jonathan S. Katz" <jkatz@postgresql.org> writes:
> > I did rule out wanting to do the "xid + $X" check after reviewing some
> > of the output. I think that both $X could end up varying, and it really
> > feels like a bandaid.
>
> It is that.  I wouldn't feel comfortable with $X less than 100 or so,
> which is probably sloppy enough to draw Robert's ire.  Still, realizing
> that what we want right now is a band-aid for 15beta3, I don't think
> it's an unreasonable short-term option.

100 << 2^32, so it's not terrible, but I'm honestly coming around to
the view that we ought to just nuke this test case.

From my point of view, the assertion that disabling autovacuum during
this test case would make the test case useless seems to be incorrect.
The original purpose of the test was to make sure that the pre-upgrade
schema matched the post-upgrade schema. If having autovacuum running
or not running affects that, we have a serious problem, but this test
case isn't especially likely to find it, because whether autovacuum
runs or not during the brief window where the test is running is
totally unpredictable. Furthermore, if we do have such a problem, it
would probably indicate that vacuum is using the wrong horizons to
prune or test the visibility of the tuples. To find that out, we might
want to compare values upon which the behavior of vacuum might depend,
like relfrozenxid. But to do that, we have to disable autovacuum, so
that the value can't change under us. From my point of view, that's
making test coverage better, not worse, because any bugs in this area
that can be found without explicit testing of relevant horizons are
dependent on low-probability race conditions materializing in the
buildfarm. If we disable autovacuum and then compare relfrozenxid and
whatever else we care about explicitly, we can find bugs in that
category reliably.

However, if people don't accept that argument, then this new test case
is kind of silly. It's not the worst idea in the world to use a
threshold of 100 XIDs or something, but without disabling autovacuum,
we're basically comparing two things that can't be expected to be
equal, so we test and see if they're approximately equal and then call
that good enough. I don't know that I believe we'll ever find a bug
that way, though.

-- 
Robert Haas
EDB: http://www.enterprisedb.com