On Fri, Aug 2, 2019 at 4:56 PM Ashwin Agrawal <aagra...@pivotal.io> wrote:
>
> On Wed, Jul 31, 2019 at 2:06 PM Andres Freund <and...@anarazel.de> wrote:
>
>> Looking at the code as of master, we currently have:
>>
>
> Super awesome feedback and insights, thank you!
>
> - PredicateLockTuple() calls SubTransGetTopmostTransaction() to figure
>> out a whether the tuple has been locked by the current
>> transaction. That check afaict just should be
>> TransactionIdIsCurrentTransactionId(), without all the other
>> stuff that's done today.
>>
>
> Agree. v1-0002 patch attached does that now. Please let me know if that's
> what you meant.
>
> TransactionIdIsCurrentTransactionId() imo ought to be optimized to
>> always check for the top level transactionid first - that's a good bet
>> today, but even moreso for the upcoming AMs that won't have separate
>> xids for subtransactions. Alternatively we shouldn't make that a
>> binary search for each subtrans level, but just have a small
>> simplehash hashtable for xids.
>>
>
> v1-0001 patch checks for GetTopTransactionIdIfAny() first in
> TransactionIdIsCurrentTransactionId() which seems yes better in general and
> more for future. That mostly meets the needs for current discussion.
>
> The alternative of not using binary search seems bigger refactoring and
> should be handled as separate optimization exercise outside of this thread.
>
>
>> - CheckForSerializableConflictOut() wants to get the toplevel xid for
>> the tuple, because that's the one the predicate hashtable stores.
>>
>> In your patch you've already moved the HTSV() call etc out of
>> CheckForSerializableConflictOut(). I'm somewhat inclined to think that
>> the SubTransGetTopmostTransaction() call ought to go along with that.
>> I don't really think that belongs in predicate.c, especially if
>> most/all new AMs don't use subtransaction ids.
>>
>> The only downside is that currently the
>> TransactionIdEquals(xid, GetTopTransactionIdIfAny()) check
>> avoids the SubTransGetTopmostTransaction() check.
>>
>> But again, the better fix for that seems to be to improve the generic
>> code. As written the check won't prevent a subtrans lookup for heap
>> when subtransactions are in use, and it's IME pretty common for tuples
>> to get looked at again in the transaction that has created them. So
>> I'm somewhat inclined to think that SubTransGetTopmostTransaction()
>> should have a fast-path for the current transaction - probably just
>> employing TransactionIdIsCurrentTransactionId().
>>
>
> That optimization, as Kuntal also mentioned, seems something which can be
> done on-top afterwards on current patch.
>
>
>> I don't really see what we gain by having the subtrans handling in the
>> predicate code. Especially given that we've already moved the HTSV()
>> handling out, it seems architecturally the wrong place to me - but I
>> admit that that's a fuzzy argument. The relevant mapping should be one
>> line in the caller.
>>
>
> Okay, I moved the sub transaction handling out of
> CheckForSerializableConflictOut() and have it along side HTSV() now.
>
> The reason I felt leaving subtransaction handling in generic place, was it
> might be premature to thing no future AM will need it. Plus, all
> serializable function api's having same expectations is easier. Like
> PredicateLockTuple() can be passed top or subtransaction id and it can
> handle it but with the change CheckForSerializableConflictOut() only be
> feed top transaction ID. But its fine and can see the point of AM needing
> it can easily get top transaction ID and feed it as heap.
>
>
>> I wonder if it'd be wroth to combine the
>> TransactionIdIsCurrentTransactionId() calls in the heap cases that
>> currently do both, PredicateLockTuple() and
>> HeapCheckForSerializableConflictOut(). The heap_fetch() case probably
>> isn't commonly that hot a pathq, but heap_hot_search_buffer() is.
>>
>
> Maybe, will give thought to it separate from the current patch.
>
>
>> Minor notes:
>> - I don't think 'insert_xid' is necessarily great - it could also be the
>> updating xid etc. And while you can argue that an update is an insert
>> in the current heap, that's not the case for future AMs.
>> - to me
>> @@ -1621,7 +1622,8 @@ heap_hot_search_buffer(ItemPointer tid, Relation
>> relation, Buffer buffer,
>> if (valid)
>> {
>> ItemPointerSetOffsetNumber(tid, offnum);
>> - PredicateLockTuple(relation, heapTuple,
>> snapshot);
>> + PredicateLockTID(relation,
>> &(heapTuple)->t_self, snapshot,
>> +
>> HeapTupleHeaderGetXmin(heapTuple->t_data));
>> if (all_dead)
>> *all_dead = false;
>> return true;
>>
>> What are those parens - as placed they can't do anything. Did you
>> intend to write &(heapTuple->t_self)? Even that is pretty superfluous,
>> but it at least clarifies the precedence.
>>
>
> Fixed. No idea what I was thinking there, mostly feel I intended to have
> it as like &(heapTuple->t_self).
>
> I'm also a bit confused why we don't need to pass in the offset of the
>> current tuple, rather than the HOT root tuple here. That's not related
>> to this patch. But aren't we locking the wrong tuple here, in case of
>> HOT?
>>
>
> Yes, root is being locked here instead of the HOT. But I don't have full
> context on the same. If we wish to fix it though, can be easily done now
> with the patch by passing "tid" instead of &(heapTuple->t_self).
>
> - I wonder if CheckForSerializableConflictOutNeeded() shouldn't have a
>> portion of it's code as a static inline. In particular, it's a shame
>> that we currently perform external function calls at quite the
>> frequency when serializable isn't even in use.
>>
>
> I am not sure on portion of the code part? SerializationNeededForRead() is
> static inline function in C file. Can't inline
> CheckForSerializableConflictOutNeeded() without moving
> SerializationNeededForRead() and some other variables to header file.
> CheckForSerializableConflictOut() wasn't inline either, so a function call
> was performed earlier as well when serializable isn't even in use.
>
> I understand that with refactor, HeapCheckForSerializableConflictOut() is
> called which calls CheckForSerializableConflictOutNeeded(). If that's the
> problem, for addressing the same, I had proposed alternative way to
> refactor. CheckForSerializableConflictOut() can take callback function and
> void* callback argument for AM specific check instead. So, the flow would
> be AM calling CheckForSerializableConflictOut() as today and only if
> serializable in use will invoke the callback to check with AM if more work
> should be performed or not. Essentially
> HeapCheckForSerializableConflictOut() will become callback function
> instead. Due to void* callback argument aspect I didn't like that solution
> and felt AM performing checks and calling CheckForSerializableConflictOut()
> seems more straight forward.
>
Attaching re-based version of the patches on top of current master, which
has the fix for HOT serializable predicate locking bug spotted by Andres
committed now.
From aec03114cea4235265461f8c427f0633a24a7ec3 Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Fri, 2 Aug 2019 15:02:28 -0700
Subject: [PATCH v2 1/3] Optimize TransactionIdIsCurrentTransactionId()
If xid is current top transaction, can fast check for the same and
exit early. This should optmize for current heap but also works very
well for future AMs, which don't have separate xid for
subtransactions. This optimization was proposed by Andres.
---
src/backend/access/transam/xact.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 1bbaeeebf4d..41952bc4d26 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -871,6 +871,9 @@ TransactionIdIsCurrentTransactionId(TransactionId xid)
if (!TransactionIdIsNormal(xid))
return false;
+ if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
+ return true;
+
/*
* In parallel workers, the XIDs we must consider as current are stored in
* ParallelCurrentXids rather than the transaction-state stack. Note that
--
2.19.1
From 703d123ffd0a35d11e67b4cb07ceaeca1a527425 Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Fri, 2 Aug 2019 15:21:16 -0700
Subject: [PATCH v2 2/3] Optimize PredicateLockTuple().
PredicateLockTuple fast exits if tuple was written by current
transaction, as for that case it already has the lock. This check can
be easily performed using TransactionIdIsCurrentTransactionId()
instead of using SubTransGetTopmostTransaction(). Since
TransactionIdIsCurrentTransactionId() is all in-memory operation,
makes this efficient compared to SubTransGetTopmostTransaction(),
which can hit the disk.
This simplification was proposed by Andres.
---
src/backend/storage/lmgr/predicate.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 85a629f4fce..f4d8c2528a1 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -2559,24 +2559,10 @@ PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
*/
if (relation->rd_index == NULL)
{
- TransactionId myxid;
-
- targetxmin = HeapTupleHeaderGetXmin(tuple->t_data);
-
- myxid = GetTopTransactionIdIfAny();
- if (TransactionIdIsValid(myxid))
- {
- if (TransactionIdFollowsOrEquals(targetxmin, TransactionXmin))
- {
- TransactionId xid = SubTransGetTopmostTransaction(targetxmin);
-
- if (TransactionIdEquals(xid, myxid))
- {
- /* We wrote it; we already have a write lock. */
- return;
- }
- }
- }
+ /* If we wrote it; we already have a write lock. */
+ if (TransactionIdIsCurrentTransactionId(
+ HeapTupleHeaderGetXmin(tuple->t_data)))
+ return;
}
/*
--
2.19.1
From 3b3b90768afea0609513f473ff3d9cdd86ef74c1 Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Fri, 2 Aug 2019 11:44:21 -0700
Subject: [PATCH v2 3/3] Remove HeapTuple dependency for predicate locking
functions.
Following changes help to make predicate locking functions generic and
not tied to particular AM.
- PredicateLockTuple() is renamed to PredicateLockTID(). It takes
ItemPointer and inserting transaction id now instead of HeapTuple.
- CheckForSerializableConflictIn() takes blocknum instead of buffer
- CheckForSerializableConflictOut() no more takes HeapTuple nor buffer
---
src/backend/access/gin/ginbtree.c | 2 +-
src/backend/access/gin/ginfast.c | 2 +-
src/backend/access/gin/gininsert.c | 4 +-
src/backend/access/gist/gist.c | 2 +-
src/backend/access/hash/hashinsert.c | 2 +-
src/backend/access/heap/heapam.c | 119 ++++++++++++++++++---
src/backend/access/heap/heapam_handler.c | 7 +-
src/backend/access/index/indexam.c | 4 +-
src/backend/access/nbtree/nbtinsert.c | 4 +-
src/backend/storage/lmgr/predicate.c | 127 +++++++----------------
src/include/access/heapam.h | 2 +
src/include/storage/predicate.h | 9 +-
12 files changed, 164 insertions(+), 120 deletions(-)
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 4c29261256a..adeb1d2aa04 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -89,7 +89,7 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
stack->predictNumber = 1;
if (rootConflictCheck)
- CheckForSerializableConflictIn(btree->index, NULL, stack->buffer);
+ CheckForSerializableConflictIn(btree->index, NULL, btree->rootBlkno);
for (;;)
{
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index 439a91b3e61..d7b52476817 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -246,7 +246,7 @@ ginHeapTupleFastInsert(GinState *ginstate, GinTupleCollector *collector)
* tree, so it conflicts with all serializable scans. All scans acquire a
* predicate lock on the metabuffer to represent that.
*/
- CheckForSerializableConflictIn(index, NULL, metabuffer);
+ CheckForSerializableConflictIn(index, NULL, GIN_METAPAGE_BLKNO);
if (collector->sumsize + collector->ntuples * sizeof(ItemIdData) > GinListPageSize)
{
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index 55eab146173..046a20a3d41 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -221,7 +221,7 @@ ginEntryInsert(GinState *ginstate,
return;
}
- CheckForSerializableConflictIn(ginstate->index, NULL, stack->buffer);
+ CheckForSerializableConflictIn(ginstate->index, NULL, BufferGetBlockNumber(stack->buffer));
/* modify an existing leaf entry */
itup = addItemPointersToLeafTuple(ginstate, itup,
items, nitem, buildStats, stack->buffer);
@@ -230,7 +230,7 @@ ginEntryInsert(GinState *ginstate,
}
else
{
- CheckForSerializableConflictIn(ginstate->index, NULL, stack->buffer);
+ CheckForSerializableConflictIn(ginstate->index, NULL, BufferGetBlockNumber(stack->buffer));
/* no match, so construct a new leaf entry */
itup = buildFreshLeafTuple(ginstate, attnum, key, category,
items, nitem, buildStats, stack->buffer);
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index e9ca4b82527..4db54a42761 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -1263,7 +1263,7 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
* Check for any rw conflicts (in serializable isolation level) just
* before we intend to modify the page
*/
- CheckForSerializableConflictIn(state->r, NULL, stack->buffer);
+ CheckForSerializableConflictIn(state->r, NULL, BufferGetBlockNumber(stack->buffer));
/* Insert the tuple(s) to the page, splitting the page if necessary */
is_split = gistplacetopage(state->r, state->freespace, giststate,
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index 89876d2ccd0..c87fa5970e3 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -88,7 +88,7 @@ restart_insert:
&usedmetap);
Assert(usedmetap != NULL);
- CheckForSerializableConflictIn(rel, NULL, buf);
+ CheckForSerializableConflictIn(rel, NULL, BufferGetBlockNumber(buf));
/* remember the primary bucket buffer to release the pin on it at end. */
bucket_buf = buf;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 6b42bdc77b2..e23c57f9391 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -40,6 +40,7 @@
#include "access/multixact.h"
#include "access/parallel.h"
#include "access/relscan.h"
+#include "access/subtrans.h"
#include "access/sysattr.h"
#include "access/tableam.h"
#include "access/transam.h"
@@ -446,7 +447,7 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
else
valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
- CheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+ HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
&loctup, buffer, snapshot);
if (valid)
@@ -668,7 +669,7 @@ heapgettup(HeapScanDesc scan,
snapshot,
scan->rs_cbuf);
- CheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+ HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
tuple, scan->rs_cbuf,
snapshot);
@@ -1477,9 +1478,10 @@ heap_fetch(Relation relation,
valid = HeapTupleSatisfiesVisibility(tuple, snapshot, buffer);
if (valid)
- PredicateLockTuple(relation, tuple, snapshot);
+ PredicateLockTID(relation, &(tuple->t_self), snapshot,
+ HeapTupleHeaderGetXmin(tuple->t_data));
- CheckForSerializableConflictOut(valid, relation, tuple, buffer, snapshot);
+ HeapCheckForSerializableConflictOut(valid, relation, tuple, buffer, snapshot);
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
@@ -1610,13 +1612,14 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
{
/* If it's visible per the snapshot, we must return it */
valid = HeapTupleSatisfiesVisibility(heapTuple, snapshot, buffer);
- CheckForSerializableConflictOut(valid, relation, heapTuple,
- buffer, snapshot);
+ HeapCheckForSerializableConflictOut(valid, relation, heapTuple,
+ buffer, snapshot);
if (valid)
{
ItemPointerSetOffsetNumber(tid, offnum);
- PredicateLockTuple(relation, heapTuple, snapshot);
+ PredicateLockTID(relation, &heapTuple->t_self, snapshot,
+ HeapTupleHeaderGetXmin(heapTuple->t_data));
if (all_dead)
*all_dead = false;
return true;
@@ -1750,7 +1753,7 @@ heap_get_latest_tid(TableScanDesc sscan,
* candidate.
*/
valid = HeapTupleSatisfiesVisibility(&tp, snapshot, buffer);
- CheckForSerializableConflictOut(valid, relation, &tp, buffer, snapshot);
+ HeapCheckForSerializableConflictOut(valid, relation, &tp, buffer, snapshot);
if (valid)
*tid = ctid;
@@ -1905,7 +1908,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
* lock "gaps" as index page locks do. So we don't need to specify a
* buffer when making the call, which makes for a faster check.
*/
- CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
+ CheckForSerializableConflictIn(relation, NULL, InvalidBlockNumber);
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
@@ -2159,7 +2162,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
* lock "gaps" as index page locks do. So we don't need to specify a
* buffer when making the call, which makes for a faster check.
*/
- CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
+ CheckForSerializableConflictIn(relation, NULL, InvalidBlockNumber);
ndone = 0;
while (ndone < ntuples)
@@ -2350,7 +2353,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
* lock "gaps" as index page locks do. So we don't need to specify a
* buffer when making the call.
*/
- CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
+ CheckForSerializableConflictIn(relation, NULL, InvalidBlockNumber);
/*
* If tuples are cachable, mark them for invalidation from the caches in
@@ -2664,7 +2667,7 @@ l1:
* being visible to the scan (i.e., an exclusive buffer content lock is
* continuously held from this point until the tuple delete is visible).
*/
- CheckForSerializableConflictIn(relation, &tp, buffer);
+ CheckForSerializableConflictIn(relation, tid, BufferGetBlockNumber(buffer));
/* replace cid with a combo cid if necessary */
HeapTupleHeaderAdjustCmax(tp.t_data, &cid, &iscombo);
@@ -3579,7 +3582,7 @@ l2:
* will include checking the relation level, there is no benefit to a
* separate check for the new tuple.
*/
- CheckForSerializableConflictIn(relation, &oldtup, buffer);
+ CheckForSerializableConflictIn(relation, otid, BufferGetBlockNumber(buffer));
/*
* At this point newbuf and buffer are both pinned and locked, and newbuf
@@ -9039,3 +9042,93 @@ heap_mask(char *pagedata, BlockNumber blkno)
}
}
}
+
+/*
+ * HeapCheckForSerializableConflictOut
+ * We are reading a tuple which has been modified. If it is visible to
+ * us but has been deleted, that indicates a rw-conflict out. If it's
+ * not visible and was created by a concurrent (overlapping)
+ * serializable transaction, that is also a rw-conflict out,
+ *
+ * We will determine the top level xid of the writing transaction with which
+ * we may be in conflict, and check for overlap with our own transaction.
+ * If the transactions overlap (i.e., they cannot see each other's writes),
+ * then we have a conflict out.
+ *
+ * This function should be called just about anywhere in heapam.c where a
+ * tuple has been read. The caller must hold at least a shared lock on the
+ * buffer, because this function might set hint bits on the tuple. There is
+ * currently no known reason to call this function from an index AM.
+ */
+void
+HeapCheckForSerializableConflictOut(bool visible, Relation relation,
+ HeapTuple tuple, Buffer buffer,
+ Snapshot snapshot)
+{
+ TransactionId xid;
+ HTSV_Result htsvResult;
+
+ if (!CheckForSerializableConflictOutNeeded(relation, snapshot))
+ return;
+
+ /*
+ * Check to see whether the tuple has been written to by a concurrent
+ * transaction, either to create it not visible to us, or to delete it
+ * while it is visible to us. The "visible" bool indicates whether the
+ * tuple is visible to us, while HeapTupleSatisfiesVacuum checks what else
+ * is going on with it.
+ */
+ htsvResult = HeapTupleSatisfiesVacuum(tuple, TransactionXmin, buffer);
+ switch (htsvResult)
+ {
+ case HEAPTUPLE_LIVE:
+ if (visible)
+ return;
+ xid = HeapTupleHeaderGetXmin(tuple->t_data);
+ break;
+ case HEAPTUPLE_RECENTLY_DEAD:
+ if (!visible)
+ return;
+ xid = HeapTupleHeaderGetUpdateXid(tuple->t_data);
+ break;
+ case HEAPTUPLE_DELETE_IN_PROGRESS:
+ xid = HeapTupleHeaderGetUpdateXid(tuple->t_data);
+ break;
+ case HEAPTUPLE_INSERT_IN_PROGRESS:
+ xid = HeapTupleHeaderGetXmin(tuple->t_data);
+ break;
+ case HEAPTUPLE_DEAD:
+ return;
+ default:
+
+ /*
+ * The only way to get to this default clause is if a new value is
+ * added to the enum type without adding it to this switch
+ * statement. That's a bug, so elog.
+ */
+ elog(ERROR, "unrecognized return value from HeapTupleSatisfiesVacuum: %u", htsvResult);
+
+ /*
+ * In spite of having all enum values covered and calling elog on
+ * this default, some compilers think this is a code path which
+ * allows xid to be used below without initialization. Silence
+ * that warning.
+ */
+ xid = InvalidTransactionId;
+ }
+
+ Assert(TransactionIdIsValid(xid));
+ Assert(TransactionIdFollowsOrEquals(xid, TransactionXmin));
+
+ /*
+ * Find top level xid. Bail out if xid is too early to be a conflict, or
+ * if it's our own xid.
+ */
+ if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
+ return;
+ xid = SubTransGetTopmostTransaction(xid);
+ if (TransactionIdPrecedes(xid, TransactionXmin))
+ return;
+
+ return CheckForSerializableConflictOut(relation, xid, snapshot);
+}
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index f1ff01e8cb9..70ed8c8af32 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2166,9 +2166,10 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
if (valid)
{
hscan->rs_vistuples[ntup++] = offnum;
- PredicateLockTuple(scan->rs_rd, &loctup, snapshot);
+ PredicateLockTID(scan->rs_rd, &loctup.t_self, snapshot,
+ HeapTupleHeaderGetXmin(loctup.t_data));
}
- CheckForSerializableConflictOut(valid, scan->rs_rd, &loctup,
+ HeapCheckForSerializableConflictOut(valid, scan->rs_rd, &loctup,
buffer, snapshot);
}
}
@@ -2356,7 +2357,7 @@ heapam_scan_sample_next_tuple(TableScanDesc scan, SampleScanState *scanstate,
/* in pagemode, heapgetpage did this for us */
if (!pagemode)
- CheckForSerializableConflictOut(visible, scan->rs_rd, tuple,
+ HeapCheckForSerializableConflictOut(visible, scan->rs_rd, tuple,
hscan->rs_cbuf, scan->rs_snapshot);
/* Try next tuple from same page. */
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 28edd4aca76..6f1093ae383 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -180,8 +180,8 @@ index_insert(Relation indexRelation,
if (!(indexRelation->rd_indam->ampredlocks))
CheckForSerializableConflictIn(indexRelation,
- (HeapTuple) NULL,
- InvalidBuffer);
+ (ItemPointer) NULL,
+ InvalidBlockNumber);
return indexRelation->rd_indam->aminsert(indexRelation, values, isnull,
heap_t_ctid, heapRelation,
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 5890f393f67..6f73fb73638 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -290,7 +290,7 @@ top:
* checkingunique and !heapkeyspace cases, but it's okay to use the
* first page the value could be on (with scantid omitted) instead.
*/
- CheckForSerializableConflictIn(rel, NULL, insertstate.buf);
+ CheckForSerializableConflictIn(rel, NULL, BufferGetBlockNumber(insertstate.buf));
/*
* Do the insertion. Note that insertstate contains cached binary
@@ -533,7 +533,7 @@ _bt_check_unique(Relation rel, BTInsertState insertstate, Relation heapRel,
* otherwise be masked by this unique constraint
* violation.
*/
- CheckForSerializableConflictIn(rel, NULL, insertstate->buf);
+ CheckForSerializableConflictIn(rel, NULL, BufferGetBlockNumber(insertstate->buf));
/*
* This is a definite conflict. Break the tuple down into
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index f4d8c2528a1..4677363be93 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -163,8 +163,8 @@
* PredicateLockRelation(Relation relation, Snapshot snapshot)
* PredicateLockPage(Relation relation, BlockNumber blkno,
* Snapshot snapshot)
- * PredicateLockTuple(Relation relation, HeapTuple tuple,
- * Snapshot snapshot)
+ * PredicateLockTID(Relation relation, ItemPointer tid, Snapshot snapshot,
+ * TransactionId insert_xid)
* PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
* BlockNumber newblkno)
* PredicateLockPageCombine(Relation relation, BlockNumber oldblkno,
@@ -173,11 +173,10 @@
* ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
*
* conflict detection (may also trigger rollback)
- * CheckForSerializableConflictOut(bool visible, Relation relation,
- * HeapTupleData *tup, Buffer buffer,
+ * CheckForSerializableConflictOut(Relation relation, TransactionId xid,
* Snapshot snapshot)
- * CheckForSerializableConflictIn(Relation relation, HeapTupleData *tup,
- * Buffer buffer)
+ * CheckForSerializableConflictIn(Relation relation, ItemPointer tid,
+ * BlockNumber blkno)
* CheckTableForSerializableConflictIn(Relation relation)
*
* final rollback checking
@@ -193,8 +192,6 @@
#include "postgres.h"
-#include "access/heapam.h"
-#include "access/htup_details.h"
#include "access/parallel.h"
#include "access/slru.h"
#include "access/subtrans.h"
@@ -2538,30 +2535,28 @@ PredicateLockPage(Relation relation, BlockNumber blkno, Snapshot snapshot)
}
/*
- * PredicateLockTuple
+ * PredicateLockTID
*
* Gets a predicate lock at the tuple level.
* Skip if not in full serializable transaction isolation level.
* Skip if this is a temporary table.
*/
void
-PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
+PredicateLockTID(Relation relation, ItemPointer tid, Snapshot snapshot,
+ TransactionId tuple_xid)
{
PREDICATELOCKTARGETTAG tag;
- ItemPointer tid;
- TransactionId targetxmin;
if (!SerializationNeededForRead(relation, snapshot))
return;
/*
- * If it's a heap tuple, return if this xact wrote it.
+ * Return if this xact wrote it.
*/
if (relation->rd_index == NULL)
{
/* If we wrote it; we already have a write lock. */
- if (TransactionIdIsCurrentTransactionId(
- HeapTupleHeaderGetXmin(tuple->t_data)))
+ if (TransactionIdIsCurrentTransactionId(tuple_xid))
return;
}
@@ -2577,7 +2572,6 @@ PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
if (PredicateLockExists(&tag))
return;
- tid = &(tuple->t_self);
SET_PREDICATELOCKTARGETTAG_TUPLE(tag,
relation->rd_node.dbNode,
relation->rd_id,
@@ -4022,33 +4016,43 @@ XidIsConcurrent(TransactionId xid)
return false;
}
+bool
+CheckForSerializableConflictOutNeeded(Relation relation, Snapshot snapshot)
+{
+ if (!SerializationNeededForRead(relation, snapshot))
+ return false;
+
+ /* Check if someone else has already decided that we need to die */
+ if (SxactIsDoomed(MySerializableXact))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("could not serialize access due to read/write dependencies among transactions"),
+ errdetail_internal("Reason code: Canceled on identification as a pivot, during conflict out checking."),
+ errhint("The transaction might succeed if retried.")));
+ }
+
+ return true;
+}
+
/*
* CheckForSerializableConflictOut
* We are reading a tuple which has been modified. If it is visible to
* us but has been deleted, that indicates a rw-conflict out. If it's
* not visible and was created by a concurrent (overlapping)
- * serializable transaction, that is also a rw-conflict out,
+ * serializable transaction, that is also a rw-conflict out.
*
* We will determine the top level xid of the writing transaction with which
* we may be in conflict, and check for overlap with our own transaction.
* If the transactions overlap (i.e., they cannot see each other's writes),
* then we have a conflict out.
- *
- * This function should be called just about anywhere in heapam.c where a
- * tuple has been read. The caller must hold at least a shared lock on the
- * buffer, because this function might set hint bits on the tuple. There is
- * currently no known reason to call this function from an index AM.
*/
void
-CheckForSerializableConflictOut(bool visible, Relation relation,
- HeapTuple tuple, Buffer buffer,
- Snapshot snapshot)
+CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot snapshot)
{
- TransactionId xid;
SERIALIZABLEXIDTAG sxidtag;
SERIALIZABLEXID *sxid;
SERIALIZABLEXACT *sxact;
- HTSV_Result htsvResult;
if (!SerializationNeededForRead(relation, snapshot))
return;
@@ -4062,64 +4066,8 @@ CheckForSerializableConflictOut(bool visible, Relation relation,
errdetail_internal("Reason code: Canceled on identification as a pivot, during conflict out checking."),
errhint("The transaction might succeed if retried.")));
}
-
- /*
- * Check to see whether the tuple has been written to by a concurrent
- * transaction, either to create it not visible to us, or to delete it
- * while it is visible to us. The "visible" bool indicates whether the
- * tuple is visible to us, while HeapTupleSatisfiesVacuum checks what else
- * is going on with it.
- */
- htsvResult = HeapTupleSatisfiesVacuum(tuple, TransactionXmin, buffer);
- switch (htsvResult)
- {
- case HEAPTUPLE_LIVE:
- if (visible)
- return;
- xid = HeapTupleHeaderGetXmin(tuple->t_data);
- break;
- case HEAPTUPLE_RECENTLY_DEAD:
- if (!visible)
- return;
- xid = HeapTupleHeaderGetUpdateXid(tuple->t_data);
- break;
- case HEAPTUPLE_DELETE_IN_PROGRESS:
- xid = HeapTupleHeaderGetUpdateXid(tuple->t_data);
- break;
- case HEAPTUPLE_INSERT_IN_PROGRESS:
- xid = HeapTupleHeaderGetXmin(tuple->t_data);
- break;
- case HEAPTUPLE_DEAD:
- return;
- default:
-
- /*
- * The only way to get to this default clause is if a new value is
- * added to the enum type without adding it to this switch
- * statement. That's a bug, so elog.
- */
- elog(ERROR, "unrecognized return value from HeapTupleSatisfiesVacuum: %u", htsvResult);
-
- /*
- * In spite of having all enum values covered and calling elog on
- * this default, some compilers think this is a code path which
- * allows xid to be used below without initialization. Silence
- * that warning.
- */
- xid = InvalidTransactionId;
- }
Assert(TransactionIdIsValid(xid));
- Assert(TransactionIdFollowsOrEquals(xid, TransactionXmin));
- /*
- * Find top level xid. Bail out if xid is too early to be a conflict, or
- * if it's our own xid.
- */
- if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
- return;
- xid = SubTransGetTopmostTransaction(xid);
- if (TransactionIdPrecedes(xid, TransactionXmin))
- return;
if (TransactionIdEquals(xid, GetTopTransactionIdIfAny()))
return;
@@ -4425,8 +4373,7 @@ CheckTargetForConflictsIn(PREDICATELOCKTARGETTAG *targettag)
* tuple itself.
*/
void
-CheckForSerializableConflictIn(Relation relation, HeapTuple tuple,
- Buffer buffer)
+CheckForSerializableConflictIn(Relation relation, ItemPointer tid, BlockNumber blkno)
{
PREDICATELOCKTARGETTAG targettag;
@@ -4456,22 +4403,22 @@ CheckForSerializableConflictIn(Relation relation, HeapTuple tuple,
* It is not possible to take and hold a lock across the checks for all
* granularities because each target could be in a separate partition.
*/
- if (tuple != NULL)
+ if (tid != NULL)
{
SET_PREDICATELOCKTARGETTAG_TUPLE(targettag,
relation->rd_node.dbNode,
relation->rd_id,
- ItemPointerGetBlockNumber(&(tuple->t_self)),
- ItemPointerGetOffsetNumber(&(tuple->t_self)));
+ ItemPointerGetBlockNumber(tid),
+ ItemPointerGetOffsetNumber(tid));
CheckTargetForConflictsIn(&targettag);
}
- if (BufferIsValid(buffer))
+ if (blkno != InvalidBlockNumber)
{
SET_PREDICATELOCKTARGETTAG_PAGE(targettag,
relation->rd_node.dbNode,
relation->rd_id,
- BufferGetBlockNumber(buffer));
+ blkno);
CheckTargetForConflictsIn(&targettag);
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 858bcb6bc96..390023c0a55 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -217,5 +217,7 @@ extern bool ResolveCminCmaxDuringDecoding(struct HTAB *tuplecid_data,
HeapTuple htup,
Buffer buffer,
CommandId *cmin, CommandId *cmax);
+extern void HeapCheckForSerializableConflictOut(bool valid, Relation relation, HeapTuple tuple,
+ Buffer buffer, Snapshot snapshot);
#endif /* HEAPAM_H */
diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h
index 376245ecd70..7e1f67b8edb 100644
--- a/src/include/storage/predicate.h
+++ b/src/include/storage/predicate.h
@@ -57,16 +57,17 @@ extern void SetSerializableTransactionSnapshot(Snapshot snapshot,
extern void RegisterPredicateLockingXid(TransactionId xid);
extern void PredicateLockRelation(Relation relation, Snapshot snapshot);
extern void PredicateLockPage(Relation relation, BlockNumber blkno, Snapshot snapshot);
-extern void PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot);
+extern void PredicateLockTID(Relation relation, ItemPointer tid, Snapshot snapshot,
+ TransactionId insert_xid);
extern void PredicateLockPageSplit(Relation relation, BlockNumber oldblkno, BlockNumber newblkno);
extern void PredicateLockPageCombine(Relation relation, BlockNumber oldblkno, BlockNumber newblkno);
extern void TransferPredicateLocksToHeapRelation(Relation relation);
extern void ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe);
/* conflict detection (may also trigger rollback) */
-extern void CheckForSerializableConflictOut(bool valid, Relation relation, HeapTuple tuple,
- Buffer buffer, Snapshot snapshot);
-extern void CheckForSerializableConflictIn(Relation relation, HeapTuple tuple, Buffer buffer);
+extern bool CheckForSerializableConflictOutNeeded(Relation relation, Snapshot snapshot);
+extern void CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot snapshot);
+extern void CheckForSerializableConflictIn(Relation relation, ItemPointer tid, BlockNumber blkno);
extern void CheckTableForSerializableConflictIn(Relation relation);
/* final rollback checking */
--
2.19.1
