Re: [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two

["Maxim Kulis" <bokuno@picodata.io>]

[-- Attachment #1: Type: text/plain, Size: 6343 bytes --]



Hi! 
Yes, here is the new patch.

Change the minimum possible value of the variable vinyl_run_size_ratio to 2. 

Closes #3346

issue:  https://github.com/tarantool/tarantool/issues/3346
---
 src/box/alter.cc | 4 ++--
 src/box/box.cc | 4 ++--
 src/box/vy_range.c | 2 +-
 test/box-tap/cfg.test.lua | 2 +-
 test/vinyl/ddl.result | 5 +++--
 test/vinyl/ddl.test.lua | 2 +-
 6 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bef25b605..123e94994 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -227,10 +227,10 @@ index_opts_decode(struct index_opts *opts, const char *map,
  "run_count_per_level must be greater than 0");
  return -1;
  }
- if (opts->run_size_ratio <= 1) {
+ if (opts->run_size_ratio < 2) {
  diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
  BOX_INDEX_FIELD_OPTS,
- "run_size_ratio must be greater than 1");
+ "run_size_ratio must be greater than or uqual to 2");
  return -1;
  }
  if (opts->bloom_fpr <= 0 || opts->bloom_fpr > 1) {
diff --git a/src/box/box.cc b/src/box/box.cc
index b119c927b..0f26e28cd 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -589,9 +589,9 @@ box_check_vinyl_options(void)
  tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level",
  "must be greater than 0");
  }
- if (run_size_ratio <= 1) {
+ if (run_size_ratio < 2) {
  tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio",
- "must be greater than 1");
+ "must be greater than or uqual to 2");
  }
  if (bloom_fpr <= 0 || bloom_fpr > 1) {
  tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr",
diff --git a/src/box/vy_range.c b/src/box/vy_range.c
index 4ff852112..8d6bfd30b 100644
--- a/src/box/vy_range.c
+++ b/src/box/vy_range.c
@@ -289,7 +289,7 @@ vy_range_update_compaction_priority(struct vy_range *range,
  const struct index_opts *opts)
 {
  assert(opts->run_count_per_level > 0);
- assert(opts->run_size_ratio > 1);
+ assert(opts->run_size_ratio >= 2);
 
  range->compaction_priority = 0;
  vy_disk_stmt_counter_reset(&range->compaction_queue);
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index d529447bb..b68c6f17b 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0)
 invalid('vinyl_write_threads', 1)
 invalid('vinyl_page_size', 0)
 invalid('vinyl_run_count_per_level', 0)
-invalid('vinyl_run_size_ratio', 1)
+invalid('vinyl_run_size_ratio', 1.9)
 invalid('vinyl_bloom_fpr', 0)
 invalid('vinyl_bloom_fpr', 1.1)
 
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 9c453a007..a97e415be 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -23,9 +23,10 @@ space:create_index('pk', {run_count_per_level = 0})
 - error: 'Wrong index options (field 4): run_count_per_level must be greater than
     0'
 ...
-space:create_index('pk', {run_size_ratio = 1})
+space:create_index('pk', {run_size_ratio = 1.9})
 ---
-- error: 'Wrong index options (field 4): run_size_ratio must be greater than 1'
+- error: 'Wrong index options (field 4): run_size_ratio must be greater than or uqual
+ to 2'
 ...
 space:create_index('pk', {bloom_fpr = 0})
 ---
diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua
index 010ec6d79..35e37a84f 100644
--- a/test/vinyl/ddl.test.lua
+++ b/test/vinyl/ddl.test.lua
@@ -6,7 +6,7 @@ space = box.schema.space.create('test', {engine = 'vinyl' })
 space:create_index('pk', {page_size = 0})
 space:create_index('pk', {page_size = 8192, range_size = 4096})
 space:create_index('pk', {run_count_per_level = 0})
-space:create_index('pk', {run_size_ratio = 1})
+space:create_index('pk', {run_size_ratio = 1.9})
 space:create_index('pk', {bloom_fpr = 0})
 space:create_index('pk', {bloom_fpr = 1.1})
 space:drop()
-- 
2.17.1

>Среда, 15 января 2020, 20:36 +03:00 от Nikita Pettik < korablev@tarantool.org >:
>
>On 13 Jan 12:48, Maxim Kulis wrote:
>> Hi! 
>> Thanks for your review. 
>> 
>
>Hi,
>
>Are you going to update your patch according to the review notes?
>Review process is described here:
>https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/#how-to-submit-a-patch-for-review
> 
>> >Четверг,  5 декабря 2019, 14:27 +03:00 от Nikita Pettik < korablev@tarantool.org >:
>> >
>> >On 02 Dec 19:11, Maksim Kulis wrote:
>> >> Change the minimum possible value of the variable vinyl_run_size_ratio to 2.
>> >> 
>> >> Closes #3346
>> >> ---
>> >>  src/box/box.cc            | 4 ++--
>> >>  test/box-tap/cfg.test.lua | 2 +-
>> >>  2 files changed, 3 insertions(+), 3 deletions(-)
>> >
>> >Hello. Thanks for the patch.
>> >
>> >Firsly, please update check of run_size_ratio during index options
>> >decode (see index_opts_decode()).
>> >
>> >I would also update assert() in vy_range_update_compaction_priority():
>> >let's make it check that run_size_ratio always >= 2.
>> >
>> >Finally, I would add brief explanation to the commit message why
>> >run_size_ratio less than 2 is meaningless (for those who are not
>> >familiar with vinyl inner organization).
>> > 
>> >> diff --git a/src/box/box.cc b/src/box/box.cc
>> >> index be2390335..e426fdab8 100644
>> >> --- a/src/box/box.cc
>> >> +++ b/src/box/box.cc
>> >> @@ -589,9 +589,9 @@ box_check_vinyl_options(void)
>> >>  		tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level",
>> >>  			  "must be greater than 0");
>> >>  	}
>> >> -	if (run_size_ratio <= 1) {
>> >> +	if (run_size_ratio < 2) {
>> >>  		tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio",
>> >> -			  "must be greater than 1");
>> >> +			  "must be greater than or uqual to 2");
>> >>  	}
>> >>  	if (bloom_fpr <= 0 || bloom_fpr > 1) {
>> >>  		tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr",
>> >> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
>> >> index d529447bb..b68c6f17b 100755
>> >> --- a/test/box-tap/cfg.test.lua
>> >> +++ b/test/box-tap/cfg.test.lua
>> >> @@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0)
>> >>  invalid('vinyl_write_threads', 1)
>> >>  invalid('vinyl_page_size', 0)
>> >>  invalid('vinyl_run_count_per_level', 0)
>> >> -invalid('vinyl_run_size_ratio', 1)
>> >> +invalid('vinyl_run_size_ratio', 1.9)
>> >>  invalid('vinyl_bloom_fpr', 0)
>> >>  invalid('vinyl_bloom_fpr', 1.1)
>> >> 
>> >> -- 
>> >> 2.17.1
>> >> 
>> 
>> 
>> -- 
>> Maxim Kulis


-- 
Maxim Kulis

[-- Attachment #2: Type: text/html, Size: 9029 bytes --]