fix: handle non-flat row id vectors when replaying RTREE index appends and deletes from the WAL

[darrenspurgeon]

Fixes #861.

Problem

If a database file is closed without checkpointing while its WAL contains inserts into a table with an RTREE index, then on the next open:

• any statement that binds the index fails with INTERNAL Error: Operation requires a flat vector but a non-flat vector was encountered
• CHECKPOINT fails with the same error, after which DuckDB reports the database as invalidated

It doesn't appear to clear itself. Every subsequent open replays the same WAL and hits the same failure. The data itself seems intact — SELECT count(*) returns every row and a read-only open reads everything — but the file can't checkpoint again until someone drops the index by hand.

A single inserted row is enough to reproduce:

-- Session 1
PRAGMA disable_checkpoint_on_shutdown;
LOAD spatial;
CREATE TABLE t (geom GEOMETRY);
CREATE INDEX i ON t USING RTREE (geom);
INSERT INTO t VALUES (ST_Point(0,0));

-- Session 2, same file
LOAD spatial;
CHECKPOINT;
-- FATAL Error: Failed to create checkpoint because of error:
--   Operation requires a flat vector but a non-flat vector was encountered

Root cause

When the WAL is replayed without the spatial extension loaded, the RTREE index cannot be bound, so DuckDB buffers the appends and deletes and applies them later after something binds the index. BoundIndex::ApplyBufferedReplays slices the buffered chunk to build the row id vector:

SelectionVector sel(offset_in_chunk, rows_to_process);
...
Vector row_ids(state.current_chunk.data.back(), sel, rows_to_process);
const auto error = Append(table_chunk, row_ids, append_info);

That produces a dictionary vector. ConvertToEntries read it with FlatVector::Validity and FlatVector::GetData, which throw on anything non-flat.

ART indexes may survive the same replay path because ART::GenerateKeyVectors goes through UnifiedVectorFormat, which is why the failure should be specific to RTREE.

Fix

• ConvertToEntries reads the row ids through UnifiedVectorFormat, indexing both the data and the validity mask through the selection vector. This covers Insert and Delete, which share the helper.
• key_chunk.SetCardinality(count) before key_chunk.Flatten() in Insert and Delete. DataChunk::Flatten() flattens up to size(), which is 0 right after Reset(), so the flatten didn't do anything. I don't see a known trigger, since the key vector comes back flat on both paths, but the call was not doing what it reads like it should be doing.

Test

test/sql/index/rtree_persistence_wal_append.test covers both the append and the delete replay: create the index, write, restart, then index scan and CHECKPOINT. It fails before this change and passes after.

Two notes on the test:

• The existing rtree_persistence_wal.test didn't look like it was actually exercising WAL replay. The sqllogictest runner defaults checkpoint_wal_size to 0 (TestConfiguration::GetCheckpointWALSize), so it checkpoints as soon as the WAL is non-empty and no .wal file survives the restart. The new test raises checkpoint_threshold after every restart, which is what keeps the WAL alive into the next session. I left the existing test alone, but certainly they can be merged.
• The delete assertions read the index directly with rtree_index_dump instead of only counting table rows. Deleted rows are filtered out when the index scan fetches them from the table, so a table-level count passes whether or not the entries were actually removed from the index.

Related

• Internal error when maintaining multiple connections while using RTREE index duckdb-rs#733 looks to be the same error and same stack, reported against v1.5.1 on linux_amd64. Filed on the Rust binding repo; the reproduction there goes through Rust and deliberately leaks a connection, which made it look like a connection-lifetime problem rather than an uncheckpointed WAL.
• Is RTree index thread safe? #706 appears to be unrelated despite also involving RTreeIndex::Append. Its stack crashes inside RTree::SplitNode / SortEntriesByRowId on the ordinary LocalTableStorage::AppendToIndexes path, which points at concurrent tree mutation rather than vector layout.

[Maxxen]

This doesn't really make sense to me - calling .Flatten() here should ensure the box_vec is a flat vector, making ConvertToEntries safe as is

[Maxxen]

Hello! Thanks for digging into this and thanks for this PR!
Nevermind the above comment - this seems correct to me.