[Tarantool-patches] [PATCH] vinyl: account statement statistics during .index rebuild

[Tarantool-patches] [PATCH] vinyl: account statement statistics during .index rebuild

[Nikita Pettik]

It may turn out to be necessary to re-create .index file corresponding
to .run file. During rebuild it was forgotten to account statement
statistics, i.e. count of INSERTs, DELETEs etc. Let's fix it and patch
vy_run_rebuild_index().
---
Branch:
https://github.com/tarantool/tarantool/tree/np/vy-account-stmt-stat-on-index-rebuild
N.B. problem is obvious and trivial to fix. However, test for it is likely
to be way more sophisticated, so I haven't included it (but checked manually
that everything works fine).

 src/box/vy_run.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index b9822dc3e..820a6ce3f 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -2452,6 +2452,8 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,
 					goto close_err;
 				}
 			}
+			vy_stmt_stat_acct(&run->info.stmt_stat,
+					  vy_stmt_type(tuple));
 			key = vy_stmt_is_key(tuple) ? tuple_data(tuple) :
 			      tuple_extract_key(tuple, cmp_def,
 						MULTIKEY_NONE, NULL);
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] vinyl: account statement statistics during .index rebuild

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 24.11.2020 22:03, Nikita Pettik wrote:
> It may turn out to be necessary to re-create .index file corresponding
> to .run file. During rebuild it was forgotten to account statement
> statistics, i.e. count of INSERTs, DELETEs etc. Let's fix it and patch
> vy_run_rebuild_index().
> ---
> Branch:
> https://github.com/tarantool/tarantool/tree/np/vy-account-stmt-stat-on-index-rebuild
> N.B. problem is obvious and trivial to fix. However, test for it is likely
> to be way more sophisticated, so I haven't included it (but checked manually
> that everything works fine).

How can I validate it manually?

>  src/box/vy_run.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/box/vy_run.c b/src/box/vy_run.c
> index b9822dc3e..820a6ce3f 100644
> --- a/src/box/vy_run.c
> +++ b/src/box/vy_run.c
> @@ -2452,6 +2452,8 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,
>  					goto close_err;
>  				}
>  			}
> +			vy_stmt_stat_acct(&run->info.stmt_stat,
> +					  vy_stmt_type(tuple));

Why is it needed? The run is not created from the scratch. Only
its index is rebuilt, which is not related to the statistics,
AFAIU. The statements in the run are not changed here. It means
it was filled some time ago, and everything should have already
been accounted via vy_run_writer_write_to_page().

Re: [Tarantool-patches] [PATCH] vinyl: account statement statistics during .index rebuild

[Nikita Pettik]

On 26 Nov 21:45, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 24.11.2020 22:03, Nikita Pettik wrote:
> > It may turn out to be necessary to re-create .index file corresponding
> > to .run file. During rebuild it was forgotten to account statement
> > statistics, i.e. count of INSERTs, DELETEs etc. Let's fix it and patch
> > vy_run_rebuild_index().
> > ---
> > Branch:
> > https://github.com/tarantool/tarantool/tree/np/vy-account-stmt-stat-on-index-rebuild
> > N.B. problem is obvious and trivial to fix. However, test for it is likely
> > to be way more sophisticated, so I haven't included it (but checked manually
> > that everything works fine).
> 
> How can I validate it manually?

Drop .index, run tarantool with force-recovery, cat the content of .index
file with tarantoolctl/xlog module.
 
> >  src/box/vy_run.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/box/vy_run.c b/src/box/vy_run.c
> > index b9822dc3e..820a6ce3f 100644
> > --- a/src/box/vy_run.c
> > +++ b/src/box/vy_run.c
> > @@ -2452,6 +2452,8 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,
> >  					goto close_err;
> >  				}
> >  			}
> > +			vy_stmt_stat_acct(&run->info.stmt_stat,
> > +					  vy_stmt_type(tuple));
> 
> Why is it needed? The run is not created from the scratch. Only
> its index is rebuilt, which is not related to the statistics,
> AFAIU. The statements in the run are not changed here. It means
> it was filled some time ago, and everything should have already
> been accounted via vy_run_writer_write_to_page().

Possibly it doesn't affect anything, but just noted that in case
of re-creating .index files during force-recovery, they contained
zeroed statistics (in contrast to ordinary .index files). It may
turn out to be helpful e.g. during digging into vinyl problems.

Re: [Tarantool-patches] [PATCH] vinyl: account statement statistics during .index rebuild

[Vladislav Shpilevoy]

>>>  src/box/vy_run.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/box/vy_run.c b/src/box/vy_run.c
>>> index b9822dc3e..820a6ce3f 100644
>>> --- a/src/box/vy_run.c
>>> +++ b/src/box/vy_run.c
>>> @@ -2452,6 +2452,8 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,
>>>  					goto close_err;
>>>  				}
>>>  			}
>>> +			vy_stmt_stat_acct(&run->info.stmt_stat,
>>> +					  vy_stmt_type(tuple));
>>
>> Why is it needed? The run is not created from the scratch. Only
>> its index is rebuilt, which is not related to the statistics,
>> AFAIU. The statements in the run are not changed here. It means
>> it was filled some time ago, and everything should have already
>> been accounted via vy_run_writer_write_to_page().
> 
> Possibly it doesn't affect anything, but just noted that in case
> of re-creating .index files during force-recovery, they contained
> zeroed statistics (in contrast to ordinary .index files). It may
> turn out to be helpful e.g. during digging into vinyl problems.

No, my question was not about it. I don't understand, why info.stmt_stat
is not already filled? It seems it is based entirely on run file
content, not on its index. It means, you should have it filled when
run is created/recovered.

When is the stat filled when run is recovered normally?

Why does not vy_run_recover() fill the statistics instead?