On 08/14/2014 12:44 PM, Andres Freund wrote:
> On 2014-08-14 12:41:35 +0300, Heikki Linnakangas wrote:
>> On 08/14/2014 10:29 AM, Michael Paquier wrote:
>>> On Thu, Aug 14, 2014 at 3:04 AM, Alvaro Herrera
>>> <alvherre@2ndquadrant.com> wrote:
>>>> Heikki Linnakangas wrote:
>>>> What's with XLogReplayLSN and XLogReplayRecord? IMO the xlog code has
>>>> more than enough global variables already, it'd be good to avoid two
>>>> more if possible. Is there no other good way to get this info down to
>>>> whatever it is that needs them?
>>> Yep, they do not look necessary. Looking at the patch, you could get
>>> rid of XLogReplayLSN and XLogReplayRecord by adding two extra argument
>>> to XLogReplayBuffer: one for the LSN of the current record, and a
>>> second for the record pointer. The code just saves those two variables
>>> in the redo loop of StartupXLOG to only reuse them in
>>> XLogReplayBufferExtended, and I saw no code paths in the redo routines
>>> where XLogReplayBuffer is called at places without the LSN position
>>> and the record pointer.
>>>
>>> However I think that Heikki introduced those two variables to make the
>>> move to the next patch easier.
>>
>> The next patch doesn't necessary require them either, you could always pass
>> the LSN and record as an argument. I wanted to avoid that, because every
>> redo function would just pass the current record being replayed, so it seems
>> nicer to pass that information "out-of-band". I guess if we do that, though,
>> we should remove those arguments from rm_redo interface altogether, and
>> always rely on the global variables to get the "current" record or its LSN.
>> I'm not wedded on this, I could be persuaded to go either way...
>
> I personally find the out of band transport really ugly.
All rightey.
Here's the next version of this work. It now comes as two patches. The
first one refactors adds the XLogReplayBuffer() function and refactors
all the redo functions to use it. It doesn't change the WAL record
format in any way. The second patch applies over the first one, and
changes the WAL format, and all the WAL-creating functions to use the
new API for constructing WAL records. The second patch removes the
relfilenode and block number arguments from XLogReplayBuffer, because
they're no longer needed when that information is included in the record
format.
Todo:
* Performance testing. Do the new WAL construction functions add
overhead to WAL insertion?
* Compare WAL record sizes before and after. I've tried to keep it as
compact as possible, but I know that some records have gotten bigger.
Need to do a more thorough analysis.
* Rename XLogReplayBuffer. I don't particularly like any of the
suggestions so far, XLogReplayBuffer included. Discussion continues..
* Anything else?
- Heikki