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