Thread: Re: [COMMITTERS] pgsql: Unite ReadBufferWithFork, ReadBufferWithStrategy, and

On Sat, 2008-11-01 at 15:18 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
> >> Log Message:
> >> -----------
> >> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
> >> functions into one ReadBufferExtended function, that takes the strategy
> >> and mode as argument. There's three modes, RBM_NORMAL which is the default
> >> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
> >> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
> >> without throwing an error. The FSM needs the new mode to recover from
> >> corrupt pages, which could happend if we crash after extending an FSM file,
> >> and the new page is "torn".
> > 
> > I thought you were adding the "read buffer only if in cache" option
> > also?
> 
> No, but if it's needed, it should now fit well into the infrastructure, 
> as a new ReadBuffer mode.

Not sure how this helps me; maybe it wasn't supposed to? I need to
implement XLogReadBufferExtended to take a cleanup lock. It seems wrong
to make that a ReadBufferMode, since the option refers to what happens
after we do ReadBuffer and especially because we need to do RBM_ZERO and
Cleanup mode at same time.

It would be much better to have ReadBufferExtended take a lockmode
argument also.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Sat, 2008-11-01 at 15:18 +0200, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>>> On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
>>>> Log Message:
>>>> -----------
>>>> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
>>>> functions into one ReadBufferExtended function, that takes the strategy
>>>> and mode as argument. There's three modes, RBM_NORMAL which is the default
>>>> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
>>>> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
>>>> without throwing an error. The FSM needs the new mode to recover from
>>>> corrupt pages, which could happend if we crash after extending an FSM file,
>>>> and the new page is "torn".
>>> I thought you were adding the "read buffer only if in cache" option
>>> also?
>> No, but if it's needed, it should now fit well into the infrastructure, 
>> as a new ReadBuffer mode.
> 
> Not sure how this helps me; maybe it wasn't supposed to? 

Well, no, not directly. it was to plug the hole in the FSM 
implementation with torn pages, and to make the ReadBuffer interface nicer.

> I need to
> implement XLogReadBufferExtended to take a cleanup lock. It seems wrong
> to make that a ReadBufferMode, since the option refers to what happens
> after we do ReadBuffer and especially because we need to do RBM_ZERO and
> Cleanup mode at same time.
> 
> It would be much better to have ReadBufferExtended take a lockmode
> argument also.

No, I don't think we should mix ReadBuffer with the locking. On the 
contrary, my suggestion is to modify XLogReadBufferExtended so that it 
doesn't lock the page either, and leave the locking to the callers.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com