Hi ZizhuanLiu,
> Rebase v4 .
Thanks for the rebase. I read through the v4 refactor of
ReserveXLogSwitch() -- computing the standard EndPos once and reusing it
reads much cleaner than the previous duplicated arithmetic. A few small,
behavior-neutral suggestions on the new out-parameter, in case you'd like
to fold them into the next version:
1) Naming. Of the three, this is the one I'd really like to see in the
next version. The parameter is spelled actual_EndPos, which is neither
snake_case nor CamelCase -- an underscore-separated lowercase prefix glued
to a CamelCase tail -- unlike its sibling out-parameters in the same
function (StartPos, EndPos, PrevPtr) that are plain CamelCase; and
"actual" doesn't say what it is actual relative to.
Since it carries the end of the record itself -- as opposed to EndPos,
which is padded out to the segment boundary -- I'd suggest RecEndPos,
which matches the surrounding names:
static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
XLogRecPtr *PrevPtr, XLogRecPtr
*RecEndPos);
2) Drop the NULL guard on the out-parameter. There is a single caller and
it always passes &..., exactly like the other three out-parameters, which
are dereferenced unconditionally. Guarding only this one is inconsistent:
/* Store the end position of just the record. */
*RecEndPos = *EndPos;
To keep that unconditional store safe, the early "already at a segment
boundary" return should set it too:
if (XLogSegmentOffset(ptr, wal_segment_size) == 0)
{
SpinLockRelease(&Insert->insertpos_lck);
*EndPos = *StartPos = ptr;
*RecEndPos = ptr;
return false;
}
3) With RecEndPos always populated, the `if (inserted)` guard around the
EndPos fixup in XLogInsertRecord() is no longer needed -- when nothing was
inserted RecEndPos already equals EndPos, so the assignment is a harmless
no-op:
/*
* Even though we reserved the rest of the segment for us, which is
* reflected in EndPos, we return a pointer to just the end of the
* xlog-switch record, which is consistent with other WAL types
* returned by XLogBytePosToEndRecPtr(). When no switch record was
* inserted, RecEndPos already equals EndPos, so this is a no-op.
*/
EndPos = RecEndPos;
None of this changes behavior; it just localizes the invariant inside
ReserveXLogSwitch() and makes the out-parameters uniform. (2) and (3) are
take-it-or-leave-it -- the rename is the only one I feel strongly about.
Feel free to fold in whichever you find worthwhile for the next version.
Regards,
Henson
