Re: WAL format and API changes (9.5) - Mailing list pgsql-hackers

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:
>>> On 2014-10-06 14:19:39 +0300, Heikki Linnakangas wrote:
>>>> Barring objections, I'll commit this, and then continue benchmarking the
>>>> second patch with the WAL format and API changes.
>>>
>>> I'd like to have a look at it beforehand.
>>
>> Ping? Here's an rebased patch. I'd like to proceed with this.
>
> Doing so.

Here's a new version of this refactoring patch. It fixes all the
concrete issues you pointed out.

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

I moved the checks for bootstrap mode and xl_len == 0, from the current
XLogInsert to the new XLogInsert() function. I don't imagine that to be
enough address your feelings about the split between xlog.c and
xloginsert.c, but makes it a tiny bit clearer IMHO. I don't know what
else to do about it, as it feels very sensible to me as it is now. So
unless you object more loudly, I'm going to just go ahead with this and
commit, probably tomorrow.

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

I replaced the boolean with an XLogRecPtr of the smallest LSN among the
pages that were modified but not backed up. That brings the behavior
back to the way it is currently; no more spurious retries. Thanks for
making me to think harder about that, I think this way is also clearer
than the boolean.

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

Fixed.

> Next thing: The patch doesn't actually compile. Misses an #include
> storage/proc.h for MyPgXact.

Fixed.

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

Renamed XLogInsertRecData() to XLogInsertRecord(). Not sure if it makes
it more consistent, but it sounds better anyway.

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

Fixed.

- Heikki