On Fri, Oct 31, 2014 at 2:52 AM, Heikki Linnakangas <hlinnakan...@vmware.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
