From aec90a22da1b53ee3decda1c32f1767c60ac8d89 Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Wed, 13 Jul 2016 09:05:55 +0300 Subject: [PATCH] sstable parsing: assert we do not lose clustering rows The sstable parsing code calls mp_row_consumer::flush() after every clustering row has been read, and this puts the now complete row in a single field "_ready". The assumption is that at this point parsing will stop, the consumer will move out this _ready (mp_row_consumer::get_mutation_fragment()) and when flush() is later called again, _ready will be empty again. This assumption is correct in our code, but is based on an intricate combination of estoreric parts of the code, such as: 1. In data_consume_row_context we stop parsing after reading the parition's header, before reading any clustering rows, giving the caller the chance to call sstable_streamed_mutation::read_next() to be prepared for the incoming mutations. 2. In mp_row_consumer::flush_if_needed(), we stop the parser after each individual clustering row. It is easy to break this assumption, and I did this in one of my code changes, and the result was silent loss of clustering rows, as "_ready" got silently overwritten before the reader had a chance to move it out. What this patch does is to add an assertion: If a clustering row is silently lost before being transferred to the mutation fragment reader, we croak. Signed-off-by: Nadav Har'El Message-Id: <1468389955-24600-1-git-send-email-nyh@scylladb.com> --- sstables/partition.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sstables/partition.cc b/sstables/partition.cc index 2cad89c150..77678c2c30 100644 --- a/sstables/partition.cc +++ b/sstables/partition.cc @@ -320,6 +320,10 @@ public: void flush() { flush_pending_collection(*_schema); + // If _ready is already set we have a bug: get_mutation_fragment() + // was not called, and below we will lose one clustering row! + assert(!_ready); + assert(!_range_tombstone_end_ready); if (!_skip_clustering_row) { _ready = move_and_disengage(_in_progress); _range_tombstone_end_ready = move_and_disengage(_range_tombstone_end_in_progress);