Re: WAL format and API changes (9.5)

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.

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.

> And it leads to things like the already complicated logic to retry after
> detecting missing fpws is now split across two files seems to confirm
> that. What happens right now is that XLogInsert() (with some helper
> routines) assembles the record. Then hands that off to
> XLogInsertRecData(). Which sometimes returns InvalidXLogRecPtr, and
> returns back to XLogInsert(), which reverses *some* of its work and then
> retries. Brr.

Modifying the chain that was already constructed is not pretty, but it's 
not this patch's fault. I don't think splitting the functions makes that 
any worse. (BTW, the follow-up patch to change the WAL format fixes 
that; the per-buffer data is kept separately from the main data chain).

> Similary the 'page_writes_omitted' logic doesn't make me particularly
> happy. Previously we retried when there actually was a page affected by
> the different RedoRecPtr. Now we do it as soon as our copy of RedoRecPtr
> is out of date? Won't that quite often spuriously trigger retries? Am I
> missing something?
> Arguably this doesn't happen often enough to matter, but it's still
> something that we should explicitly discuss.

The situation where it leads to a spurious retry is when the constructed 
WAL record skipped the FPW of a buffer, and RedoRecPtr was updated, but 
the buffer stilll doesn't need to be FPW according to the updated 
RedoRecPtr. That only happens if the same buffer was updated (by another 
backend) after the new checkpoint began. I believe that's extremely rare.

We could make that more fine-grained, though. Instead of passing a 
boolean 'fpw_sensitive' flag to XLogInsertRecData, pass the lowest LSN 
among the pages whose FPW was skipped. If that's less than the new 
RedoRecPtr, then retry is needed. That would eliminate the spurious retries.

> The implementation of the split seems to change the meaning of
> TRACE_POSTGRESQL_XLOG_INSERT - it's now counting retries. I don't
> particularly care about those tracepoints, but I see no simplification
> due to changing its meaning.

I wasn't sure what to do about that. I don't use tracepoints, and I 
don't know what others use them for.

> 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?

> I'm also somewhat confused about the new split of the headers. I'm not
> adverse to splitting them, but it's getting a bit hard to understand:
> * xlog.h - generic mishmash
> * xlogdefs.h - Three typedefs and some #defines
> * xlog_fn.h - SQL functions
> * xlog_internal.h - Pretty random collection of stuff
> * xlogutils.h - "Utilities for replaying WAL records"
> * xlogreader.h - "generic XLog reading facility"
> * xloginsert.h - "Functions for generating WAL records"
> * xlogrecord.h - "Definitions for the WAL record format."
>
> Isn't that a bit too much? Pretty much the only files that have a
> recognizable separation to me are xlog_fn.h and xlogreader.h.

This is a matter of taste, I guess. I find xlog.h, xlogdefs.h and 
xlog_internal.h fairly random, while the rest are have a clear function. 
xlogutils.h is needed by redo functions, and xloginsert.h is needed for 
generating WAL.

Note that the second patch adds more stuff to xloginsert.h and xlogrecord.h.

> You've removed the
> - *
> - * NB: this routine feels free to scribble on the XLogRecData structs,
> - * though not on the data they reference.  This is OK since the
> XLogRecData
> - * structs are always just temporaries in the calling code.
> comment, but we still do, no?

True. XLogInsertRecData doesn't scribble on its input, but XLogInsert 
still does. (this is moot after the second patch though)

> While not this patches blame, I see currently we finish the critical
> section in XLogInsert/XLogInsertRecData pretty early:
>          END_CRIT_SECTION();
>
>          /*
>           * Update shared LogwrtRqst.Write, if we crossed page boundary.
>           */
>          if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
>          {
>                  SpinLockAcquire(&XLogCtl->info_lck);
>                  /* advance global request to include new block(s) */
>                  if (XLogCtl->LogwrtRqst.Write < EndPos)
>                          XLogCtl->LogwrtRqst.Write = EndPos;
>                  /* update local result copy while I have the chance */
>                  LogwrtResult = XLogCtl->LogwrtResult;
>                  SpinLockRelease(&XLogCtl->info_lck);
>          }
> I don't think this is particularly critical, but it still looks wrong to me.

Updating LogwrtRqst.Write is not critical. If it's not done for some 
reason, everything still works. Keeping the critical section as small as 
possible is a good idea, no?

> Hm. I think that's what I see for now.

Thanks!

- Heikki