storage: call out caveats for MVCCIterator timestamp hints

This patch calls out some subtle issues with `MVCCIterator` timestamp
hints that may cause them to return stale data.

Release note: None

pkg/storage/engine.go

@@ -384,18 +384,29 @@ type IterOptions struct {
 	// of the time range formed by [MinTimestampHint, MaxTimestampHint] do not
 	// need to be presented by the iterator. The underlying iterator may be able
 	// to efficiently skip over keys outside of the hinted time range, e.g., when
-	// an SST indicates that it contains no keys within the time range.
+	// an SST indicates that it contains no keys within the time range. Intents
+	// will not be visible to such iterators at all. This is only relevant for
+	// MVCCIterators.
 	//
 	// Note that time bound hints are strictly a performance optimization, and
 	// iterators with time bounds hints will frequently return keys outside of the
 	// [start, end] time range. If you must guarantee that you never see a key
 	// outside of the time bounds, perform your own filtering.
 	//
-	// These fields are only relevant for MVCCIterators. Additionally, an
-	// MVCCIterator with timestamp hints will not see separated intents, and may
-	// not see some interleaved intents. Currently, the only way to correctly
-	// use such an iterator is to use it in concert with an iterator without
-	// timestamp hints, as done by MVCCIncrementalIterator.
+	// NB: The iterator may surface stale data. Pebble range tombstones do not have
+	// timestamps and thus may be ignored entirely depending on whether their SST
+	// happens to satisfy the filter. Furthermore, keys outside the timestamp
+	// range may be stale and must be ignored -- for example, consider a key foo@5
+	// written in an SST with timestamp range [3-7], and then a non-MVCC removal
+	// or update of this key in a different SST with timestamp range [3-5]. Using
+	// an iterator with range [6-9] would surface the old foo@5 key because it
+	// would return all keys in the old [3-7] SST but not take into account the
+	// separate [3-5] SST where foo@5 was removed or updated. See also:
+	// https://github.com/cockroachdb/pebble/issues/1786
+	//
+	// Currently, the only way to correctly use such an iterator is to use it in
+	// concert with an iterator without timestamp hints, as done by
+	// MVCCIncrementalIterator.
 	MinTimestampHint, MaxTimestampHint hlc.Timestamp
 	// KeyTypes specifies the types of keys to surface: point and/or range keys.
 	// Use HasPointAndRange() to determine which key type is present at a given