Skip to content

bps: prevent missing matras head view changes#11980

Merged
sergepetrenko merged 1 commit intotarantool:masterfrom
mkostoevr:m.kostoev/gh-11979-head-view-changes
Oct 31, 2025
Merged

bps: prevent missing matras head view changes#11980
sergepetrenko merged 1 commit intotarantool:masterfrom
mkostoevr:m.kostoev/gh-11979-head-view-changes

Conversation

@mkostoevr
Copy link
Contributor

@mkostoevr mkostoevr commented Oct 24, 2025

We've removed CoW'ing of garbage blocks for creating new inner/leaf blocks in commit 52b3d4b ("salad: reserve block before bps_tree_insert_first_elem") as the blocks are not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to a tricky problem: if we've created a block, CoW'ed another one and then update values of the created one, the new block could've been CoW'ed by the previous matras_touch (as the blocks are located in extents of greater size than the blocks themselves). So following updates in the created block gone into the view while the current matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation, we need to make sure there's no any matras_touch between the block creation and the last assignment of its field. We could fix the only place where the guarantee is not preserved: the leaf insert routine, but let's keep the tree similar to the BPS vector and simply revert the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more memory for touches, as some new garbage blocks can be allocated on the bps_vec_reserve_blocks, but other ones can be existing in the vector previously, and, for these ones, we must reserve touches. Also, one more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the bps_tree_process_insert_leaf by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

@mkostoevr mkostoevr requested review from Astronomax and nshy October 24, 2025 12:48
@mkostoevr mkostoevr self-assigned this Oct 24, 2025
@mkostoevr mkostoevr force-pushed the m.kostoev/gh-11979-head-view-changes branch from 50fc9c3 to 747d248 Compare October 24, 2025 12:51
@coveralls
Copy link

coveralls commented Oct 24, 2025

Coverage Status

coverage: 87.655% (-0.01%) from 87.665%
when pulling 18c422f on mkostoevr:m.kostoev/gh-11979-head-view-changes
into 9941bd5
on tarantool:master
.

@mkostoevr mkostoevr assigned nshy and Astronomax and unassigned mkostoevr Oct 24, 2025
@mkostoevr mkostoevr marked this pull request as ready for review October 24, 2025 13:41
@mkostoevr mkostoevr assigned mkostoevr and unassigned nshy and Astronomax Oct 24, 2025
@mkostoevr mkostoevr marked this pull request as draft October 24, 2025 13:46
@mkostoevr
Copy link
Contributor Author

Discussed offline and decided to keep it similar to the BPS vector for now.

@mkostoevr mkostoevr added backport/3.3 Automatically create a 3.3 backport PR backport/3.4 Automatically create a 3.4 backport PR backport/3.5 Automatically create a 3.5 backport PR labels Oct 24, 2025
@mkostoevr mkostoevr force-pushed the m.kostoev/gh-11979-head-view-changes branch 2 times, most recently from 8c4162b to ee1dfc6 Compare October 24, 2025 15:34
@mkostoevr mkostoevr assigned nshy and Astronomax and unassigned mkostoevr Oct 26, 2025
@mkostoevr mkostoevr marked this pull request as ready for review October 26, 2025 10:58
@nshy nshy removed their assignment Oct 27, 2025
@nshy nshy removed their assignment Oct 27, 2025
@Astronomax Astronomax assigned mkostoevr and unassigned Astronomax Oct 27, 2025
@mkostoevr mkostoevr added the full-ci Enables all tests for a pull request label Oct 27, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up tarantool#11857
Closes tarantool#11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
@mkostoevr mkostoevr force-pushed the m.kostoev/gh-11979-head-view-changes branch from ee1dfc6 to 18c422f Compare October 28, 2025 09:45
@mkostoevr
Copy link
Contributor Author

Fixed a typo.
--- a/src/lib/salad/bps_tree.h
+++ b/src/lib/salad/bps_tree.h
@@ -3193,7 +3193,7 @@ bps_tree_garbage_pop(struct bps_tree_common *tree, bps_tree_block_id_t *id)
                 * an existing leaf to link it with the new one, and then
                 * update the new leaf's `next_id` (which updates data of
                 * the read view by the pointer returned here instead of
-                * new CoWed data). This case is easily fixable bit let's
+                * new CoWed data). This case is easily fixable but let's
                 * fix it here the same way as it was fixed in BPS vector,
                 * as keeping this function non-CoW makes its usage error-
                 * prone.

@mkostoevr mkostoevr assigned sergepetrenko and unassigned mkostoevr Oct 28, 2025
@sergepetrenko sergepetrenko merged commit 24cdd89 into tarantool:master Oct 31, 2025
62 of 67 checks passed
@TarantoolBot
Copy link
Collaborator

Successfully created backport PR for release/3.3:

@TarantoolBot
Copy link
Collaborator

Successfully created backport PR for release/3.4:

@TarantoolBot
Copy link
Collaborator

Successfully created backport PR for release/3.5:

@TarantoolBot
Copy link
Collaborator

Backport summary

@mkostoevr
Copy link
Contributor Author

mkostoevr commented Nov 1, 2025

Cherry-picked to 3.3, 3.4 and 3.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/3.3 Automatically create a 3.3 backport PR backport/3.4 Automatically create a 3.4 backport PR backport/3.5 Automatically create a 3.5 backport PR full-ci Enables all tests for a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BPS tree can get invalidated in rare cases in presense of a read view

6 participants