View building crashes on large partitions with range tombstones. #14503

michoecho · 2023-07-04T12:51:13Z

View update routines accept mutation objects.
But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects.
To build view updates after a repair/streaming, we have to convert the fragment stream into mutations. This is done by piping the stream to mutation_rebuilder_v2.

To keep memory usage limited, the stream for a single partition might have to be split into multiple partial mutation objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error.

View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes scylladb#14503

michoecho · 2023-07-04T12:54:29Z

Refs scylladb/scylla-enterprise#3072

View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes scylladb#14503

…andom_mutations A random mutation test for view_updating_consumer's buffering logic. Reproduces scylladb#14503.

…ator_buffering This patch adds a full-range tombstone to the compacted mutation. This raises the coverage of the test. In particular, it reproduces issue scylladb#14503, which should have been caught by this test, but wasn't.

…_consumer' from Michał Chojnowski View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes #14503 Closes #14502 * github.com:scylladb/scylladb: test: view_build_test: add range tombstones to test_view_update_generator_buffering test: view_build_test: add test_view_udate_generator_buffering_with_random_mutations view_updating_consumer: make buffer limit a variable view: fix range tombstone handling on flushes in view_updating_consumer

View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes scylladb#14503

…andom_mutations A random mutation test for view_updating_consumer's buffering logic. Reproduces scylladb#14503.

…ator_buffering This patch adds a full-range tombstone to the compacted mutation. This raises the coverage of the test. In particular, it reproduces issue scylladb#14503, which should have been caught by this test, but wasn't.

michoecho · 2023-07-11T07:06:57Z

@scylladb/scylla-maint Pinging for backports.

nyh · 2023-07-11T07:17:00Z

@scylladb/scylla-maint Pinging for backports.

I'll do it now.

…_consumer' from Michał Chojnowski View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes #14503 Closes #14502 * github.com:scylladb/scylladb: test: view_build_test: add range tombstones to test_view_update_generator_buffering test: view_build_test: add test_view_udate_generator_buffering_with_random_mutations view_updating_consumer: make buffer limit a variable view: fix range tombstone handling on flushes in view_updating_consumer (cherry picked from commit c25201c)

nyh · 2023-07-11T07:32:14Z

Backported to next-5.3 (b0d3dc5).

The backport to next-5.2 and next-5.1 wasn't clean. The conflict was just header files in test/boost/view_build_test.cc so I thought and fixed it but it turns out it uses header files (test/lib/key_utils.hh) which didn't yet exist in 5.1 or 5.2, so it won't build... (I briefly added a backport commit, and then dequeued it, sorry about the mess). So @michoecho please look into the tests and what header files you need for it (and how to avoid needing them) and prepare separate backport PRs (probably the same PR could work for both 5.1 and 5.2).

michoecho · 2023-07-11T07:33:38Z

Backported to next-5.3 (b0d3dc5).

The backport to next-5.2 and next-5.1 wasn't clean. The conflict was just header files in test/boost/view_build_test.cc so I thought and fixed it but it turns out it uses header files (test/lib/key_utils.hh) which didn't yet exist in 5.1 or 5.2, so it won't build... (I briefly added a backport commit, and then dequeued it, sorry about the mess). So @michoecho please look into the tests and what header files you need for it (and how to avoid needing them) and prepare separate backport PRs (probably the same PR could work for both 5.1 and 5.2).

I see, thanks. I will prepare the PRs.

View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes scylladb#14503

…andom_mutations A random mutation test for view_updating_consumer's buffering logic. Reproduces scylladb#14503.

…ator_buffering This patch adds a full-range tombstone to the compacted mutation. This raises the coverage of the test. In particular, it reproduces issue scylladb#14503, which should have been caught by this test, but wasn't.

View update routines accept `mutation` objects. But what comes out of staging sstable readers is a stream of mutation_fragment_v2 objects. To build view updates after a repair/streaming, we have to convert the fragment stream into `mutation`s. This is done by piping the stream to mutation_rebuilder_v2. To keep memory usage limited, the stream for a single partition might have to be split into multiple partial `mutation` objects. view_update_consumer does that, but in improper way -- when the split/flush happens inside an active range tombstone, the range tombstone isn't closed properly. This is illegal, and triggers an internal error. This patch fixes the problem by closing the active range tombstone (and reopening in the same position in the next `mutation` object). The tombstone is closed just after the last seen clustered position. This is not necessary for correctness -- for example we could delay all processing of the range tombstone until we see its end bound -- but it seems like the most natural semantic. Fixes scylladb#14503

…andom_mutations A random mutation test for view_updating_consumer's buffering logic. Reproduces scylladb#14503.

…ator_buffering This patch adds a full-range tombstone to the compacted mutation. This raises the coverage of the test. In particular, it reproduces issue scylladb#14503, which should have been caught by this test, but wasn't.

denesb · 2023-07-11T08:39:43Z

5.3 backport was dequeued as it breaks the build. @michoecho please prepare a backport PR for 5.3.