Skip to content

Commit b72e5fa

Browse files
tglsfdctglsfdc
committed
Adjust time qual checking code so that we always check TransactionIdIsInProgress
before we check commit/abort status. Formerly this was done in some paths but not all, with the result that a transaction might be considered committed for some purposes before it became committed for others. Per example found by Jan Wieck.
1 parent 4cd4ed0 commit b72e5fa

File tree

1 file changed

+93
-80
lines changed

1 file changed

+93
-80
lines changed

‎src/backend/utils/time/tqual.c‎

Lines changed: 93 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,28 @@
1111
* "hint" status bits if we see that the inserting or deleting transaction
1212
* has now committed or aborted.
1313
*
14+
* NOTE: must check TransactionIdIsInProgress (which looks in PGPROC array)
15+
* before TransactionIdDidCommit/TransactionIdDidAbort (which look in
16+
* pg_clog). Otherwise we have a race condition: we might decide that a
17+
* just-committed transaction crashed, because none of the tests succeed.
18+
* xact.c is careful to record commit/abort in pg_clog before it unsets
19+
* MyProc->xid in PGPROC array. That fixes that problem, but it also
20+
* means there is a window where TransactionIdIsInProgress and
21+
* TransactionIdDidCommit will both return true. If we check only
22+
* TransactionIdDidCommit, we could consider a tuple committed when a
23+
* later GetSnapshotData call will still think the originating transaction
24+
* is in progress, which leads to application-level inconsistency. The
25+
* upshot is that we gotta check TransactionIdIsInProgress first in all
26+
* code paths, except for a few cases where we are looking at
27+
* subtransactions of our own main transaction and so there can't be any
28+
* race condition.
29+
*
1430
*
1531
* Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
1632
* Portions Copyright (c) 1994, Regents of the University of California
1733
*
1834
* IDENTIFICATION
19-
* $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.87 2005/04/28 21:47:16 tgl Exp $
35+
* $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.88 2005/05/07 21:22:01 tgl Exp $
2036
*
2137
*-------------------------------------------------------------------------
2238
*/
@@ -147,19 +163,19 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple, Buffer buffer)
147163

148164
return false;
149165
}
150-
else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
151-
{
152-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
153-
{
154-
tuple->t_infomask |= HEAP_XMIN_INVALID;
155-
SetBufferCommitInfoNeedsSave(buffer);
156-
}
166+
else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
157167
return false;
168+
else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
169+
{
170+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
171+
SetBufferCommitInfoNeedsSave(buffer);
158172
}
159173
else
160174
{
161-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
175+
/* it must have aborted or crashed */
176+
tuple->t_infomask |= HEAP_XMIN_INVALID;
162177
SetBufferCommitInfoNeedsSave(buffer);
178+
return false;
163179
}
164180
}
165181

@@ -189,13 +205,14 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple, Buffer buffer)
189205
return false;
190206
}
191207

208+
if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
209+
return true;
210+
192211
if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
193212
{
194-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
195-
{
196-
tuple->t_infomask |= HEAP_XMAX_INVALID;
197-
SetBufferCommitInfoNeedsSave(buffer);
198-
}
213+
/* it must have aborted or crashed */
214+
tuple->t_infomask |= HEAP_XMAX_INVALID;
215+
SetBufferCommitInfoNeedsSave(buffer);
199216
return true;
200217
}
201218

@@ -330,19 +347,19 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple, Buffer buffer)
330347
else
331348
return false; /* deleted before scan started */
332349
}
333-
else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
334-
{
335-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
336-
{
337-
tuple->t_infomask |= HEAP_XMIN_INVALID;
338-
SetBufferCommitInfoNeedsSave(buffer);
339-
}
350+
else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
340351
return false;
352+
else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
353+
{
354+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
355+
SetBufferCommitInfoNeedsSave(buffer);
341356
}
342357
else
343358
{
344-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
359+
/* it must have aborted or crashed */
360+
tuple->t_infomask |= HEAP_XMIN_INVALID;
345361
SetBufferCommitInfoNeedsSave(buffer);
362+
return false;
346363
}
347364
}
348365

@@ -375,13 +392,14 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple, Buffer buffer)
375392
return false; /* deleted before scan started */
376393
}
377394

395+
if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
396+
return true;
397+
378398
if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
379399
{
380-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
381-
{
382-
tuple->t_infomask |= HEAP_XMAX_INVALID;
383-
SetBufferCommitInfoNeedsSave(buffer);
384-
}
400+
/* it must have aborted or crashed */
401+
tuple->t_infomask |= HEAP_XMAX_INVALID;
402+
SetBufferCommitInfoNeedsSave(buffer);
385403
return true;
386404
}
387405

@@ -569,19 +587,19 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
569587
return HeapTupleInvisible; /* updated before scan
570588
* started */
571589
}
572-
else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
573-
{
574-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
575-
{
576-
tuple->t_infomask |= HEAP_XMIN_INVALID;
577-
SetBufferCommitInfoNeedsSave(buffer);
578-
}
590+
else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
579591
return HeapTupleInvisible;
592+
else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
593+
{
594+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
595+
SetBufferCommitInfoNeedsSave(buffer);
580596
}
581597
else
582598
{
583-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
599+
/* it must have aborted or crashed */
600+
tuple->t_infomask |= HEAP_XMIN_INVALID;
584601
SetBufferCommitInfoNeedsSave(buffer);
602+
return HeapTupleInvisible;
585603
}
586604
}
587605

@@ -620,16 +638,15 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
620638
return HeapTupleInvisible; /* updated before scan started */
621639
}
622640

641+
if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
642+
return HeapTupleBeingUpdated;
643+
623644
if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
624645
{
625-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
626-
{
627-
tuple->t_infomask |= HEAP_XMAX_INVALID;
628-
SetBufferCommitInfoNeedsSave(buffer);
629-
return HeapTupleMayBeUpdated;
630-
}
631-
/* running xact */
632-
return HeapTupleBeingUpdated; /* in updation by other */
646+
/* it must have aborted or crashed */
647+
tuple->t_infomask |= HEAP_XMAX_INVALID;
648+
SetBufferCommitInfoNeedsSave(buffer);
649+
return HeapTupleMayBeUpdated;
633650
}
634651

635652
/* xmax transaction committed */
@@ -735,23 +752,24 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Buffer buffer)
735752

736753
return false;
737754
}
738-
else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
755+
else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
739756
{
740-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
741-
{
742-
tuple->t_infomask |= HEAP_XMIN_INVALID;
743-
SetBufferCommitInfoNeedsSave(buffer);
744-
return false;
745-
}
746757
SnapshotDirty->xmin = HeapTupleHeaderGetXmin(tuple);
747758
/* XXX shouldn't we fall through to look at xmax? */
748759
return true; /* in insertion by other */
749760
}
750-
else
761+
else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
751762
{
752763
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
753764
SetBufferCommitInfoNeedsSave(buffer);
754765
}
766+
else
767+
{
768+
/* it must have aborted or crashed */
769+
tuple->t_infomask |= HEAP_XMIN_INVALID;
770+
SetBufferCommitInfoNeedsSave(buffer);
771+
return false;
772+
}
755773
}
756774

757775
/* by here, the inserting transaction has committed */
@@ -781,17 +799,18 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Buffer buffer)
781799
return false;
782800
}
783801

784-
if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
802+
if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
785803
{
786-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
787-
{
788-
tuple->t_infomask |= HEAP_XMAX_INVALID;
789-
SetBufferCommitInfoNeedsSave(buffer);
790-
return true;
791-
}
792-
/* running xact */
793804
SnapshotDirty->xmax = HeapTupleHeaderGetXmax(tuple);
794-
return true; /* in updation by other */
805+
return true;
806+
}
807+
808+
if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
809+
{
810+
/* it must have aborted or crashed */
811+
tuple->t_infomask |= HEAP_XMAX_INVALID;
812+
SetBufferCommitInfoNeedsSave(buffer);
813+
return true;
795814
}
796815

797816
/* xmax transaction committed */
@@ -907,19 +926,19 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot,
907926
else
908927
return false; /* deleted before scan started */
909928
}
910-
else if (!TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
911-
{
912-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(tuple)))
913-
{
914-
tuple->t_infomask |= HEAP_XMIN_INVALID;
915-
SetBufferCommitInfoNeedsSave(buffer);
916-
}
929+
else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
917930
return false;
931+
else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
932+
{
933+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
934+
SetBufferCommitInfoNeedsSave(buffer);
918935
}
919936
else
920937
{
921-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
938+
/* it must have aborted or crashed */
939+
tuple->t_infomask |= HEAP_XMIN_INVALID;
922940
SetBufferCommitInfoNeedsSave(buffer);
941+
return false;
923942
}
924943
}
925944

@@ -982,13 +1001,14 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot,
9821001
return false; /* deleted before scan started */
9831002
}
9841003

1004+
if (TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
1005+
return true;
1006+
9851007
if (!TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))
9861008
{
987-
if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
988-
{
989-
tuple->t_infomask |= HEAP_XMAX_INVALID;
990-
SetBufferCommitInfoNeedsSave(buffer);
991-
}
1009+
/* it must have aborted or crashed */
1010+
tuple->t_infomask |= HEAP_XMAX_INVALID;
1011+
SetBufferCommitInfoNeedsSave(buffer);
9921012
return true;
9931013
}
9941014

@@ -1057,13 +1077,6 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin,
10571077
*
10581078
* If the inserting transaction aborted, then the tuple was never visible
10591079
* to any other transaction, so we can delete it immediately.
1060-
*
1061-
* NOTE: must check TransactionIdIsInProgress (which looks in PROC array)
1062-
* before TransactionIdDidCommit/TransactionIdDidAbort (which look in
1063-
* pg_clog). Otherwise we have a race condition where we might decide
1064-
* that a just-committed transaction crashed, because none of the
1065-
* tests succeed. xact.c is careful to record commit/abort in pg_clog
1066-
* before it unsets MyProc->xid in PROC array.
10671080
*/
10681081
if (!(tuple->t_infomask & HEAP_XMIN_COMMITTED))
10691082
{

0 commit comments

Comments
 (0)