Re: Split xlog.c

On Tue, Jun 22, 2021 at 2:37 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 17/06/2021 02:00, Andres Freund wrote:
> > On 2021-06-16 16:30:45 +0300, Heikki Linnakangas wrote:
> >> That's a fairly clean split. StartupXLOG() stays in xlog.c, but much of the
> >> code from it has been moved to new functions InitWalRecovery(),
> >> PerformWalRecovery() and EndWalRecovery(). The general idea is that xlog.c is
> >> still responsible for orchestrating the servers startup, but xlogrecovery.c
> >> is responsible for figuring out whether WAL recovery is needed, performing
> >> it, and deciding when it can stop.
> >
> > For some reason "recovery" bothers me a tiny bit, even though it's obviously
> > already in use. Using "apply", or "replay" seems more descriptive to me, but
> > whatever.
>
> I think of "recovery" as a broader term than applying or replaying.
> Replaying the WAL records is one part of recovery. But yeah, the
> difference is not well-defined and we tend to use those terms
> interchangeably.
>
> >> There's surely more refactoring we could do. xlog.c has a lot of global
> >> variables, with similar names but slightly different meanings for example.
> >> (Quick: what's the difference between InRedo, InRecovery, InArchiveRecovery,
> >> and RecoveryInProgress()? I have to go check the code every time to remind
> >> myself). But this patch tries to just move source code around for clarity.
> >
> > Agreed, it's quite chaotic. I think a good initial step to clean up that mess
> > would be to just collect the relevant variables into one or two structs.
>
> Not a bad idea.
>
> >> There are small changes in the order that some of things are done in
> >> StartupXLOG(), for readability. I tried to be careful and check that the
> >> changes are safe, but a second pair of eyes would be appreciated on that.
> >
> > I think it might be worth trying to break this into a bit more incremental
> > changes - it's a huge commit and mixing code movement with code changes makes
> > it really hard to review the non-movement portion.
>
> Fair. Attached is a new patch set which contains a few smaller commits
> that reorder things in xlog.c, and then the big commit that moves things
> to xlogrecovery.c.
>
> > If we're refactoring all of this, can we move the apply-one-record part into
> > its own function as well? Happy to do that as a followup or precursor patch
> > too. The per-record logic has grown complicated enough to make that quite
> > worthwhile imo - and imo most of the time one either is interested in the
> > per-record work, or in the rest of the StartupXLog/PerformWalRecovery logic.
>
> Added a commit to do that, as a follow-up. Yeah, I agree that makes sense.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh