Re: Refactor StartupXLOG?

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.

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.

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.

- Heikki