Re: Attempt to consolidate reading of XLOG page

Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> I spent a couple of hours on this patchset today.  I merged 0001 and
> 0002, and decided the result was still messier than I would have liked,
> so I played with it a bit more -- see attached.  I think this is
> committable, but I'm afraid it'll cause quite a few conflicts with the
> rest of your series.
> 
> I had two gripes, which I feel solved with my changes:
> 
> 1. I didn't like that "dir" and "wal segment size" were part of the
> "currently open segment" supporting struct.  It seemed that those two
> were slightly higher-level, since they apply to every segment that's
> going to be opened, not just the current one.

ok

> My first thought was to put those as members of XLogReaderState, but
> that doesn't work because the physical walsender.c code does not use
> xlogreader at all, even though it is reading WAL.

`I don't remember clearly but I think that this was the reason I tried to move
"wal_segment_size" away from XLogReaderState.

 
> Separately from those two API-wise points, there was one bug which meant
> that with your 0002+0003 the recovery tests did not pass -- code
> placement bug.  I suppose the bug disappears with later patches in your
> series, which probably is why you didn't notice.  This is the fix for that:
> 
> -   XLogRead(cur_page, state->seg.size, state->seg.tli, targetPagePtr,
> -   state->seg.tli = pageTLI;
> +   state->seg.ws_tli = pageTLI;
> +   XLogRead(cur_page, state->segcxt.ws_segsize, state->seg.ws_tli, targetPagePtr,
>              XLOG_BLCKSZ);
> 

Yes, it seems so - the following parts ensure that XLogRead() adjusts the
timeline itself. I only checked that the each part of the series keeps the
source tree compilable. Thanks for fixing.

> ... Also, yes, I renamed all the struct members.
>
> 
> If you don't have any strong dislikes for these changes, I'll push this
> part and let you rebase the remains on top.

No objections here.

> 2. Not a fan of the InvalidTimeLineID stuff offhand.  Maybe it's okay ...
> not convinced yet either way.

Well, it seems that the individual callbacks only use this constant in
Assert() statements. I'll consider if we really need it. The argument value
should not determine whether the callback derives the TLI or not.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com