Skip to content

Commit 5870fb2

Browse files
authored
[fix](be) Release SNII writer reservations on auxiliary failure (#66855)
### What problem does this PR solve? Issue Number: N/A Related PR: #66052 Problem Summary: SNII adopted a `LogicalIndexWriter` into `SniiCompoundWriter::indexes_` before appending its norms, null bitmap, and block-split bloom-filter sections. An injected append failure at those three boundaries reproduced retained `MemoryReporter` charges of 96, 86, and 60 bytes respectively after `add_logical_index()` returned. The ordinary build caller transfers reporter ownership only after that call succeeds, so failure teardown could leave the compound writer holding reservations that refer to an already-destroyed reporter. This change keeps the logical writer and its placement local while writing all auxiliary sections, then adopts both into the compound writer only after every append succeeds. The same ownership rule is applied to the streamed path. Poisoning behavior and the successful append order, offsets, file layout, and bytes are unchanged, so this does not change the SNII storage format and does not require rebuilding existing indexes.
1 parent 78a3920 commit 5870fb2

3 files changed

Lines changed: 126 additions & 40 deletions

File tree

be/src/storage/index/snii/writer/snii_compound_writer.cpp

Lines changed: 30 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,12 @@ Status SniiCompoundWriter::add_logical_index(const SniiIndexInput& in) {
173173
status = liw->stream_dict_region_into(out_);
174174
if (!status.ok()) return poison(status);
175175
p.dict_len = out_->bytes_written() - p.dict_off;
176+
status = write_index_aux_sections(*liw, p);
177+
if (!status.ok()) {
178+
return poison(status);
179+
}
176180
indexes_.push_back(std::move(liw));
177181
placements_.push_back(p);
178-
// liw has been moved from; write_index_aux_sections works off indexes_.back().
179-
status = write_index_aux_sections(indexes_.size() - 1);
180-
if (!status.ok()) return poison(status);
181182
return Status::OK();
182183
}
183184

@@ -479,18 +480,15 @@ Status SniiCompoundWriter::finish_streamed_index(SniiStreamedIndexSession* sessi
479480
status = session->writer_->stream_dict_region_into(out_);
480481
if (!status.ok()) return poison(status);
481482
p.dict_len = out_->bytes_written() - p.dict_off;
482-
// The index joins the container (indexes_/placements_) here, but session->finished_
483-
// is not set until write_index_aux_sections below also succeeds. A failure ANYWHERE
484-
// in this function -- finish_streamed()/stream_dict_region_into() above, or
485-
// write_index_aux_sections below -- calls poison(), which sets failed_ before
486-
// returning. finish() checks "if (!failed_.ok()) return failed_;" ahead of its
487-
// has_active_session() gate, so a poisoned writer fails loudly on its own; it can
488-
// never fall through to sealing a tail that silently omits an index whose posting
489-
// bytes are already in the file.
483+
// The index joins the container only after every section succeeds. A failure
484+
// anywhere in this function poisons the compound writer, so finish() cannot seal
485+
// a tail that omits posting bytes already written to the file.
486+
status = write_index_aux_sections(*session->writer_, p);
487+
if (!status.ok()) {
488+
return poison(status);
489+
}
490490
indexes_.push_back(std::move(session->writer_));
491491
placements_.push_back(p);
492-
status = write_index_aux_sections(indexes_.size() - 1);
493-
if (!status.ok()) return poison(status);
494492
session->finished_ = true;
495493
return Status::OK();
496494
}
@@ -506,29 +504,25 @@ Status SniiCompoundWriter::write_bootstrap() {
506504
// Writes one index's norms / null bitmap / bsbf directly after its [posting][dict] pair.
507505
// Bytes are released as soon as they are on disk rather than being held until finish(),
508506
// which also lowers import peak memory -- a content column's bsbf runs to MBs.
509-
Status SniiCompoundWriter::write_index_aux_sections(size_t index) {
510-
DORIS_CHECK_LT(index, indexes_.size());
511-
DORIS_CHECK_LT(index, placements_.size());
512-
LogicalIndexWriter& w = *indexes_[index];
513-
Placement& p = placements_[index];
514-
515-
if (w.has_norms() && !w.norms_bytes().empty()) {
516-
p.norms_off = out_->bytes_written();
517-
RETURN_IF_ERROR(append(w.norms_bytes()));
518-
p.norms_len = out_->bytes_written() - p.norms_off;
519-
w.release_norms_bytes();
520-
}
521-
if (w.has_null_bitmap()) {
522-
p.null_off = out_->bytes_written();
523-
RETURN_IF_ERROR(append(w.null_bitmap_bytes()));
524-
p.null_len = out_->bytes_written() - p.null_off;
525-
w.release_null_bitmap_bytes();
526-
}
527-
if (w.has_bsbf()) {
528-
p.bsbf_off = out_->bytes_written();
529-
RETURN_IF_ERROR(append(w.bsbf_bytes()));
530-
p.bsbf_len = out_->bytes_written() - p.bsbf_off;
531-
w.release_bsbf_bytes();
507+
Status SniiCompoundWriter::write_index_aux_sections(LogicalIndexWriter& writer,
508+
Placement& placement) {
509+
if (writer.has_norms() && !writer.norms_bytes().empty()) {
510+
placement.norms_off = out_->bytes_written();
511+
RETURN_IF_ERROR(append(writer.norms_bytes()));
512+
placement.norms_len = out_->bytes_written() - placement.norms_off;
513+
writer.release_norms_bytes();
514+
}
515+
if (writer.has_null_bitmap()) {
516+
placement.null_off = out_->bytes_written();
517+
RETURN_IF_ERROR(append(writer.null_bitmap_bytes()));
518+
placement.null_len = out_->bytes_written() - placement.null_off;
519+
writer.release_null_bitmap_bytes();
520+
}
521+
if (writer.has_bsbf()) {
522+
placement.bsbf_off = out_->bytes_written();
523+
RETURN_IF_ERROR(append(writer.bsbf_bytes()));
524+
placement.bsbf_len = out_->bytes_written() - placement.bsbf_off;
525+
writer.release_bsbf_bytes();
532526
}
533527
return Status::OK();
534528
}

be/src/storage/index/snii/writer/snii_compound_writer.h

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -279,12 +279,11 @@ class SniiCompoundWriter {
279279

280280
Status ensure_bootstrap();
281281
Status write_bootstrap();
282-
// Writes indexes_[index]'s norms/null-bitmap/bsbf immediately after its
283-
// [posting][dict] pair and fills placements_[index]. Keeping one index's sections
282+
// Writes one index's norms/null-bitmap/bsbf immediately after its
283+
// [posting][dict] pair and fills its placement. Keeping one index's sections
284284
// contiguous is what makes a single-index cold query touch one cache block instead
285285
// of three; the previous layout grouped these by section type across all indexes.
286-
// Must be called after indexes_/placements_ have been pushed for this index.
287-
Status write_index_aux_sections(size_t index);
286+
Status write_index_aux_sections(LogicalIndexWriter& writer, Placement& placement);
288287
Status write_tail();
289288
Status append(const std::vector<uint8_t>& bytes);
290289
Status poison(Status status);

be/test/storage/index/snii/writer/snii_compound_writer_test.cpp

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
#include "storage/index/snii/format/frq_prelude.h"
4848
#include "storage/index/snii/format/metadata_blob.h"
4949
#include "storage/index/snii/format/metadata_directory.h"
50+
#include "storage/index/snii/format/null_bitmap.h"
5051
#include "storage/index/snii/format/prx_pod.h"
5152
#include "storage/index/snii/format/sampled_term_index.h"
5253
#include "storage/index/snii/format/tail_pointer.h"
@@ -336,6 +337,55 @@ class FailOnAppendWriter final : public io::FileWriter {
336337
std::vector<uint8_t> bytes_;
337338
};
338339

340+
enum class AuxiliarySection { kNorms, kNullBitmap, kBsbf };
341+
342+
class FailOnAuxiliarySectionWriter final : public io::FileWriter {
343+
public:
344+
explicit FailOnAuxiliarySectionWriter(AuxiliarySection target) : target_(target) {}
345+
346+
Status append(Slice data) override {
347+
if (is_target(data)) {
348+
++target_append_calls_;
349+
return Status::Error<doris::ErrorCode::IO_ERROR, false>(
350+
"injected auxiliary section append failure");
351+
}
352+
bytes_.insert(bytes_.end(), data.data(), data.data() + data.size());
353+
return Status::OK();
354+
}
355+
356+
Status finalize() override { return Status::OK(); }
357+
358+
uint64_t bytes_written() const override { return bytes_.size(); }
359+
size_t target_append_calls() const { return target_append_calls_; }
360+
361+
private:
362+
bool is_target(Slice data) const {
363+
if (target_ == AuxiliarySection::kBsbf) {
364+
if (data.size() < kBsbfHeaderSize) {
365+
return false;
366+
}
367+
BsbfHeader header;
368+
return BsbfHeader::parse(data.subslice(0, kBsbfHeaderSize), bytes_.size(), &header)
369+
.ok() &&
370+
data.size() == kBsbfHeaderSize + header.num_bytes;
371+
}
372+
373+
ByteSource source(data);
374+
FramedSection section;
375+
if (!SectionFramer::read(source, &section).ok() || source.remaining() != 0) {
376+
return false;
377+
}
378+
const uint8_t target_type = target_ == AuxiliarySection::kNorms
379+
? static_cast<uint8_t>(SectionType::kNormsPod)
380+
: kNullBitmapSectionType;
381+
return section.type == target_type;
382+
}
383+
384+
AuxiliarySection target_;
385+
size_t target_append_calls_ = 0;
386+
std::vector<uint8_t> bytes_;
387+
};
388+
339389
void VerifyAppendFailurePoisonsWriter(size_t fail_on_append) {
340390
FailOnAppendWriter file(fail_on_append);
341391
SniiCompoundWriter writer(&file);
@@ -487,6 +537,37 @@ SniiIndexInput MakeIndex(uint64_t index_id, const std::string& suffix, uint32_t
487537
return in;
488538
}
489539

540+
SniiIndexInput MakeIndexWithAllAuxiliarySections(MemoryReporter* reporter) {
541+
SniiIndexInput in;
542+
in.index_id = 7;
543+
in.index_suffix = "body";
544+
in.config = IndexConfig::kDocsPositionsScoring;
545+
in.doc_count = 3;
546+
in.null_docids = {2};
547+
in.encoded_norms = {1, 2, 3};
548+
in.terms.push_back(MakeTerm("apple", {0, 1}, true));
549+
in.mem_reporter = reporter;
550+
return in;
551+
}
552+
553+
void VerifyAuxiliaryAppendFailureReleasesReservations(AuxiliarySection target) {
554+
FailOnAuxiliarySectionWriter file(target);
555+
auto reporter = std::make_unique<MemoryReporter>();
556+
auto compound = std::make_unique<SniiCompoundWriter>(&file);
557+
const SniiIndexInput input = MakeIndexWithAllAuxiliarySections(reporter.get());
558+
559+
const Status status = compound->add_logical_index(input);
560+
ASSERT_FALSE(status.ok());
561+
ASSERT_EQ(1U, file.target_append_calls());
562+
ASSERT_EQ(0, reporter->current_bytes())
563+
<< "failed logical writer retained reservations after add_logical_index returned";
564+
565+
// The caller only transfers reporter ownership after add_logical_index succeeds.
566+
// Exercise the real failed-call teardown order under ASAN: the reporter dies first.
567+
reporter.reset();
568+
compound.reset();
569+
}
570+
490571
// Locate a term through the full reader walk and return its DictEntry.
491572
Status LocateEntry(const std::vector<uint8_t>& file, const SampledTermIndexReader& sti,
492573
const DictBlockDirectoryReader& dbd, const std::string& term, bool* found,
@@ -933,6 +1014,18 @@ TEST(SniiCompoundWriter, AppendFailurePoisonsWriterBeforeAnyValidFooter) {
9331014
}
9341015
}
9351016

1017+
TEST(SniiCompoundWriter, NormsAppendFailureReleasesReservationsBeforeReturn) {
1018+
VerifyAuxiliaryAppendFailureReleasesReservations(AuxiliarySection::kNorms);
1019+
}
1020+
1021+
TEST(SniiCompoundWriter, NullBitmapAppendFailureReleasesReservationsBeforeReturn) {
1022+
VerifyAuxiliaryAppendFailureReleasesReservations(AuxiliarySection::kNullBitmap);
1023+
}
1024+
1025+
TEST(SniiCompoundWriter, BsbfAppendFailureReleasesReservationsBeforeReturn) {
1026+
VerifyAuxiliaryAppendFailureReleasesReservations(AuxiliarySection::kBsbf);
1027+
}
1028+
9361029
TEST(SniiCompoundWriter, ReopeningLogicalReaderClearsPreviousCommonGramsState) {
9371030
auto with_common_grams = EmptyIndex(7, "with");
9381031
with_common_grams.common_grams_metadata = CompleteCommonGramsMetadata();

0 commit comments

Comments
 (0)