Skip to content

Commit 2e64824

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 68ab4de commit 2e64824

File tree

1 file changed

+83
-76
lines changed

1 file changed

+83
-76
lines changed

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

Lines changed: 83 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,26 @@
1111
* containing the tuple. (VACUUM FULL assumes it's sufficient to have
1212
* exclusive lock on the containing relation, instead.)
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.
27+
*
1428
*
1529
* Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group
1630
* Portions Copyright (c) 1994, Regents of the University of California
1731
*
1832
* IDENTIFICATION
19-
* $Header: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v 1.49 2002/01/16 23:51:56 tgl Exp $
33+
* $Header: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v 1.49.2.1 2005/05/07 21:23:49 tgl Exp $
2034
*
2135
*-------------------------------------------------------------------------
2236
*/
@@ -109,14 +123,16 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple)
109123

110124
return false;
111125
}
112-
else if (!TransactionIdDidCommit(tuple->t_xmin))
126+
else if (TransactionIdIsInProgress(tuple->t_xmin))
127+
return false;
128+
else if (TransactionIdDidCommit(tuple->t_xmin))
129+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
130+
else
113131
{
114-
if (TransactionIdDidAbort(tuple->t_xmin))
115-
tuple->t_infomask |= HEAP_XMIN_INVALID; /* aborted */
132+
/* it must have aborted or crashed */
133+
tuple->t_infomask |= HEAP_XMIN_INVALID;
116134
return false;
117135
}
118-
else
119-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
120136
}
121137

122138
/* by here, the inserting transaction has committed */
@@ -138,10 +154,13 @@ HeapTupleSatisfiesItself(HeapTupleHeader tuple)
138154
return false;
139155
}
140156

157+
if (TransactionIdIsInProgress(tuple->t_xmax))
158+
return true;
159+
141160
if (!TransactionIdDidCommit(tuple->t_xmax))
142161
{
143-
if (TransactionIdDidAbort(tuple->t_xmax))
144-
tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */
162+
/* it must have aborted or crashed */
163+
tuple->t_infomask |= HEAP_XMAX_INVALID;
145164
return true;
146165
}
147166

@@ -254,14 +273,16 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple)
254273
else
255274
return false; /* deleted before scan started */
256275
}
257-
else if (!TransactionIdDidCommit(tuple->t_xmin))
276+
else if (TransactionIdIsInProgress(tuple->t_xmin))
277+
return false;
278+
else if (TransactionIdDidCommit(tuple->t_xmin))
279+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
280+
else
258281
{
259-
if (TransactionIdDidAbort(tuple->t_xmin))
260-
tuple->t_infomask |= HEAP_XMIN_INVALID; /* aborted */
282+
/* it must have aborted or crashed */
283+
tuple->t_infomask |= HEAP_XMIN_INVALID;
261284
return false;
262285
}
263-
else
264-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
265286
}
266287

267288
/* by here, the inserting transaction has committed */
@@ -286,10 +307,13 @@ HeapTupleSatisfiesNow(HeapTupleHeader tuple)
286307
return false; /* deleted before scan started */
287308
}
288309

310+
if (TransactionIdIsInProgress(tuple->t_xmax))
311+
return true;
312+
289313
if (!TransactionIdDidCommit(tuple->t_xmax))
290314
{
291-
if (TransactionIdDidAbort(tuple->t_xmax))
292-
tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */
315+
/* it must have aborted or crashed */
316+
tuple->t_infomask |= HEAP_XMAX_INVALID;
293317
return true;
294318
}
295319

@@ -428,14 +452,16 @@ HeapTupleSatisfiesUpdate(HeapTuple htuple)
428452
return HeapTupleInvisible; /* updated before scan
429453
* started */
430454
}
431-
else if (!TransactionIdDidCommit(tuple->t_xmin))
455+
else if (TransactionIdIsInProgress(tuple->t_xmin))
456+
return HeapTupleInvisible;
457+
else if (TransactionIdDidCommit(tuple->t_xmin))
458+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
459+
else
432460
{
433-
if (TransactionIdDidAbort(tuple->t_xmin))
434-
tuple->t_infomask |= HEAP_XMIN_INVALID; /* aborted */
461+
/* it must have aborted or crashed */
462+
tuple->t_infomask |= HEAP_XMIN_INVALID;
435463
return HeapTupleInvisible;
436464
}
437-
else
438-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
439465
}
440466

441467
/* by here, the inserting transaction has committed */
@@ -461,15 +487,14 @@ HeapTupleSatisfiesUpdate(HeapTuple htuple)
461487
return HeapTupleInvisible; /* updated before scan started */
462488
}
463489

490+
if (TransactionIdIsInProgress(tuple->t_xmax))
491+
return HeapTupleBeingUpdated;
492+
464493
if (!TransactionIdDidCommit(tuple->t_xmax))
465494
{
466-
if (TransactionIdDidAbort(tuple->t_xmax))
467-
{
468-
tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */
469-
return HeapTupleMayBeUpdated;
470-
}
471-
/* running xact */
472-
return HeapTupleBeingUpdated; /* in updation by other */
495+
/* it must have aborted or crashed */
496+
tuple->t_infomask |= HEAP_XMAX_INVALID;
497+
return HeapTupleMayBeUpdated;
473498
}
474499

475500
/* xmax transaction committed */
@@ -553,19 +578,20 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple)
553578

554579
return false;
555580
}
556-
else if (!TransactionIdDidCommit(tuple->t_xmin))
581+
else if (TransactionIdIsInProgress(tuple->t_xmin))
557582
{
558-
if (TransactionIdDidAbort(tuple->t_xmin))
559-
{
560-
tuple->t_infomask |= HEAP_XMIN_INVALID;
561-
return false;
562-
}
563583
SnapshotDirty->xmin = tuple->t_xmin;
564584
/* XXX shouldn't we fall through to look at xmax? */
565585
return true; /* in insertion by other */
566586
}
567-
else
587+
else if (TransactionIdDidCommit(tuple->t_xmin))
568588
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
589+
else
590+
{
591+
/* it must have aborted or crashed */
592+
tuple->t_infomask |= HEAP_XMIN_INVALID;
593+
return false;
594+
}
569595
}
570596

571597
/* by here, the inserting transaction has committed */
@@ -588,16 +614,17 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple)
588614
return false;
589615
}
590616

591-
if (!TransactionIdDidCommit(tuple->t_xmax))
617+
if (TransactionIdIsInProgress(tuple->t_xmax))
592618
{
593-
if (TransactionIdDidAbort(tuple->t_xmax))
594-
{
595-
tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */
596-
return true;
597-
}
598-
/* running xact */
599619
SnapshotDirty->xmax = tuple->t_xmax;
600-
return true; /* in updation by other */
620+
return true;
621+
}
622+
623+
if (!TransactionIdDidCommit(tuple->t_xmax))
624+
{
625+
/* it must have aborted or crashed */
626+
tuple->t_infomask |= HEAP_XMAX_INVALID;
627+
return true;
601628
}
602629

603630
/* xmax transaction committed */
@@ -693,14 +720,16 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot)
693720
else
694721
return false; /* deleted before scan started */
695722
}
696-
else if (!TransactionIdDidCommit(tuple->t_xmin))
723+
else if (TransactionIdIsInProgress(tuple->t_xmin))
724+
return false;
725+
else if (TransactionIdDidCommit(tuple->t_xmin))
726+
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
727+
else
697728
{
698-
if (TransactionIdDidAbort(tuple->t_xmin))
699-
tuple->t_infomask |= HEAP_XMIN_INVALID;
729+
/* it must have aborted or crashed */
730+
tuple->t_infomask |= HEAP_XMIN_INVALID;
700731
return false;
701732
}
702-
else
703-
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
704733
}
705734

706735
/*
@@ -737,10 +766,13 @@ HeapTupleSatisfiesSnapshot(HeapTupleHeader tuple, Snapshot snapshot)
737766
return false; /* deleted before scan started */
738767
}
739768

769+
if (TransactionIdIsInProgress(tuple->t_xmax))
770+
return true;
771+
740772
if (!TransactionIdDidCommit(tuple->t_xmax))
741773
{
742-
if (TransactionIdDidAbort(tuple->t_xmax))
743-
tuple->t_infomask |= HEAP_XMAX_INVALID; /* aborted */
774+
/* it must have aborted or crashed */
775+
tuple->t_infomask |= HEAP_XMAX_INVALID;
744776
return true;
745777
}
746778

@@ -785,13 +817,6 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin)
785817
*
786818
* If the inserting transaction aborted, then the tuple was never visible
787819
* to any other transaction, so we can delete it immediately.
788-
*
789-
* NOTE: must check TransactionIdIsInProgress (which looks in PROC array)
790-
* before TransactionIdDidCommit/TransactionIdDidAbort (which look in
791-
* pg_clog). Otherwise we have a race condition where we might decide
792-
* that a just-committed transaction crashed, because none of the
793-
* tests succeed. xact.c is careful to record commit/abort in pg_clog
794-
* before it unsets MyProc->xid in PROC array.
795820
*/
796821
if (!(tuple->t_infomask & HEAP_XMIN_COMMITTED))
797822
{
@@ -828,17 +853,9 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin)
828853
return HEAPTUPLE_INSERT_IN_PROGRESS;
829854
else if (TransactionIdDidCommit(tuple->t_xmin))
830855
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
831-
else if (TransactionIdDidAbort(tuple->t_xmin))
832-
{
833-
tuple->t_infomask |= HEAP_XMIN_INVALID;
834-
return HEAPTUPLE_DEAD;
835-
}
836856
else
837857
{
838-
/*
839-
* Not in Progress, Not Committed, Not Aborted - so it's from
840-
* crashed process. - vadim 11/26/96
841-
*/
858+
/* it must be aborted or crashed */
842859
tuple->t_infomask |= HEAP_XMIN_INVALID;
843860
return HEAPTUPLE_DEAD;
844861
}
@@ -879,22 +896,12 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin)
879896
return HEAPTUPLE_DELETE_IN_PROGRESS;
880897
else if (TransactionIdDidCommit(tuple->t_xmax))
881898
tuple->t_infomask |= HEAP_XMAX_COMMITTED;
882-
else if (TransactionIdDidAbort(tuple->t_xmax))
883-
{
884-
tuple->t_infomask |= HEAP_XMAX_INVALID;
885-
return HEAPTUPLE_LIVE;
886-
}
887899
else
888900
{
889-
/*
890-
* Not in Progress, Not Committed, Not Aborted - so it's from
891-
* crashed process. - vadim 06/02/97
892-
*/
901+
/* it must be aborted or crashed */
893902
tuple->t_infomask |= HEAP_XMAX_INVALID;
894903
return HEAPTUPLE_LIVE;
895904
}
896-
/* Should only get here if we set XMAX_COMMITTED */
897-
Assert(tuple->t_infomask & HEAP_XMAX_COMMITTED);
898905
}
899906

900907
/*

0 commit comments

Comments
 (0)