[tarantool-patches] Re: [PATCH 2/3] vinyl: don't produce deferred DELETE on commit if key isn't updated

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/25 06:41]:

this one is basically lgtm

> Consider the following example:
> 
>   s = box.schema.space.create('test', {engine = 'vinyl'})
>   s:create_index('primary')
>   s:create_index('secondary', {parts = {2, 'unsigned'}})
> 
>   s:insert{1, 1, 1}
>   s:replace{1, 1, 2}
> 
> When REPLACE{1,1} is committed to the secondary index, the overwritten
> tuple, i.e. INSERT{1,1}, is found in the primary index memory, and so
> deferred DELETE{1,1} is generated right away and committed along with
> REPLACE{1,1}. However, there's no need to commit anything to the
> secondary index in this case, because its key isn't updated. Apart from
> eating memory and loading disk, this also breaks index stats, as vy_tx
> implementation doesn't expect two statements committed for the same key
> in a single transaction.
> 
> Fix this by checking if there's a statement in the log for the deleted
> key and if there's skipping them both as we do in the regular case, see
> the comment in vy_tx_set.
> 
> Closes #3693
> ---
>  src/box/vy_tx.c                     | 18 +++++++++
>  test/vinyl/deferred_delete.result   | 78 +++++++++++++++++++++++++++++++++++++
>  test/vinyl/deferred_delete.test.lua | 26 +++++++++++++
>  test/vinyl/quota.result             |  2 +-
>  test/vinyl/stat.result              |  6 +--
>  test/vinyl/stat.test.lua            |  2 +-
>  test/vinyl/write_iterator.result    |  2 +-
>  test/vinyl/write_iterator.test.lua  |  2 +-
>  8 files changed, 129 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
> index aedd0610..119d975b 100644
> --- a/src/box/vy_tx.c
> +++ b/src/box/vy_tx.c
> @@ -629,6 +629,23 @@ vy_tx_handle_deferred_delete(struct vy_tx *tx, struct txv *v)
>  		struct vy_lsm *lsm = vy_lsm(space->index[i]);
>  		struct vy_entry entry;
>  		vy_stmt_foreach_entry(entry, delete_stmt, lsm->cmp_def) {
> +			struct txv *other = write_set_search_key(&tx->write_set,
> +								 lsm, entry);
> +			if (other != NULL && !other->is_overwritten) {
> +				/*
> +				 * The write set contains a statement
> +				 * for the key to be deleted. This can
> +				 * only occur if it's a REPLACE that
> +				 * happens not to update the secondary
> +				 * index key parts. It's safe to skip it,
> +				 * see vy_tx_set_entry().
> +				 */
> +				assert(vy_stmt_type(stmt) == IPROTO_REPLACE);
> +				assert(vy_stmt_type(other->entry.stmt) ==
> +								IPROTO_REPLACE);
> +				other->is_nop = true;
> +				continue;
> +			}
>  			struct txv *delete_txv = txv_new(tx, lsm, entry);
>  			if (delete_txv == NULL) {
>  				rc = -1;
> @@ -717,6 +734,7 @@ vy_tx_prepare(struct vy_tx *tx)
>  			 * to the primary index LSM tree. So we must
>  			 * skip it for secondary indexes as well.
>  			 */
> +			v->is_overwritten = true;
>  			continue;
>  		}
>  
> diff --git a/test/vinyl/deferred_delete.result b/test/vinyl/deferred_delete.result
> index 422f2fe0..3f187a5b 100644
> --- a/test/vinyl/deferred_delete.result
> +++ b/test/vinyl/deferred_delete.result
> @@ -726,6 +726,84 @@ sk:select() -- ditto
>  s:drop()
>  ---
>  ...
> +--
> +-- gh-3693 If a REPLACE doesn't update secondary index key parts,
> +-- we must not generate a deferred DELETE for it on commit.
> +-- Moreover, we must skip it as it has no effect.
> +--
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> +---
> +...
> +pk = s:create_index('pk')
> +---
> +...
> +sk = s:create_index('sk', {parts = {2, 'unsigned'}})
> +---
> +...
> +s:insert{1, 1, 1}
> +---
> +- [1, 1, 1]
> +...
> +s:insert{2, 2, 2}
> +---
> +- [2, 2, 2]
> +...
> +s:replace{2, 2, 3}
> +---
> +- [2, 2, 3]
> +...
> +pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
> +---
> +- 3
> +...
> +sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
> +---
> +- 2
> +...
> +box.begin()
> +---
> +...
> +s:replace{1, 1, 2}
> +---
> +- [1, 1, 2]
> +...
> +s:delete{1}
> +---
> +...
> +box.commit()
> +---
> +...
> +pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
> +---
> +- 4
> +...
> +sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
> +---
> +- 3
> +...
> +box.snapshot()
> +---
> +- ok
> +...
> +pk:stat().rows -- 1: INSERT{2,2,3}
> +---
> +- 1
> +...
> +sk:stat().rows -- 1: INSERT{2,2}
> +---
> +- 1
> +...
> +pk:select()
> +---
> +- - [2, 2, 3]
> +...
> +sk:select()
> +---
> +- - [2, 2, 3]
> +...
> +s:drop()
> +---
> +...
>  box.cfg{vinyl_cache = vinyl_cache}
>  ---
>  ...
> diff --git a/test/vinyl/deferred_delete.test.lua b/test/vinyl/deferred_delete.test.lua
> index 6dfcbde5..689c8f93 100644
> --- a/test/vinyl/deferred_delete.test.lua
> +++ b/test/vinyl/deferred_delete.test.lua
> @@ -265,6 +265,32 @@ box.snapshot()
>  sk:select() -- ditto
>  s:drop()
>  
> +--
> +-- gh-3693 If a REPLACE doesn't update secondary index key parts,
> +-- we must not generate a deferred DELETE for it on commit.
> +-- Moreover, we must skip it as it has no effect.
> +--
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> +pk = s:create_index('pk')
> +sk = s:create_index('sk', {parts = {2, 'unsigned'}})
> +s:insert{1, 1, 1}
> +s:insert{2, 2, 2}
> +s:replace{2, 2, 3}
> +pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
> +sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
> +box.begin()
> +s:replace{1, 1, 2}
> +s:delete{1}
> +box.commit()
> +pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
> +sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
> +box.snapshot()
> +pk:stat().rows -- 1: INSERT{2,2,3}
> +sk:stat().rows -- 1: INSERT{2,2}
> +pk:select()
> +sk:select()
> +s:drop()
> +
>  box.cfg{vinyl_cache = vinyl_cache}
>  
>  --
> diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
> index 1a0842f9..d1b28ee5 100644
> --- a/test/vinyl/quota.result
> +++ b/test/vinyl/quota.result
> @@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 * 5)}
>  ...
>  box.stat.vinyl().memory.level0
>  ---
> -- 5341267
> +- 5292076
>  ...
>  space:drop()
>  ---
> diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
> index aa47a7be..e2991d53 100644
> --- a/test/vinyl/stat.result
> +++ b/test/vinyl/stat.result
> @@ -1158,7 +1158,7 @@ s = box.schema.space.create('test', {engine = 'vinyl'})
>  i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
>  ---
>  ...
> -i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
> +i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
>  ---
>  ...
>  for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
> @@ -1191,7 +1191,7 @@ box.snapshot()
>  stat_diff(gstat(), st, 'scheduler')
>  ---
>  - dump_input: 10420
> -  dump_output: 10371
> +  dump_output: 10411
>    tasks_completed: 2
>    dump_count: 1
>  ...
> @@ -1206,7 +1206,7 @@ while i1:stat().disk.compaction.count == 0 do fiber.sleep(0.01) end
>  ...
>  stat_diff(gstat(), st, 'scheduler')
>  ---
> -- compaction_input: 112188
> +- compaction_input: 112228
>    tasks_completed: 1
>    compaction_output: 101984
>  ...
> diff --git a/test/vinyl/stat.test.lua b/test/vinyl/stat.test.lua
> index 4a360f33..4c751aad 100644
> --- a/test/vinyl/stat.test.lua
> +++ b/test/vinyl/stat.test.lua
> @@ -333,7 +333,7 @@ s:drop()
>  -- sched stats
>  s = box.schema.space.create('test', {engine = 'vinyl'})
>  i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
> -i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
> +i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
>  
>  for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
>  st = gstat()
> diff --git a/test/vinyl/write_iterator.result b/test/vinyl/write_iterator.result
> index 5c5200b8..f0244113 100644
> --- a/test/vinyl/write_iterator.result
> +++ b/test/vinyl/write_iterator.result
> @@ -931,7 +931,7 @@ box.snapshot()
>  - ok
>  ...
>  -- Some padding to trigger minor compaction.
> -for i = 1001, 1000 + PAD2 do s:replace{i, i} end
> +for i = 1001, 1000 + PAD2 do s:delete{i} end
>  ---
>  ...
>  box.snapshot()
> diff --git a/test/vinyl/write_iterator.test.lua b/test/vinyl/write_iterator.test.lua
> index fa44961a..0de21f21 100644
> --- a/test/vinyl/write_iterator.test.lua
> +++ b/test/vinyl/write_iterator.test.lua
> @@ -389,7 +389,7 @@ _ = s:insert{7, 7}
>  _ = s:insert{8, 8}
>  box.snapshot()
>  -- Some padding to trigger minor compaction.
> -for i = 1001, 1000 + PAD2 do s:replace{i, i} end
> +for i = 1001, 1000 + PAD2 do s:delete{i} end
>  box.snapshot()
>  -- Generate DELETE+INSERT statements and write them to disk.
>  s:delete{1} s:insert{1, 100}
> -- 
> 2.11.0
> 

-- 
Konstantin Osipov, Moscow, Russia