Fix dead tombstone-vs-expiring tie-break in cursor cell reconciliation

ReusableLivenessInfo.isExpiring() means "has an expiration time", which is also true for
tombstones (a deletion also carries a localExpirationTime). The tie-break used
!left.isExpiring() to mean "is a tombstone", so it was false for BOTH sides whenever both
already passed the earlier presence check — making the tie-break dead code and letting the
localDeletionTime comparison below it pick an expiring cell over a genuine tombstone at the
same timestamp. Tombstone semantics need ttl() == NO_TTL instead: within this code path both
sides already have localExpirationTime set, so ttl == NO_TTL is exactly "is a tombstone" and
mirrors Cells.resolveRegular's actual rule.

EdgeCaseDifferentialCompactionTest.tombstoneVsExpiringTies pins both flush orders and both
tombstone shapes (UPDATE SET v = null, DELETE v).
This commit is contained in:
Jon Haddad 2026-06-11 13:18:10 -07:00
parent adac2555b8
commit f9d9e54881
2 changed files with 46 additions and 2 deletions

View File

@ -837,8 +837,15 @@ public class CursorCompactor extends CompactionInfo.Holder
// (i.e. before expiry, the pure tombstone; after expiry, whichever is more recent)
// this inconsistency has no user-visible distinction, as at this point they are both logically tombstones
// (the only possible difference is the time at which the cells become purgeable)
boolean leftIsTombstone = !left.isExpiring(); // !isExpiring() == isTombstone(), but does not need to consider localDeletionTime()
boolean rightIsTombstone = !right.isExpiring();
// NOTE: ReusableLivenessInfo.isExpiring() means "has an expiration time", which is
// also true for tombstones !isExpiring() would be false for BOTH sides here (both
// have an expiration time once we are past the presence check above), making this
// tie-break dead and letting the localDeletionTime comparison below pick an expiring
// cell over a tombstone. Tombstone semantics need AbstractCell.isTombstone():
// within this block both sides have localExpirationTime set, so ttl == NO_TTL is
// exactly "is a tombstone" (mirrors Cells.resolveRegular's !left.isExpiring()).
boolean leftIsTombstone = left.ttl() == LivenessInfo.NO_TTL;
boolean rightIsTombstone = right.ttl() == LivenessInfo.NO_TTL;
if (leftIsTombstone != rightIsTombstone)
return leftIsTombstone ? LEFT : RIGHT;

View File

@ -538,6 +538,43 @@ public class EdgeCaseDifferentialCompactionTest extends DifferentialCompactionTe
assertCursorMatchesIteratorAcrossGenerations(cfs);
}
/**
* Cell TOMBSTONE vs EXPIRING cell at the SAME timestamp: rule (b) of the reference
* decision table (Cells.resolveRegular) the tombstone wins the tie, BEFORE any
* localDeletionTime comparison. This is the one same-timestamp pairing where both
* sides carry a localExpirationTime (tombstone: deletion second; expiring: expiry
* second), so a resolver that classifies "tombstone" via the loose
* ReusableLivenessInfo.isExpiring() predicate (true for tombstones too) never
* fires rule (b) and falls through to the ldt compare,
* which the expiring cell's future expiry second wins: deleted data resurrected
* until the TTL lapses. Both flush orders and both tombstone shapes
* (UPDATE SET v = null, DELETE v) across distinct partitions.
*/
@Test
public void tombstoneVsExpiringTies() throws Exception
{
createTable("CREATE TABLE %s (pk bigint, ck bigint, v text, PRIMARY KEY (pk, ck)) " +
"WITH gc_grace_seconds = 864000");
ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
cfs.disableAutoCompaction();
// pk 1: tombstone flushed first, expiring second; tombstone via UPDATE SET null
for (long ck = 0; ck < 5; ck++)
execute("UPDATE %s USING TIMESTAMP 5000 SET v = null WHERE pk = ? AND ck = ?", 1L, ck);
// pk 2: expiring flushed first, tombstone second; tombstone via DELETE column
for (long ck = 0; ck < 5; ck++)
execute("UPDATE %s USING TTL 86400 AND TIMESTAMP 5000 SET v = ? WHERE pk = ? AND ck = ?", "live" + ck, 2L, ck);
flush();
for (long ck = 0; ck < 5; ck++)
execute("UPDATE %s USING TTL 86400 AND TIMESTAMP 5000 SET v = ? WHERE pk = ? AND ck = ?", "live" + ck, 1L, ck);
for (long ck = 0; ck < 5; ck++)
execute("DELETE v FROM %s USING TIMESTAMP 5000 WHERE pk = ? AND ck = ?", 2L, ck);
flush();
assertCursorMatchesIteratorAcrossGenerations(cfs);
}
/** Vector and duration columns: fixed-dimension float vectors and the
* variable-length duration encoding as ordinary single cells, overwritten and
* null-overwritten (cell tombstone) across sstables. */