Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«

Peter Geoghegan <pg@bowt.ie>

From: Peter Geoghegan <pg@bowt.ie>
To: Christoph Berg <myon@debian.org>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>, Teodor Sigaev <teodor@sigaev.ru>, Alexander Korotkov <a.korotkov@postgrespro.ru>
Date: 2019-07-17T23:10:56Z
Lists: pgsql-bugs
On Tue, Jul 16, 2019 at 1:13 AM Christoph Berg <myon@debian.org> wrote:
> I applied the patch to 12/HEAD and the Debian testsuite now passes the
> 10->12 upgrade test.

Thanks for testing!

I am not quite sure if I should push ahead with this, simply because I
don't know what the point of commit 0a64b45152b really was (Alexandar?
Teodor?). Why not just make the assertions a bit more less strict in
one or two places? Is the _bt_cachemetadata() function really
necessary? Can we remove it now?

AFAICT the only purpose that _bt_cachemetadata() serves that isn't
better handled by updating the assertions that were failing back in
April of 2018 (and still sometimes fail) is initializing the new-to-v3
fields defensively (initializing btm_oldest_btpo_xact and
btm_last_cleanup_num_heap_tuples are set to their default values).
Even that seems unnecessary, since every piece of code knows that it
isn't sensible to read those values. Including contrib/pageinspect,
which was actually taught this by commit 0a64b45152b itself.

I'm a bit nervous about pushing a commit that will almost be a
straight revert of 0a64b45152b without first getting confirmation that
_bt_cachemetadata() is actually totally unnecessary.

-- 
Peter Geoghegan



Commits

  1. Fix nbtree metapage cache upgrade bug.

  2. Make heap TID a tiebreaker nbtree index column.

  3. Fix handling of non-upgraded B-tree metapages

  4. Skip full index scan during cleanup of B-tree indexes when possible