Re: Split xlog.c

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: vignesh C <vignesh21(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Split xlog.c
Date: 2021-07-31 12:24:23
Message-ID: e5ebefad-2ca6-7c7b-2944-1def0641fa06@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 31/07/2021 10:54, Heikki Linnakangas wrote:
> On 31/07/2021 02:11, Andres Freund wrote:
>>> @@ -5592,111 +5591,6 @@ validateRecoveryParameters(void)
>>> }
>>> }
>>>
>>> -/*
>>> - * Exit archive-recovery state
>>> - */
>>> -static void
>>> -exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog)
>>> -{
>>
>> I don't really understand the motivation for this part of the change? This
>> kind of seems to run counter to the stated goals of the patch series? Seems
>> like it'd need a different commit message at last?
>
> Hmm. Some parts of exitArchiveRecovery are being moved into
> xlogrecovery.c, so it becomes smaller than before. Maybe there's still
> enough code left there that a separate function makes sense. I'll try
> that differently.

So, my issue with exitArchiveRecovery() was that after this refactoring,
the function didn't really exit archive recovery anymore.
InArchiveRecovery flag is cleared earlier already, in xlogrecovery.c. I
renamed exitArchiveRecovery() to XLogInitNewTimeline(), and moved the
unlinking of the signal files into the caller. The function now only
initializes the first WAL segment on the new timeline, and the new name
reflects that. I'm pretty happy with this now.

>>> + /*---- BEGIN FreeWalRecovery ----*/
>>> +
>>> /* Shut down xlogreader */
>>> if (readFile >= 0)
>>> {
>>
>> FWIW, FreeWalRecovery() for something that closes and unlinks files among
>> other things doesn't seem like a great name.
>
> Rename to CloseWalRecovery(), maybe? I'll try that.

I renamed it to ShutdownWalRecovery(). I also refactored the
FinishWalRecovery() function so that instead of having a dozen output
pointer parameters, it returns a struct with all the return values. New
patch set attached.

- Heikki

Attachment Content-Type Size
v4-0001-Move-code-around-in-StartupXLOG.patch text/x-patch 23.3 KB
v4-0002-Split-xlog.c-into-xlog.c-and-xlogrecovery.c.patch text/x-patch 309.5 KB
v4-0003-Move-code-to-apply-one-WAL-record-to-a-subroutine.patch text/x-patch 10.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2021-07-31 12:30:13 Re: Detecting File Damage & Inconsistencies
Previous Message Amit Kapila 2021-07-31 11:36:43 Re: [HACKERS] logical decoding of two-phase transactions