On 08/13/2014 02:07 PM, Andres Freund wrote:
> On 2014-08-13 02:36:59 +0300, Heikki Linnakangas wrote:
>> On 08/13/2014 01:04 AM, Andres Freund wrote:
>>> * The patch mixes the API changes around WAL records with changes of how
>>> individual actions are logged. That makes it rather hard to review -
>>> and it's a 500kb patch already.
>>>
>>> I realize it's hard to avoid because the new API changes which
>>> information about blocks is available, but it does make it really hard
>>> to see whether the old/new code is doing something
>>> equivalent. It's hard to find more critical code than this :/
>>
>> Yeah, I hear you. I considered doing this piecemeal, just adding the new
>> functions first so that you could still use the old XLogRecData API, until
>> all the functions have been converted. But in the end, I figured it's not
>> worth it, as sooner or later we'd want to convert all the functions anyway.
>
> I think it might be worthwile anyway. I'd be very surprised if there
> aren't several significant bugs in the conversion. Your full page
> checking tool surely helps to reduce the number, but it's not
> foolproof. I can understand not wanting to do it though, it's a
> significant amount of work.
>
> Would you ask somebody else to do it in two steps?
Hmm, thinking about this some more, there is one sensible way to split
this patch: We can add the XLogReplayBuffer() function and rewrite all
the redo routines to use it, without changing any WAL record formats or
anything in the way the WAL records are constructed. In the patch,
XLogReplayBuffer() takes one input arument, the block reference ID, and
it fetches the RelFileNode and BlockNumber of the block based on that.
Without the WAL format changes, the information isn't there in the
record, but we can require the callers to pass the RelFileNode and
BlockNumber. The final patch will remove those arguments from every
caller, but that's a very mechanical change.
As in the attached patch. I only modified the heapam redo routines to
use the new XLogReplayBuffer() idiom; the idea is to do that for every
redo routine.
After applying such a patch, the main WAL format changing patch becomes
much smaller, and makes it easier to see from the redo routines where
significant changes to the WAL record formats have been made. This also
allows us to split the bikeshedding; we can discuss the name of
XLogReplayBuffer() first :-).
- Heikki
