Core: Don't clear snapshot log when intermediate snapshots are detected

[nastra]

fixes #5442

[rdblue]

This problem was introduced in #3664. The original code that this comment applies to cleared the log when it encountered a snapshot that has been expired and removed. That's the correct behavior.

The problem is that I combined that behavior with the check to suppress intermediate snapshots in a transaction. We should suppress those snapshots in the log, but we don't need to clear the log. Here's what I think should happen instead:

        if (snapshotsById.containsKey(snapshotId)) {
          if (!intermediateSnapshotIds.contains(snapshotId)) {
            // copy the log entries that are still valid
            newSnapshotLog.add(logEntry);
          }
        } else {
          // any invalid entry causes the history before it to be removed. otherwise, there could be
          // history gaps that cause time-travel queries to produce incorrect results. for example,
          // if history is [(t1, s1), (t2, s2), (t3, s3)] and s2 is removed, the history cannot be
          // [(t1, s1), (t3, s3)] because it appears that s3 was current during the time between t2
          // and t3 when in fact s2 was the current snapshot.
          newSnapshotLog.clear();
        }

[kbendick]

💯 This looks correct to me.

[nastra]

makes total sense, thanks

[kbendick]

This LGTM once tests pass.

Considering that this change reverts an earlier refactor, I don't think that additional manual testing is necessarily needed.

[kbendick]

You might consider asserting on the entire history that should be there -- which should be the initial history + the one entry post txn.commitTransaction().

[rdblue]

Thanks for fixing this, @nastra!