-
Type:
Improvement
-
Resolution: Done
-
Priority:
Major - P3
-
None
-
Affects Version/s: None
-
Component/s: None
-
Labels:None
-
0
-
Storage - Ra 2020-12-28
In reviewing the reconciliation visibility code I noticed a refactoring change that seems to have altered a conditional, and there isn't an explanation as to why.
The code in question is:
static inline bool __rec_need_save_upd( WT_SESSION_IMPL *session, WT_RECONCILE *r, WT_UPDATE_SELECT *upd_select, bool has_newer_updates) { ... if (F_ISSET(r, WT_REC_EVICT) && has_newer_updates) return (true); ... /* When in checkpoint, no need to save update if no onpage value is selected. */ if (F_ISSET(r, WT_REC_CHECKPOINT) && upd_select->upd == NULL) return (false); ... }
It is called with:
if (__rec_need_save_upd(session, r, upd_select, has_newer_updates)
The code was changed in WT-5390, the earlier version of the code looked like:
/*
* If we're doing checkpoint reconciliation, and the update doesn't have any further updates
* that need to be written to the history store, skip saving the update as saving the update
* will cause reconciliation to think there is work that needs to be done when there might
* not be.
*
* Additionally if we have arrived here via checkpoint but lookaside reconciliation is not
* set skip saving an update.
*/
if (F_ISSET(r, WT_REC_EVICT) ||
(F_ISSET(r, WT_REC_CHECKPOINT) && F_ISSET(r, WT_REC_LOOKASIDE) &&
upd_select->upd != NULL && upd_select->upd->next != NULL)) {
The additional check for WT_REC_LOOKASIDE isn't interesting - we changed to be always doing the equivalent of that. The additional check for upd_select->upd->next != NULL is missing in the new code, and the context of the comment was stripped down significantly.
I believe that the relaxed check means that we will restore updates more often than necessary, so it should just be an efficiency problem. I don't fully understand though, so we should check the code and make sure it works as expected.