Re: BUG #17245: Index corruption involving deduplicated entries

Hi,

On 2021-11-02 19:03:05 -0700, Peter Geoghegan wrote:
> Attached patch adds ERRORs in the event of detecting
> index-tuple-TID-points-to-LP_UNUSED conditions, as well as other
> similar conditions -- not just assertions, as before.
>
> I do think that this would be a good idea on HEAD. Still have
> reservations about doing that for 14, but you're welcome to try and
> change my mind.

I'm not quite sure either - I'm worried about followon corruption based on the
bug. That'll be much more likely be detected with something like this in
place. But it's also easy to get some subtlety wrong...

> +static inline void
> +index_delete_check_htid(TM_IndexDeleteOp *delstate,
> + Page page, OffsetNumber maxoff,
> + ItemPointer htid, TM_IndexStatus *istatus)
> +{
> + OffsetNumber indexpagehoffnum = ItemPointerGetOffsetNumber(htid);
> + ItemId iid;
> + HeapTupleHeader htup;
> +
> + Assert(OffsetNumberIsValid(istatus->idxoffnum));
> +
> + if (indexpagehoffnum > maxoff)
> + ereport(ERROR,
> + (errcode(ERRCODE_INDEX_CORRUPTED),
> + errmsg_internal("heap tid from index tuple (%u,%u) points past end of heap page line pointer array at offset %u of block %u in index \"%s\"",
> + ItemPointerGetBlockNumber(htid),
> + indexpagehoffnum,
> + istatus->idxoffnum, delstate->iblknum,
> + RelationGetRelationName(delstate->irel))));
> + iid = PageGetItemId(page, indexpagehoffnum);
> + if (!ItemIdIsUsed(iid))
> + ereport(ERROR,
> + (errcode(ERRCODE_INDEX_CORRUPTED),
> + errmsg_internal("heap tid from index tuple (%u,%u) points to unused heap page item at offset %u of block %u in index \"%s\"",
> + ItemPointerGetBlockNumber(htid),
> + indexpagehoffnum,
> + istatus->idxoffnum, delstate->iblknum,
> + RelationGetRelationName(delstate->irel))));
> +
> + if (ItemIdHasStorage(iid))
> + {
> + Assert(ItemIdIsNormal(iid));
> + htup = (HeapTupleHeader) PageGetItem(page, iid);
> +
> + if (HeapTupleHeaderIsHeapOnly(htup))
> + ereport(ERROR,
> + (errcode(ERRCODE_INDEX_CORRUPTED),
> + errmsg_internal("heap tid from index tuple (%u,%u) points to heap-only tuple at offset %u of block %u in index \"%s\"",
> + ItemPointerGetBlockNumber(htid),
> + indexpagehoffnum,
> + istatus->idxoffnum, delstate->iblknum,
> + RelationGetRelationName(delstate->irel))));
> + }

I'd make the error paths unlikely().

> diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
> index db6912e9f..43549be04 100644
> --- a/src/backend/access/heap/pruneheap.c
> +++ b/src/backend/access/heap/pruneheap.c
> @@ -844,39 +844,115 @@ heap_page_prune_execute(Buffer buffer,
> {
> Page page = (Page) BufferGetPage(buffer);
> OffsetNumber *offnum;
> - int i;
> + HeapTupleHeader htup PG_USED_FOR_ASSERTS_ONLY;
>
> /* Shouldn't be called unless there's something to do */
> Assert(nredirected > 0 || ndead > 0 || nunused > 0);
>
> /* Update all redirected line pointers */
> offnum = redirected;
> - for (i = 0; i < nredirected; i++)
> + for (int i = 0; i < nredirected; i++)
> {
> OffsetNumber fromoff = *offnum++;
> OffsetNumber tooff = *offnum++;
> ItemId fromlp = PageGetItemId(page, fromoff);
> + ItemId tolp PG_USED_FOR_ASSERTS_ONLY;
> +
> +#ifdef USE_ASSERT_CHECKING

These I'd definitely only do in HEAD.

> + /*
> + * The existing items that we set as redirects must be the first tuple
> + * of a HOT chain that has not be pruned before now (can't be a
> + * heap-only tuple) when target item has tuple storage. When the item
> + * has no storage, then we must just be maintaining the LP_REDIRECT of
> + * a HOT chain that has been pruned at least once before now.
> + */
> + if (!ItemIdIsRedirected(fromlp))
> + {
> + Assert(ItemIdHasStorage(fromlp) && ItemIdIsNormal(fromlp));
> +
> + htup = (HeapTupleHeader) PageGetItem(page, fromlp);
> + Assert(!HeapTupleHeaderIsHeapOnly(htup));
> + }

I was a bit confused because I initially read this this as the first tuple
redirect *to* can't be a heap-only tuple. Perhaps this could be rephrased a
bit ("redirecting item"?).

Greetings,

Andres Freund