On 11 August 2015 at 11:39, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Tue, Aug 11, 2015 at 3:44 PM, Simon Riggs <si...@2ndquadrant.com> > wrote: > >> On 11 August 2015 at 10:55, Amit Kapila <amit.kapil...@gmail.com> wrote: >> >>> > What "tricks" are being used?? >>> > >>> > Please explain why taking 2 locks is bad here, yet works fine >>> elsewhere. >>> > >>> >>> One thing that could be risky in this new scheme of locking >>> is that now in functions TransactionIdSetPageStatus and >>> TransactionIdSetStatusBit, we modify slru's shared state with Control >>> Lock >>> in Shared mode whereas I think it is mandated in the code that those >>> should be modified with ControlLock in Exlusive mode. This could have >>> some repercussions. >>> >> >> Do you know of any? This is a technical forum, so if we see a problem we >> say what it is, and if we don't, that's usually classed as a positive point >> in a code review. >> > > One of the main reason of saying this is that it is written in File > level comments in slru.c that for accessing (examine or modify) > the shared state, Control lock *must* be held in Exclusive mode > except in function SimpleLruReadPage_ReadOnly(). So, whereas > I agree that I should think more about if there is any breakage due > to patch, but I don't find any explanation either in your e-mail or in > patch why it is safe to modify the state after patch when it was not > before. If you think it is safe, then atleast modify comments in > slru.c. > "except"... I explained that a reader will never be reading data that is concurrently changed by a writer, so it was safe to break the general rule for this specific case only. Yes, I will modify comments in the patch. > Another thing is that in this flow, with patch there will be three locks >>> (we take per-buffer locks before doing I/O) that will get >>> involved rather than >>> two, so one effect of this patch will be that currently while doing I/O, >>> concurrent committers will be allowed to proceed as we >>> release ControlLock >>> before doing the same whereas with Patch, they will not be allowed as >>> they >>> are blocked by CommitLock. Now may be this scenario is less common and >>> doesn't matter much if the patch improves the more common scenario, >>> however this is an indication of what Andres tries to highlight that >>> having more >>> locks for this might make patch more complicated. >>> >> >> It's easy to stripe the CommitLock in that case, if it is a problem. >> > > Sure, I think other places in code that take both the other locks also > needs to be checked for updation. > Not sure what that means, but there are no other places calling CommitLock -- Simon Riggs http://www.2ndQuadrant.com/ <http://www.2ndquadrant.com/> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
