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