On Sat, Sep 24, 2016 at 8:24 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 09/24/2016 05:01 AM, Thomas Munro wrote:
>>
>> What would the appetite be for that kind of refactoring work,
>> considering the increased burden on committers who have to backpatch
>> bug fixes? Is it a project goal to reduce the size of large
>> complicated functions like StartupXLOG and heap_update? It seems like
>> a good way for new players to learn how they work.
>
>
> +1. Yes, it does increase the burden of backpatching, but I think it'd still
> be very much worth it.
Cool.
> A couple of little details that caught my eye at a quick read:
>
>> /* Try to find a backup label. */
>> if (read_backup_label(&checkPointLoc, &backupEndRequired,
>> &backupFromStandby))
>> {
>> wasShutdown = ProcessBackupLabel(xlogreader, &checkPoint,
>> checkPointLoc,
>>
>> &haveTblspcMap);
>>
>> /* set flag to delete it later */
>> haveBackupLabel = true;
>> }
>> else
>> {
>> /* Clean up any orphaned tablespace map files with no
>> backup label. */
>> CleanUpTablespaceMap();
>> ...
>
>
> This is a bit asymmetric: In the true-branch, ProcessBackupLabel() reads the
> tablespace map, and sets InArchiveRecovery and StandbyMode, but in the
> false-branch, StartupXLog() calls CleanupTablespaceMap() and sets those
> variables directly.
Right. I need to move all or some of the other branch out to its own
function too.
> For functions like BeginRedo, BeginHotStandby, ReplayRedo, etc., I think
> it'd be better to have the "if (InRecovery)" checks in the caller, rather
> than in the functions.
Yeah. I was thinking that someone might value the preservation of
indention level, since that might make small localised bug fixes
easier to backport to the monolithic StartupXLOG. Plain old
otherwise-redundant curly braces would achieve that. Or maybe it's
better not to worry about preserving that.
--
Thomas Munro
http://www.enterprisedb.com