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

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