Re: Logical decoding timeline following fails to handle records split across segments

On 3 May 2016 at 22:03, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> A corrected and handily much, much simpler patch is attached. The logic
> for finding the last timeline on a segment was massively more complex than
> it needed to be, and that wasn't the only thing.
>

I'm aware that this is late in the piece, btw, and that revert will be
considered. I think that's probably unnecessary - in part because this code
only gets called when doing logical decoding, and only does anything
remotely interesting when replay from a slot hits a timeline boundary. So
it should be assessed mainly based on its risk of breaking what works. That
said, I also think that now that I've fixed the fundamental
misunderstanding embodied by the original patch it should be pretty clear
and reasonable.

The diff looks big, but it just rewrites the new XLogReadDetermineTimeline
function and otherwise just removes the no-longer-needed
XlogReaderState->timelineHistory member the prior one added. It's best to
just read the new XLogReadDetermineTimeline function, not the diff of that
function, since diff has done confusing things with it.

The changes here are that:

* XLogReadDetermineTimeline(...) now looks up the page being fetched by
ReadPageInternal. It previously used the record start pointer, but that
didn't work if a continuation spilled over to the first page on a new
segment where there's a timeline switch.

* The function first tests cases where it can say "nothing to do, carry on"
and only then handles a timeline switch if one might be required. Simpler,
clearer.

* The logic for determining the last timeline on a segment was way too
complicated in the old patch. Instead, just look up the timeline of the LSN
of the last byte on the segment. No more loops.

* XlogReaderState->timelineHistory is removed; the timeline file is now
read only when needed when making a timeline decision, then discarded. It's
read only very infrequently. Any time we read it we might need to re-read
it if there's a newer one anyway, so it's simpler this way.

* XLogReaderState->currTLIValidUntil is now the tliSwitchPoint for currTLI
and is no longer truncated to the start of the segment boundary like
before. So it's actually useful now. That's a result of fixing the stupid
logic I used in the old version for calculating the last timeline on a
segment and whether there's a timeline change on the segment.

Álvaro is aware of this and I've added it to open items.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services