Re: Split xlog.c

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Split xlog.c
Date: 2021-06-21 21:06:41
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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

Attachment Content-Type Size
0001-Don-t-use-O_SYNC-or-similar-when-opening-signal-file.patch text/x-patch 1.3 KB
0002-Remove-unnecessary-restoredFromArchive-global-variab.patch text/x-patch 1.7 KB
0003-Extract-code-to-get-reason-that-recovery-was-stopped.patch text/x-patch 3.7 KB
0004-Move-InRecovery-and-standbyState-global-vars-to-xlog.patch text/x-patch 13.3 KB
0005-Move-code-around-in-StartupXLOG.patch text/x-patch 26.1 KB
0006-Split-xlog.c-into-xlog.c-and-xlogrecovery.c.patch text/x-patch 306.3 KB
0007-Move-code-to-apply-one-WAL-record-to-a-subroutine.patch text/x-patch 10.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2021-06-21 21:26:33 Re: disfavoring unparameterized nested loops
Previous Message Tom Lane 2021-06-21 20:52:37 Re: disfavoring unparameterized nested loops