Re: Move PinBuffer and UnpinBuffer to atomics

On Tue, Mar 22, 2016 at 7:57 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> On Tue, Mar 22, 2016 at 12:31 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company