Thread: HeapTupleSatisfiesUpdate missing a bet?

Hackers,

I got very strange results in my shared-row-locking test today, so I
took a look at HeapTupleSatisfiesUpdate and came to the conclusion
that it's delivering the wrong answer in some cases; specifically, it
returns HeapTupleBeingUpdated for tuples whose Xmax were touched by a
crashed transaction.

Apparently this doesn't matter on the current code because all callers
iterate using XactLockTableWait, which marks the deleting transaction as
aborted if it isn't running.  However, I don't want to use
XactLockTableWait in the new code (in case we may be able to remove it).
So I'd like to have HeapTupleSatisfiesUpdate call TransactionIdAbort()
in case it's necessary.

What do people think of this patch?  This is of course only to
illustrate what I'm talking about -- the real patch needs to change
other HeapTupleSatisfies routines as well.  The other possibility would
be to have the new heap_locktuple routine check Xmax instead.  Not sure
which is worst.

Index: src/backend/utils/time/tqual.c
===================================================================
RCS file: /home/alvherre/cvs/pgsql/src/backend/utils/time/tqual.c,v
retrieving revision 1.86
diff -c -r1.86 tqual.c
*** src/backend/utils/time/tqual.c    20 Mar 2005 23:40:27 -0000    1.86
--- src/backend/utils/time/tqual.c    25 Mar 2005 22:40:53 -0000
***************
*** 539,544 ****
--- 539,550 ----                 tuple->t_infomask |= HEAP_XMIN_INVALID;
SetBufferCommitInfoNeedsSave(buffer);            }
 
+             else if (!TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
+             {
+                 TransactionIdAbort(HeapTupleHeaderGetXmin(tuple));
+                 tuple->t_infomask |= HEAP_XMIN_INVALID;
+                 SetBufferCommitInfoNeedsSave(buffer);
+             }             return HeapTupleInvisible;         }         else
***************
*** 579,584 ****
--- 585,597 ----             SetBufferCommitInfoNeedsSave(buffer);             return HeapTupleMayBeUpdated;         }
+         else if (!TransactionIdIsInProgress(HeapTupleHeaderGetXmax(tuple)))
+         {
+             TransactionIdAbort(HeapTupleHeaderGetXmax(tuple));
+             tuple->t_infomask |= HEAP_XMAX_INVALID;
+             SetBufferCommitInfoNeedsSave(buffer);
+             return HeapTupleMayBeUpdated;
+         }         /* running xact */         return HeapTupleBeingUpdated;    /* in updation by other */     }

-- 
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"I personally became interested in Linux while I was dating an English major
who wouldn't know an operating system if it walked up and bit him."
(Val Henson)

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> I got very strange results in my shared-row-locking test today, so I
> took a look at HeapTupleSatisfiesUpdate and came to the conclusion
> that it's delivering the wrong answer in some cases; specifically, it
> returns HeapTupleBeingUpdated for tuples whose Xmax were touched by a
> crashed transaction.

It's not wrong: the transaction *is* in progress, or has to be treated
as such, until you prove differently.

> What do people think of this patch?

It looks like an expensive solution to a non-problem.
TransactionIdIsInProgress isn't particularly cheap and the test will be
wasted 99.999% of the time.

Also, you just introduced a race condition, since the transaction might
have committed after the earlier tests and before you did
TransactionIdIsInProgress.  You really have to do
TransactionIdIsInProgress *first*, which makes the proposed change even
more expensive.

What's wrong with using XactLockTableWait?  It's not going away --- the
implementation might change but I can't see getting rid of the
functionality.
        regards, tom lane

On Fri, Mar 25, 2005 at 06:46:58PM -0500, Tom Lane wrote:

> Also, you just introduced a race condition, since the transaction might
> have committed after the earlier tests and before you did
> TransactionIdIsInProgress.  You really have to do
> TransactionIdIsInProgress *first*, which makes the proposed change even
> more expensive.

Oh, right.  I knew there was a reason, I just couldn't remember it.

> What's wrong with using XactLockTableWait?  It's not going away --- the
> implementation might change but I can't see getting rid of the
> functionality.

Nothing wrong indeed, if you take this PoV.  That's exactly what I've
done now, since it is what heap_mark4update (which I'm replacing) does
at present.  (I use LockTuple(), a lock which is only released at
transaction end, so the net result is semantically equivalent to
XactLockTableWait -- that's why I want to get rid of it.)

-- 
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"El sentido de las cosas no viene de las cosas, sino de
las inteligencias que las aplican a sus problemas diarios
en busca del progreso." (Ernesto Hernández-Novich)