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