From f4e787c826446102eb38ffb5f398bea43b44cacf Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 1 Oct 2016 16:32:55 -0400 Subject: [PATCH] Fix bugs in contrib/pg_visibility. collect_corrupt_items() failed to initialize tuple.t_self. While HeapTupleSatisfiesVacuum() doesn't actually use that value, it does Assert that it's valid, so that the code would dump core if ip_posid chanced to be zero. (That's somewhat unlikely, which probably explains how this got missed. In any case it wouldn't matter for field use.) Also, collect_corrupt_items was returning the wrong TIDs, that is the contents of t_ctid rather than the tuple's own location. This would be the same thing in simple cases, but it could be wrong if, for example, a past update attempt had been rolled back, leaving a live tuple whose t_ctid doesn't point at itself. Also, in pg_visibility(), guard against trying to read a page past the end of the rel. The VM code handles inquiries beyond the end of the map by silently returning zeroes, and it seems like we should do the same thing here. I ran into the assertion failure while using pg_visibility to check pg_upgrade's behavior, and then noted the other problems while reading the code. Report: <29043.1475288648@sss.pgh.pa.us> --- contrib/pg_visibility/pg_visibility.c | 38 ++++++++++++++++++--------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index 7034066663..9985e3e3a1 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -3,6 +3,8 @@ * pg_visibility.c * display visibility map information and page-level visibility bits * + * Copyright (c) 2016, PostgreSQL Global Development Group + * * contrib/pg_visibility/pg_visibility.c *------------------------------------------------------------------------- */ @@ -54,6 +56,10 @@ static bool tuple_all_visible(HeapTuple tup, TransactionId OldestXmin, /* * Visibility map information for a single block of a relation. + * + * Note: the VM code will silently return zeroes for pages past the end + * of the map, so we allow probes up to MaxBlockNumber regardless of the + * actual relation size. */ Datum pg_visibility_map(PG_FUNCTION_ARGS) @@ -122,13 +128,22 @@ pg_visibility(PG_FUNCTION_ARGS) values[0] = BoolGetDatum((mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0); values[1] = BoolGetDatum((mapbits & VISIBILITYMAP_ALL_FROZEN) != 0); - buffer = ReadBuffer(rel, blkno); - LockBuffer(buffer, BUFFER_LOCK_SHARE); + /* Here we have to explicitly check rel size ... */ + if (blkno < RelationGetNumberOfBlocks(rel)) + { + buffer = ReadBuffer(rel, blkno); + LockBuffer(buffer, BUFFER_LOCK_SHARE); - page = BufferGetPage(buffer); - values[2] = BoolGetDatum(PageIsAllVisible(page)); + page = BufferGetPage(buffer); + values[2] = BoolGetDatum(PageIsAllVisible(page)); - UnlockReleaseBuffer(buffer); + UnlockReleaseBuffer(buffer); + } + else + { + /* As with the vismap, silently return 0 for pages past EOF */ + values[2] = BoolGetDatum(false); + } relation_close(rel, AccessShareLock); @@ -611,14 +626,13 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) /* Dead line pointers are neither all-visible nor frozen. */ if (ItemIdIsDead(itemid)) { - ItemPointerData tid; - - ItemPointerSet(&tid, blkno, offnum); - record_corrupt_item(items, &tid); + ItemPointerSet(&(tuple.t_self), blkno, offnum); + record_corrupt_item(items, &tuple.t_self); continue; } /* Initialize a HeapTupleData structure for checks below. */ + ItemPointerSet(&(tuple.t_self), blkno, offnum); tuple.t_data = (HeapTupleHeader) PageGetItem(page, itemid); tuple.t_len = ItemIdGetLength(itemid); tuple.t_tableOid = relid; @@ -649,12 +663,12 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) RecomputedOldestXmin = GetOldestXmin(NULL, true); if (!TransactionIdPrecedes(OldestXmin, RecomputedOldestXmin)) - record_corrupt_item(items, &tuple.t_data->t_ctid); + record_corrupt_item(items, &tuple.t_self); else { OldestXmin = RecomputedOldestXmin; if (!tuple_all_visible(&tuple, OldestXmin, buffer)) - record_corrupt_item(items, &tuple.t_data->t_ctid); + record_corrupt_item(items, &tuple.t_self); } } @@ -665,7 +679,7 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) if (check_frozen) { if (heap_tuple_needs_eventual_freeze(tuple.t_data)) - record_corrupt_item(items, &tuple.t_data->t_ctid); + record_corrupt_item(items, &tuple.t_self); } }