WT-12096 Don't validate timestamp against the on-page value if the update is globally visible

[tod-johnson-mongodb]

If an update is globally visible it can be evicted and EBUSY should not be returned. Thus the check for EBUSY should be skipped if an update is globally visible. The bug is the check for EBUSY was not skipped if an update is globally visible.

Please review just all commits. There were temporary changes that have been backed out.

[sueloverso]

@tod-johnson-mongodb do you want me to run this branch against the format reproducer I've used so far?

@sueloverso github won't let me add a reply. So I am editing the comment.
You can do so. However, there have been some source changes, not mine, that do not build with the v3 toolchain you have. They require the v4 toolchain. If you want to do that work, then go ahead and run this branch. You could also run the new test wiredtiger/test/suite/test_bug033.py.

The error message from not using the v4 toolchain on file wiredtiger/bench/workgen/workgen.c
../bench/workgen/workgen.cpp:43:10: fatal error: filesystem: No such file or directory

[sueloverso]

My build on kodkod-aws completes. If there is a failure in building I don't see it and the process exits with success (unlike say a compilation error). I have started the format run.

[sueloverso]

This is not code that I know well enough to say this is the correct fix. So I removed myself as reviewer and added @luke-pearson.

I will say that my repro that readily failed has run for the last 4 days, over 2000 iterations, which is far longer than any of the failure runs took to hit. So that is a positive.

[sueloverso]

The test failure in PR testing should be investigated to see if it is related to this change/fix since the test has to do with updates and tombstones (based on the name).

[luke-pearson]

This is a reconciliation change that should have a lot of eyes on it, I've added Chenhao.

[quchenhao]

evicted is not the right word. I think it should be the following updates can be removed by obsolete check.

[tod-johnson-mongodb]

Why is evicted not the right word? That is the word used throughout this function. I will talk with you about this.

 381      * If eviction reconciliation starts before checkpoint, it is fine to evict updates without
 382      * timestamps.

 389         WT_STAT_CONN_DATA_INCR(session, cache_eviction_blocked_remove_hs_race_with_checkpoint);

 401         WT_STAT_CONN_DATA_INCR(session, cache_eviction_blocked_no_ts_checkpoint_race_2);

 433             WT_STAT_CONN_DATA_INCR(session, cache_eviction_blocked_no_ts_checkpoint_race_4);

 475          * If prev_upd is globally visible it can be evicted. Thus this check applies only if
 476          * prev_upd is not globally visible.

 482             WT_STAT_CONN_DATA_INCR(session, cache_eviction_blocked_no_ts_checkpoint_race_1);

[quchenhao]

obsolete check may happen at any time not necessarily in eviction.

[quchenhao]

Rollback to stable may recover updates from the history store that is out of order to the on-disk value. Normally these updates have the WT_UPDATE_RESTORED_FROM_HS flag on them. However, in rare cases, if the newer update becomes globally visible, the restored update may be removed by the obsolete check. This may lead to an out of order edge case but it is benign. Check the global visibility of the update and ignore this case.

[tod-johnson-mongodb]

Done

[quchenhao]

Me and tod discussed about changing these assert as

if (WT_TIME_WINDOW_HAS_STOP(&vpack->tw))
...
else
...

This improves readability

[tod-johnson-mongodb]

Not done.

1. When we discussed this I misread this. The first WT_ASSERT_ALWAYS() is checking vpack->tw.durable_start_ts. The second WT_ASSERT_ALWAYS() is checking vpack->tw.durable_stop_ts.

2. If WT_ASSERT_ALWAYS() fails the assert condition is printed. Moving part of the condition out of the WT_ASSERT_ALWAYS() prints just part of the assert condition if the assert fails.

I did change the asserts to have different comments.

[quchenhao]

The part that is moved out of the assert is not important.

[quchenhao]

I thought we agree to do

  if (WT_TIME_WINDOW_HAS_STOP(&vpack->tw))
            WT_ASSERT_ALWAYS(session,
               prev_upd->prepare_state == WT_PREPARE_INPROGRESS || prev_upd->start_ts == prev_upd->durable_ts ||
                prev_upd->durable_ts >= vpack->tw.durable_stop_ts,
              "Stop: Durable timestamps cannot be out of order for prepared updates");
   else
             WT_ASSERT_ALWAYS(session, prev_upd->prepare_state == WT_PREPARE_INPROGRESS || prev_upd->start_ts == prev_upd->durable_ts || prev_upd->durable_ts >= vpack->tw.durable_start_ts,
              "Start: Durable timestamps cannot be out of order for prepared updates");

[luke-pearson]

Wouldn't this be best?

        if (prev_upd->prepare_state != WT_PREPARE_INPROGRESS && prev_upd->start_ts != prev_upd->durable_ts) {
            WT_ASSERT_ALWAYS(session, prev_upd->durable_ts >= vpack->tw.durable_start_ts && (!WT_TIME_WINDOW_HAS_STOP(&vpack->tw) ||  prev_upd->durable_ts >= vpack->tw.durable_stop_ts));

        }

[quchenhao]

I prefer my version because I think the code is clearer in that way. We compare the stop time pair if on the disk image we have one. Otherwise, we compare with the start time pair.

[luke-pearson]

Okay fair enough.

[tod-johnson-mongodb]

Done

[luke-pearson]

This solution skips comparing the last update in the update chain with the on disk update if the last update is globally visible. This ignores the fact that it may have already walked obsolete updates prior to that. Would it not be better if the loop on line 420 became:

    /* Loop forward from update after the selected on-page update. */
    for (prev_upd = select_upd, upd = select_upd->next; upd != NULL; upd = upd->next) {
        if (upd->txnid == WT_TXN_ABORTED)
            continue;
        WT_ASSERT_ALWAYS(session,
          prev_upd->prepare_state == WT_PREPARE_INPROGRESS ||
            prev_upd->start_ts == prev_upd->durable_ts || prev_upd->durable_ts >= upd->durable_ts,
          "Durable timestamps cannot be out of order for prepared updates");
        /* Validate that the updates older than us have older timestamps. */
        if (prev_upd->start_ts < upd->start_ts) {
            WT_ASSERT_ALWAYS(
              session, prev_upd->start_ts == WT_TS_NONE, "Previous update missing start timestamp");
            WT_STAT_CONN_DATA_INCR(session, cache_eviction_blocked_no_ts_checkpoint_race_4);
            return (EBUSY);
        }
        /*
         * Rollback to stable may restore older updates from the data store or history store. In
         * this case, the restored update has older update than the onpage value, which is expected.
         * Reconciliation may restore the onpage value to the update chain. In this case, no need to
         * check further as the value is the same as the onpage value. If we have a committed
         * prepared update restored from the onpage value, no need to check further as well because
         * the update chain after it should only contain committed prepared updates from the same
         * transaction.
         */
        if (F_ISSET(upd,
              WT_UPDATE_RESTORED_FROM_DS | WT_UPDATE_RESTORED_FROM_HS |
                WT_UPDATE_PREPARE_RESTORED_FROM_DS))
            return (0);

        /* Don't walk obsolete updates. */
        if (__wt_txn_visible_all(session, upd))
            return (0);

        prev_upd = upd;
    }

The only concern I have with this solution is that that globally visible update could still be OOO with respect to the on disk update but it shouldn't matter as its globally visible?

[quchenhao]

This solution skips comparing the last update in the update chain with the on disk update if the last update is globally visible. This ignores the fact that it may have already walked obsolete updates prior to that. Would it not be better if the loop on line 420 became:

    /* Loop forward from update after the selected on-page update. */
    for (prev_upd = select_upd, upd = select_upd->next; upd != NULL; upd = upd->next) {
        if (upd->txnid == WT_TXN_ABORTED)
            continue;
        WT_ASSERT_ALWAYS(session,
          prev_upd->prepare_state == WT_PREPARE_INPROGRESS ||
            prev_upd->start_ts == prev_upd->durable_ts || prev_upd->durable_ts >= upd->durable_ts,
          "Durable timestamps cannot be out of order for prepared updates");
        /* Validate that the updates older than us have older timestamps. */
        if (prev_upd->start_ts < upd->start_ts) {
            WT_ASSERT_ALWAYS(
              session, prev_upd->start_ts == WT_TS_NONE, "Previous update missing start timestamp");
            WT_STAT_CONN_DATA_INCR(session, cache_eviction_blocked_no_ts_checkpoint_race_4);
            return (EBUSY);
        }
        /*
         * Rollback to stable may restore older updates from the data store or history store. In
         * this case, the restored update has older update than the onpage value, which is expected.
         * Reconciliation may restore the onpage value to the update chain. In this case, no need to
         * check further as the value is the same as the onpage value. If we have a committed
         * prepared update restored from the onpage value, no need to check further as well because
         * the update chain after it should only contain committed prepared updates from the same
         * transaction.
         */
        if (F_ISSET(upd,
              WT_UPDATE_RESTORED_FROM_DS | WT_UPDATE_RESTORED_FROM_HS |
                WT_UPDATE_PREPARE_RESTORED_FROM_DS))
            return (0);

        /* Don't walk obsolete updates. */
        if (__wt_txn_visible_all(session, upd))
            return (0);

        prev_upd = upd;
    }

The only concern I have with this solution is that that globally visible update could still be OOO with respect to the on disk update but it shouldn't matter as its globally visible?

This can work because we are here only in eviction. My concern is that checking global visibility for each update is expensive.

globally visible update could still be OOO with respect to the on disk update but it shouldn't matter as its globally visible

This is OK and it is exactly the exception case that is false positive.

[luke-pearson]

My concern is that checking global visibility for each update is expensive.

I suppose that is true, we can definitely reduce the number of checks by keeping only one... I guess for performance reasons we'll only do one check but it should be detailed in a comment why we chose to do it that way @tod-johnson-mongodb .

[quchenhao]

lgtm

[evergreen-ci-prod]

Test coverage is too low, this change probably shouldn't be merged as-is.

Metric (for added/changed code)	Coverage
Line coverage	71% (5/7)
Branch coverage	40% (12/30)

[luke-pearson]

LGTM.

[quchenhao]

@tod-johnson-mongodb Are you ready to merge this?

[tod-johnson-mongodb]

I am merging this. I have two approvals. Sue said she didn't need to re-review since I already had two approvals. I didn't hear whether Sean wanted to re-review.