Re: WAL format and API changes (9.5)

On Fri, Oct 31, 2014 at 2:52 AM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
> wrote:

> On 10/30/2014 06:02 PM, Andres Freund wrote:
>
>> On 2014-10-29 10:24:20 +0200, Heikki Linnakangas wrote:
>>
>>> On 10/06/2014 02:29 PM, Andres Freund wrote:
>>>
>>>> I've not yet really looked,
>>>> but on a quick readthrough XLogInsertRecData() staying in xlog.c doesn't
>>>> make me happy...
>>>>
>>>
>>> Ok.. Can you elaborate?
>>>
>>
>> To me the split between xloginsert.c doing some of the record assembly,
>> and xlog.c doing the lower level part of the assembly is just wrong.
>>
>
> Really? To me, that feels like a natural split. xloginsert.c is
> responsible for constructing the final WAL record, with the backup blocks
> and all. It doesn't access any shared memory (related to WAL; it does look
> at Buffers, to determine what to back up). The function in xlog.c inserts
> the assembled record to the WAL, as is. It handles the locking and WAL
> buffer management related to that.
>
FWIW, I tend to the same opinion here.

What would you suggest? I don't think putting everything in one XLogInsert
> function, like we have today, is better. Note that the second patch makes
> xloginsert.c a lot more complicated.
>
I recall some time ago seeing complaints that xlog.c is too complicated and
should be refactored. Any effort in this area is a good thing IMO, and this
split made sense when I went through the code.

> I'm not a big fan of the naming for the new split. We have
>> * XLogInsert() which is the external interface
>> * XLogRecordAssemble() which builds the rdata chain that will be
>> inserted
>> * XLogInsertRecData() which actually inserts the data into the xl buffers.
>>
>> To me that's pretty inconsistent.
>>
>
> Got a better suggestion? I wanted to keep the name XLogInsert() unchanged,
> to avoid code churn, and because it's still a good name for that.
> XLogRecordAssemble is pretty descriptive for what it does, although
> "XLogRecord" implies that it construct an XLogRecord struct. It does fill
> in that too, but the main purpose is to build the XLogRecData chain.
> Perhaps XLogAssembleRecord() would be better.
>
> I'm not very happy with XLogInsertRecData myself. XLogInsertRecord?
>
+1 for XLogInsertRecord.
--
Michael