On Tue, Jun 22, 2021 at 2:37 AM Heikki Linnakangas <hlinn...@iki.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
