scan subcompaction split point covered by range dels

[ajkr]

Fix an infinite loop in range tombstone seek-to-end-key optimization. It
happened when a range tombstone was split according to its containing
files' boundaries. In particular, the split point had to have a lower
sequence number than a point key at the same user key in a file that the
range tombstone covered.

---

This change is 

[ajkr]

I do not see a problem for backwards scanning but who knows. In that case, the internal key is forged with the tombstone seqnum, and SeekForPrev()ing to that point feels like it must make progress in moving backwards. Whereas in this bug, in forward scan we were seeking to a point that was actually before the current key, thus no progress was made.

[petermattis]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ajkr and @petermattis)

---

table/block_based_table_reader.cc, line 1795 at r1 (raw file):

  if (range_tombstone_.seq() > 0 &&
      (!Valid() ||
       icomp_.Compare(key(), tombstone_internal_end_key()) < 0)) {

Using Valid() and key() here seems suspicious as we haven't seeked the iterator yet. I'm not clear on why this portion of the change is necessary. Can you provide some commentary?

Also, is this part of the change tested in some way? Unless I'm missing something, the test you added only seems to cover the FindKeyForward problem.

[ajkr]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @petermattis)

---

table/block_based_table_reader.cc, line 1795 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Using Valid() and key() here seems suspicious as we haven't seeked the iterator yet. I'm not clear on why this portion of the change is necessary. Can you provide some commentary?

Also, is this part of the change tested in some way? Unless I'm missing something, the test you added only seems to cover the FindKeyForward problem.

I added it for symmetry. It is not needed for preventing infinite loop. It just prevents one repetitive seek in rare cases. Also it should be comparing target rather than key(). Will remove it for simplicity.

Even though we're removing it, it's still reasonable to make the test also hit the Seek() path, particularly seeking to the split point user key.

[ajkr]

TFTR. RFAL.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @petermattis)

---

table/block_based_table_reader.cc, line 1795 at r1 (raw file):

Previously, ajkr (Andrew Kryczka) wrote…

I added it for symmetry. It is not needed for preventing infinite loop. It just prevents one repetitive seek in rare cases. Also it should be comparing target rather than key(). Will remove it for simplicity.

Even though we're removing it, it's still reasonable to make the test also hit the Seek() path, particularly seeking to the split point user key.

Done.

[petermattis]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ajkr)

---

table/block_based_table_reader.cc, line 1795 at r1 (raw file):

Previously, ajkr (Andrew Kryczka) wrote…

Done.

Ack.