From 621a99a666ba1a27b852dc5ddc0e1b224c388f53 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 4 Jun 2014 21:36:19 +0200 Subject: [PATCH] Fix longstanding bug in HeapTupleSatisfiesVacuum(). HeapTupleSatisfiesVacuum() didn't properly discern between DELETE_IN_PROGRESS and INSERT_IN_PROGRESS for rows that have been inserted in the current transaction and deleted in a aborted subtransaction of the current backend. At the very least that caused problems for CLUSTER and CREATE INDEX in transactions that had aborting subtransactions producing rows, leading to warnings like: WARNING: concurrent delete in progress within table "..." possibly in an endless, uninterruptible, loop. Instead of treating *InProgress xmins the same as *IsCurrent ones, treat them as being distinct like the other visibility routines. As implemented this separatation can cause a behaviour change for rows that have been inserted and deleted in another, still running, transaction. HTSV will now return INSERT_IN_PROGRESS instead of DELETE_IN_PROGRESS for those. That's both, more in line with the other visibility routines and arguably more correct. The latter because a INSERT_IN_PROGRESS will make callers look at/wait for xmin, instead of xmax. The only current caller where that's possibly worse than the old behaviour is heap_prune_chain() which now won't mark the page as prunable if a row has concurrently been inserted and deleted. That's harmless enough. As a cautionary measure also insert a interrupt check before the gotos in IndexBuildHeapScan() that lead to the uninterruptible loop. There are other possible causes, like a row that several sessions try to update and all fail, for repeated loops and the cost of doing so in the retry case is low. As this bug goes back all the way to the introduction of subtransactions in 573a71a5da backpatch to all supported releases. Reported-By: Sandro Santilli --- src/backend/catalog/index.c | 2 ++ src/backend/utils/time/tqual.c | 19 +++++++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 80acc0ec27..a5a204eb40 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2298,6 +2298,7 @@ IndexBuildHeapScan(Relation heapRelation, XactLockTableWait(xwait, heapRelation, &heapTuple->t_data->t_ctid, XLTW_InsertIndexUnique); + CHECK_FOR_INTERRUPTS(); goto recheck; } } @@ -2346,6 +2347,7 @@ IndexBuildHeapScan(Relation heapRelation, XactLockTableWait(xwait, heapRelation, &heapTuple->t_data->t_ctid, XLTW_InsertIndexUnique); + CHECK_FOR_INTERRUPTS(); goto recheck; } diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index 75cd53e1fd..96874ab80a 100644 --- a/src/backend/utils/time/tqual.c +++ b/src/backend/utils/time/tqual.c @@ -1166,7 +1166,7 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin, return HEAPTUPLE_DEAD; } } - else if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmin(tuple))) + else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple))) { if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid */ return HEAPTUPLE_INSERT_IN_PROGRESS; @@ -1175,7 +1175,22 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin, HeapTupleHeaderIsOnlyLocked(tuple)) return HEAPTUPLE_INSERT_IN_PROGRESS; /* inserted and then deleted by same xact */ - return HEAPTUPLE_DELETE_IN_PROGRESS; + if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetUpdateXid(tuple))) + return HEAPTUPLE_DELETE_IN_PROGRESS; + /* deleting subtransaction must have aborted */ + return HEAPTUPLE_INSERT_IN_PROGRESS; + } + else if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmin(tuple))) + { + /* + * It'd be possible to discern between INSERT/DELETE in progress + * here by looking at xmax - but that doesn't seem beneficial for + * the majority of callers and even detrimental for some. We'd + * rather have callers look at/wait for xmin than xmax. It's + * always correct to return INSERT_IN_PROGRESS because that's + * what's happening from the view of other backends. + */ + return HEAPTUPLE_INSERT_IN_PROGRESS; } else if (TransactionIdDidCommit(HeapTupleHeaderGetRawXmin(tuple))) SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED, -- 2.40.0