Heikki Linnakangas <hlinnakan...@vmware.com> wrote:
> So I don't think you can ever get a false conflict because of
> slot reuse.
I spent some time looking at this, and I now agree.
> And if there's a hole in that thinking I can't see right now, the
> worst that will happen is some unnecessary conflicts, ie. it's
> still correct.
That is definitely true; no doubt about that part.
> Summary: IMHO we should just remove xmin from the predicate lock
> tag.
I spent some time trying to see how the vacuum could happen at a
point that could cause a false positive, and was unable to do so.
Even if that is just a failure of imagination on my part, the above
argument that it doesn't produce incorrect results still holds. I
think the fact that I couldn't find a sequence of events which
would trigger a false positive suggests it would be a rare case,
anyway.
I found the original bug report which led to the addition of xmin
to the tag, and it was because we were still carrying tuple locks
forward to new versions of those locks at the time. This was later
proven to be unnecessary, which simplified other code; we
apparently missed a trick in not removing xmin from the lock tag at
that point. Since leaving it there has now been shown to *cause* a
bug, I'm inclined to agree that we should simply remove it, and
backpatch that.
Patch attached. Any objections to applying that Real Soon Now?
(When, exactly is the deadline to make today's minor release
cut-off?)
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
***************
*** 156,164 ****
* PredicateLockTuple(Relation relation, HeapTuple tuple,
* Snapshot snapshot)
* PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno);
* PredicateLockPageCombine(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno);
* TransferPredicateLocksToHeapRelation(Relation relation)
* ReleasePredicateLocks(bool isCommit)
*
--- 156,164 ----
* PredicateLockTuple(Relation relation, HeapTuple tuple,
* Snapshot snapshot)
* PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno)
* PredicateLockPageCombine(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno)
* TransferPredicateLocksToHeapRelation(Relation relation)
* ReleasePredicateLocks(bool isCommit)
*
***************
*** 381,387 **** static SHM_QUEUE *FinishedSerializableTransactions;
* this entry, you can ensure that there's enough scratch space available for
* inserting one entry in the hash table. This is an otherwise-invalid tag.
*/
! static const PREDICATELOCKTARGETTAG ScratchTargetTag = {0, 0, 0, 0, 0};
static uint32 ScratchTargetTagHash;
static int ScratchPartitionLock;
--- 381,387 ----
* this entry, you can ensure that there's enough scratch space available for
* inserting one entry in the hash table. This is an otherwise-invalid tag.
*/
! static const PREDICATELOCKTARGETTAG ScratchTargetTag = {0, 0, 0, 0};
static uint32 ScratchTargetTagHash;
static int ScratchPartitionLock;
***************
*** 2492,2499 **** PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
}
}
}
- else
- targetxmin = InvalidTransactionId;
/*
* Do quick-but-not-definitive test for a relation lock first. This will
--- 2492,2497 ----
***************
*** 2512,2519 **** PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
relation->rd_node.dbNode,
relation->rd_id,
ItemPointerGetBlockNumber(tid),
! ItemPointerGetOffsetNumber(tid),
! targetxmin);
PredicateLockAcquire(&tag);
}
--- 2510,2516 ----
relation->rd_node.dbNode,
relation->rd_id,
ItemPointerGetBlockNumber(tid),
! ItemPointerGetOffsetNumber(tid));
PredicateLockAcquire(&tag);
}
***************
*** 4283,4290 **** CheckForSerializableConflictIn(Relation relation, HeapTuple tuple,
relation->rd_node.dbNode,
relation->rd_id,
ItemPointerGetBlockNumber(&(tuple->t_data->t_ctid)),
! ItemPointerGetOffsetNumber(&(tuple->t_data->t_ctid)),
! HeapTupleHeaderGetXmin(tuple->t_data));
CheckTargetForConflictsIn(&targettag);
}
--- 4280,4286 ----
relation->rd_node.dbNode,
relation->rd_id,
ItemPointerGetBlockNumber(&(tuple->t_data->t_ctid)),
! ItemPointerGetOffsetNumber(&(tuple->t_data->t_ctid)));
CheckTargetForConflictsIn(&targettag);
}
*** a/src/include/storage/predicate.h
--- b/src/include/storage/predicate.h
***************
*** 52,57 **** extern void PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snap
--- 52,58 ----
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 ReleasePredicateTupleLocks(Relation relation, BlockNumber blkno, OffsetNumber offset);
extern void ReleasePredicateLocks(bool isCommit);
/* conflict detection (may also trigger rollback) */
*** a/src/include/storage/predicate_internals.h
--- b/src/include/storage/predicate_internals.h
***************
*** 254,263 **** typedef struct SERIALIZABLEXID
* be the target of predicate locks.
*
* Note that the hash function being used doesn't properly respect tag
! * length -- it will go to a four byte boundary past the end of the tag.
! * If you change this struct, make sure any slack space is initialized,
! * so that any random bytes in the middle or at the end are not included
! * in the hash.
*
* TODO SSI: If we always use the same fields for the same type of value, we
* should rename these. Holding off until it's clear there are no exceptions.
--- 254,263 ----
* be the target of predicate locks.
*
* Note that the hash function being used doesn't properly respect tag
! * length -- if the length of the structure isn't a multiple of four bytes it
! * will go to a four byte boundary past the end of the tag. If you change
! * this struct, make sure any slack space is initialized, so that any random
! * bytes in the middle or at the end are not included in the hash.
*
* TODO SSI: If we always use the same fields for the same type of value, we
* should rename these. Holding off until it's clear there are no exceptions.
***************
*** 272,278 **** typedef struct PREDICATELOCKTARGETTAG
uint32 locktag_field2; /* a 32-bit ID field */
uint32 locktag_field3; /* a 32-bit ID field */
uint32 locktag_field4; /* a 32-bit ID field */
- uint32 locktag_field5; /* a 32-bit ID field */
} PREDICATELOCKTARGETTAG;
/*
--- 272,277 ----
***************
*** 283,294 **** typedef struct PREDICATELOCKTARGETTAG
* added when a predicate lock is requested on an object which doesn't
* already have one. An entry is removed when the last lock is removed from
* its list.
- *
- * Because a particular target might become obsolete, due to update to a new
- * version, before the reading transaction is obsolete, we need some way to
- * prevent errors from reuse of a tuple ID. Rather than attempting to clean
- * up the targets as the related tuples are pruned or vacuumed, we check the
- * xmin on access. This should be far less costly.
*/
typedef struct PREDICATELOCKTARGET
{
--- 282,287 ----
***************
*** 398,419 **** typedef struct PredicateLockData
((locktag).locktag_field1 = (dboid), \
(locktag).locktag_field2 = (reloid), \
(locktag).locktag_field3 = InvalidBlockNumber, \
! (locktag).locktag_field4 = InvalidOffsetNumber, \
! (locktag).locktag_field5 = InvalidTransactionId)
#define SET_PREDICATELOCKTARGETTAG_PAGE(locktag,dboid,reloid,blocknum) \
((locktag).locktag_field1 = (dboid), \
(locktag).locktag_field2 = (reloid), \
(locktag).locktag_field3 = (blocknum), \
! (locktag).locktag_field4 = InvalidOffsetNumber, \
! (locktag).locktag_field5 = InvalidTransactionId)
! #define SET_PREDICATELOCKTARGETTAG_TUPLE(locktag,dboid,reloid,blocknum,offnum,xmin) \
((locktag).locktag_field1 = (dboid), \
(locktag).locktag_field2 = (reloid), \
(locktag).locktag_field3 = (blocknum), \
! (locktag).locktag_field4 = (offnum), \
! (locktag).locktag_field5 = (xmin))
#define GET_PREDICATELOCKTARGETTAG_DB(locktag) \
((Oid) (locktag).locktag_field1)
--- 391,409 ----
((locktag).locktag_field1 = (dboid), \
(locktag).locktag_field2 = (reloid), \
(locktag).locktag_field3 = InvalidBlockNumber, \
! (locktag).locktag_field4 = InvalidOffsetNumber)
#define SET_PREDICATELOCKTARGETTAG_PAGE(locktag,dboid,reloid,blocknum) \
((locktag).locktag_field1 = (dboid), \
(locktag).locktag_field2 = (reloid), \
(locktag).locktag_field3 = (blocknum), \
! (locktag).locktag_field4 = InvalidOffsetNumber)
! #define SET_PREDICATELOCKTARGETTAG_TUPLE(locktag,dboid,reloid,blocknum,offnum) \
((locktag).locktag_field1 = (dboid), \
(locktag).locktag_field2 = (reloid), \
(locktag).locktag_field3 = (blocknum), \
! (locktag).locktag_field4 = (offnum))
#define GET_PREDICATELOCKTARGETTAG_DB(locktag) \
((Oid) (locktag).locktag_field1)
***************
*** 423,430 **** typedef struct PredicateLockData
((BlockNumber) (locktag).locktag_field3)
#define GET_PREDICATELOCKTARGETTAG_OFFSET(locktag) \
((OffsetNumber) (locktag).locktag_field4)
- #define GET_PREDICATELOCKTARGETTAG_XMIN(locktag) \
- ((TransactionId) (locktag).locktag_field5)
#define GET_PREDICATELOCKTARGETTAG_TYPE(locktag) \
(((locktag).locktag_field4 != InvalidOffsetNumber) ? PREDLOCKTAG_TUPLE : \
(((locktag).locktag_field3 != InvalidBlockNumber) ? PREDLOCKTAG_PAGE : \
--- 413,418 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
