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?