On 30/11/17 11:48, Simon Riggs wrote: > On 30 November 2017 at 11:30, Petr Jelinek <petr.jeli...@2ndquadrant.com> > wrote: >> 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(). > > This will still fail an Assert in TransactionIdIsInProgress() when > snapshots are overflowed. >
Hmm, which one, why? I see 2 Asserts there, one is: > Assert(nxids == 0); Which is inside the RecoveryInProgress(), surely on standbys there will still be no PGXACTs with assigned xids so that should be fine. The other one is: > Assert(TransactionIdIsValid(topxid)); Which should be again fine toplevel xid of toplevel xid is same xid which is a valid one. So I think we should be fine there. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
