Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit bfa2ab5

Browse files
committed
Fix torn-page, unlogged xid and further risks from heap_update().
When heap_update needs to look for a page for the new tuple version, because the current one doesn't have sufficient free space, or when columns have to be processed by the tuple toaster, it has to release the lock on the old page during that. Otherwise there'd be lock ordering and lock nesting issues. To avoid concurrent sessions from trying to update / delete / lock the tuple while the page's content lock is released, the tuple's xmax is set to the current session's xid. That unfortunately was done without any WAL logging, thereby violating the rule that no XIDs may appear on disk, without an according WAL record. If the database were to crash / fail over when the page level lock is released, and some activity lead to the page being written out to disk, the xid could end up being reused; potentially leading to the row becoming invisible. There might be additional risks by not having t_ctid point at the tuple itself, without having set the appropriate lock infomask fields. To fix, compute the appropriate xmax/infomask combination for locking the tuple, and perform WAL logging using the existing XLOG_HEAP_LOCK record. That allows the fix to be backpatched. This issue has existed for a long time. There appears to have been partial attempts at preventing dangers, but these never have fully been implemented, and were removed a long time ago, in 1191916 (cf. HEAP_XMAX_UNLOGGED). In master / 9.6, there's an additional issue, namely that the visibilitymap's freeze bit isn't reset at that point yet. Since that's a new issue, introduced only in a892234, that'll be fixed in a separate commit. Author: Masahiko Sawada and Andres Freund Reported-By: Different aspects by Thomas Munro, Noah Misch, and others Discussion: CAEepm=3fWAbWryVW9swHyLTY4sXVf0xbLvXqOwUoDiNCx9mBjQ@mail.gmail.com Backpatch: 9.1/all supported versions
1 parent a4d357b commit bfa2ab5

File tree

1 file changed

+73
-23
lines changed

1 file changed

+73
-23
lines changed

src/backend/access/heap/heapam.c

Lines changed: 73 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3455,8 +3455,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
34553455
newbuf,
34563456
vmbuffer = InvalidBuffer,
34573457
vmbuffer_new = InvalidBuffer;
3458-
bool need_toast,
3459-
already_marked;
3458+
bool need_toast;
34603459
Size newtupsize,
34613460
pagefree;
34623461
bool have_tuple_lock = false;
@@ -3898,8 +3897,8 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
38983897
* on the same page as the old, then we need to release the content lock
38993898
* (but not the pin!) on the old tuple's buffer while we are off doing
39003899
* TOAST and/or table-file-extension work. We must mark the old tuple to
3901-
* show that it's already being updated, else other processes may try to
3902-
* update it themselves.
3900+
* show that it's locked, else other processes may try to update it
3901+
* themselves.
39033902
*
39043903
* We need to invoke the toaster if there are already any out-of-line
39053904
* toasted values present, or if the new tuple is over-threshold.
@@ -3923,19 +3922,73 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
39233922

39243923
if (need_toast || newtupsize > pagefree)
39253924
{
3925+
TransactionId xmax_lock_old_tuple;
3926+
uint16 infomask_lock_old_tuple,
3927+
infomask2_lock_old_tuple;
3928+
3929+
/*
3930+
* To prevent concurrent sessions from updating the tuple, we have to
3931+
* temporarily mark it locked, while we release the lock.
3932+
*
3933+
* To satisfy the rule that any xid potentially appearing in a buffer
3934+
* written out to disk, we unfortunately have to WAL log this
3935+
* temporary modification. We can reuse xl_heap_lock for this
3936+
* purpose. If we crash/error before following through with the
3937+
* actual update, xmax will be of an aborted transaction, allowing
3938+
* other sessions to proceed.
3939+
*/
3940+
3941+
/*
3942+
* Compute xmax / infomask appropriate for locking the tuple. This has
3943+
* to be done separately from the lock, because the potentially
3944+
* created multixact would otherwise be wrong.
3945+
*/
3946+
compute_new_xmax_infomask(HeapTupleHeaderGetRawXmax(oldtup.t_data),
3947+
oldtup.t_data->t_infomask,
3948+
oldtup.t_data->t_infomask2,
3949+
xid, *lockmode, false,
3950+
&xmax_lock_old_tuple, &infomask_lock_old_tuple,
3951+
&infomask2_lock_old_tuple);
3952+
3953+
Assert(HEAP_XMAX_IS_LOCKED_ONLY(infomask_lock_old_tuple));
3954+
3955+
START_CRIT_SECTION();
3956+
39263957
/* Clear obsolete visibility flags ... */
39273958
oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
39283959
oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
39293960
HeapTupleClearHotUpdated(&oldtup);
39303961
/* ... and store info about transaction updating this tuple */
3931-
Assert(TransactionIdIsValid(xmax_old_tuple));
3932-
HeapTupleHeaderSetXmax(oldtup.t_data, xmax_old_tuple);
3933-
oldtup.t_data->t_infomask |= infomask_old_tuple;
3934-
oldtup.t_data->t_infomask2 |= infomask2_old_tuple;
3962+
Assert(TransactionIdIsValid(xmax_lock_old_tuple));
3963+
HeapTupleHeaderSetXmax(oldtup.t_data, xmax_lock_old_tuple);
3964+
oldtup.t_data->t_infomask |= infomask_lock_old_tuple;
3965+
oldtup.t_data->t_infomask2 |= infomask2_lock_old_tuple;
39353966
HeapTupleHeaderSetCmax(oldtup.t_data, cid, iscombo);
3936-
/* temporarily make it look not-updated */
3967+
3968+
/* temporarily make it look not-updated, but locked */
39373969
oldtup.t_data->t_ctid = oldtup.t_self;
3938-
already_marked = true;
3970+
3971+
MarkBufferDirty(buffer);
3972+
3973+
if (RelationNeedsWAL(relation))
3974+
{
3975+
xl_heap_lock xlrec;
3976+
XLogRecPtr recptr;
3977+
3978+
XLogBeginInsert();
3979+
XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
3980+
3981+
xlrec.offnum = ItemPointerGetOffsetNumber(&oldtup.t_self);
3982+
xlrec.locking_xid = xmax_lock_old_tuple;
3983+
xlrec.infobits_set = compute_infobits(oldtup.t_data->t_infomask,
3984+
oldtup.t_data->t_infomask2);
3985+
XLogRegisterData((char *) &xlrec, SizeOfHeapLock);
3986+
recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_LOCK);
3987+
PageSetLSN(page, recptr);
3988+
}
3989+
3990+
END_CRIT_SECTION();
3991+
39393992
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
39403993

39413994
/*
@@ -4006,7 +4059,6 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
40064059
else
40074060
{
40084061
/* No TOAST work needed, and it'll fit on same page */
4009-
already_marked = false;
40104062
newbuf = buffer;
40114063
heaptup = newtup;
40124064
}
@@ -4093,18 +4145,16 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
40934145

40944146
RelationPutHeapTuple(relation, newbuf, heaptup, false); /* insert new tuple */
40954147

4096-
if (!already_marked)
4097-
{
4098-
/* Clear obsolete visibility flags ... */
4099-
oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
4100-
oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
4101-
/* ... and store info about transaction updating this tuple */
4102-
Assert(TransactionIdIsValid(xmax_old_tuple));
4103-
HeapTupleHeaderSetXmax(oldtup.t_data, xmax_old_tuple);
4104-
oldtup.t_data->t_infomask |= infomask_old_tuple;
4105-
oldtup.t_data->t_infomask2 |= infomask2_old_tuple;
4106-
HeapTupleHeaderSetCmax(oldtup.t_data, cid, iscombo);
4107-
}
4148+
4149+
/* Clear obsolete visibility flags, possibly set by ourselves above... */
4150+
oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
4151+
oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
4152+
/* ... and store info about transaction updating this tuple */
4153+
Assert(TransactionIdIsValid(xmax_old_tuple));
4154+
HeapTupleHeaderSetXmax(oldtup.t_data, xmax_old_tuple);
4155+
oldtup.t_data->t_infomask |= infomask_old_tuple;
4156+
oldtup.t_data->t_infomask2 |= infomask2_old_tuple;
4157+
HeapTupleHeaderSetCmax(oldtup.t_data, cid, iscombo);
41084158

41094159
/* record address of new tuple in t_ctid of old one */
41104160
oldtup.t_data->t_ctid = heaptup->t_self;

0 commit comments

Comments
 (0)