On Tue, Mar 22, 2016 at 1:08 PM, Alexander Korotkov < a.korot...@postgrespro.ru> wrote:
> On Tue, Mar 22, 2016 at 7:57 AM, Dilip Kumar <dilipbal...@gmail.com> > wrote: > >> >> On Tue, Mar 22, 2016 at 12:31 PM, Dilip Kumar <dilipbal...@gmail.com> >> wrote: >> >>> ! pg_atomic_write_u32(&bufHdr->state, state); >>> } while (!StartBufferIO(bufHdr, true)); >>> >>> Better Write some comment, about we clearing the BM_LOCKED from stage >>> directly and need not to call UnlockBufHdr explicitly. >>> otherwise its confusing. >>> >> >> Few more comments.. >> >> *** 828,837 **** >> */ >> do >> { >> ! LockBufHdr(bufHdr); >> *! Assert(bufHdr->flags & BM_VALID);* >> ! bufHdr->flags &= ~BM_VALID; >> ! UnlockBufHdr(bufHdr); >> } while (!StartBufferIO(bufHdr, true)); >> } >> } >> --- 826,834 ---- >> */ >> do >> { >> ! uint32 state = LockBufHdr(bufHdr); >> ! state &= ~(BM_VALID | BM_LOCKED); >> ! pg_atomic_write_u32(&bufHdr->state, state); >> } while (!StartBufferIO(bufHdr, true)); >> >> 1. Previously there was a Assert, any reason why we removed it ? >> Assert(bufHdr->flags & BM_VALID); >> > > It was missed. In the attached patch I've put it back. > > 2. And also if we don't need assert then can't we directly clear BM_VALID >> flag from state variable (if not locked) like pin/unpin buffer does, >> without taking buffer header lock? >> > > In this version of patch it could be also done as loop of CAS operation. > But I'm not intended to it so because it would significantly complicate > code. It's not yes clear that traffic in this place is high enough to make > such optimizations. > Since v4 patch implements slightly different approach. Could you please > test it? We need to check that this approach worth putting more efforts on > it. Or through it away otherwise. > Could anybody run benchmarks? Feature freeze is soon, but it would be *very nice* to fit it into 9.6 release cycle, because it greatly improves scalability on large machines. Without this patch PostgreSQL 9.6 will be significantly behind competitors like MySQL 5.7. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company