On Sat, Sep 24, 2016 at 8:24 PM, Heikki Linnakangas <hlinn...@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.


> 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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to