On 30/11/17 00:47, Andres Freund wrote: > On 2017-11-30 00:45:44 +0100, Petr Jelinek wrote: >> I don't understand. I mean sure the SnapBuildWaitSnapshot() can live >> with it, but the problematic logic happens inside the >> XactLockTableInsert() and SnapBuildWaitSnapshot() has no way of >> detecting the situation short of reimplementing the >> XactLockTableInsert() instead of calling it. > > Right. But we fairly trivially can change that. I'm remarking on it > because other people's, not yours, suggestions aimed at making this > bulletproof. I just wanted to make clear that I don't think that's > necessary at all. >
Okay, then I guess we are in agreement. I can confirm that the attached fixes the issue in my tests. Using SubTransGetTopmostTransaction() instead of SubTransGetParent() means 3 more ifs in terms of extra CPU cost for other callers. I don't think it's worth worrying about given we are waiting for heavyweight lock, but if we did we can just inline the code directly into SnapBuildWaitSnapshot(). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
From 7d230d6453e2ac6e4c47b2af989dea62e667fc4b Mon Sep 17 00:00:00 2001 From: Petr Jelinek <pjmo...@pjmodos.net> Date: Thu, 30 Nov 2017 01:18:55 +0100 Subject: [PATCH] Make XactLockTableWait work for transactions that are not yet self locked The XactLockTableWait assumed that the xid given on input has already added itself to the locktable. This led to another assumption that if the lock of the xid has succeeded but the xid is still in progress, the inpud xid must have been subtransaction. These assumtions might not be true in logical slot snapshot building on rare occation where the standby snapshot logged contains xid that has already been added to ProcArray but not to lock table yet. The new coding will just not assume xid is subtransaction anymore and will retry the lock with same toplevel xid it was given before (courtesy of SubTransGetTopmostTransaction()). For consistency reasons, same change is made to ConditionalXactLockTableWait. --- src/backend/storage/lmgr/lmgr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index da5679b7a3..9ba6ea00b1 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -590,7 +590,7 @@ XactLockTableWait(TransactionId xid, Relation rel, ItemPointer ctid, if (!TransactionIdIsInProgress(xid)) break; - xid = SubTransGetParent(xid); + xid = SubTransGetTopmostTransaction(xid); } if (oper != XLTW_None) @@ -622,7 +622,7 @@ ConditionalXactLockTableWait(TransactionId xid) if (!TransactionIdIsInProgress(xid)) break; - xid = SubTransGetParent(xid); + xid = SubTransGetTopmostTransaction(xid); } return true; -- 2.14.1
