[tarantool-patches] Re: [PATCH v2 1/1] box: option to start tarantool with no format checks.

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

LGTM.

On 17/08/2018 11:11, Imeev Mergen wrote:
> 
> 
> On 08/17/2018 12:08 AM, Vladislav Shpilevoy wrote:
>> Thanks for the patch! I have pushed my review fix
>> on a separate commit. Please, look at it, squash.
>> Then the patch will be lgtm.
> Thank you for review! I squashed and rebased. Also, I renamed
> created in test space to 't0' because some conflicts emerged
> after rebase.
>>
>> On 14/08/2018 14:50, imeevma@tarantool.org wrote:
>>> It is a common case that an instance is running on a version
>>> 1.6.*, then is upgraded to 1.9 with box.schema.upgrade(). But
>>> some of users has malformed space formats, and some of them got
>>> and ignored errors on box.schema.upgrade(). Such half-upgraded
>>> data can not be used to start a new version due to format
>>> violations, and can not be rolled back because some of new system
>>> spaces managed to be created. And we can not repair raw xlogs and
>>> snapshots because we have no such tools and they are zipped.
>>>
>>> So it would be useful to have a special box.cfg option
>>> ignore_space_formats. It would allow to start an instance on
>>> the spaces with malformed formats, fix them, create a snapshot
>>> and then start with the formats turned on.
>>>
>>> Closes #3605
>>>
>>> @TarantoolBot document
>>> Title: box.cfg option 'ignore_space_formats'
>>> The option allows to turn off space formats validation before
>>> the instance is started. Useful to fix malformed formats after
>>> an upgrade from version < 1.7.5 to >= 1.7.5.
>>> ---
>>> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3605-ignore-space-formats-option
>>> Issue: https://github.com/tarantool/tarantool/issues/3605
>>>
> commit 2124ae8057df48ec19161e9fa38bc70e8c09aa80
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Mon Aug 13 18:55:46 2018 +0300
> 
>      box: option to start tarantool with no format checks.
> 
>      It is a common case that an instance is running on a version
>      1.6.*, then is upgraded to 1.9 with box.schema.upgrade(). But
>      some of users has malformed space formats, and some of them got
>      and ignored errors on box.schema.upgrade(). Such half-upgraded
>      data can not be used to start a new version due to format
>      violations, and can not be rolled back because some of new system
>      spaces managed to be created. And we can not repair raw xlogs and
>      snapshots because we have no such tools and they are zipped.
> 
>      So it would be useful to have a special box.cfg option
>      ignore_space_formats. It would allow to start an instance on
>      the spaces with malformed formats, fix them, create a snapshot
>      and then start with the formats turned on.
> 
>      Closes #3605
> 
>      @TarantoolBot document
>      Title: box.cfg option 'ignore_space_formats'
>      The option allows to turn off space formats validation before
>      the instance is started. Useful to fix malformed formats after
>      an upgrade from version < 1.7.5 to >= 1.7.5.
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index ec3b91b..b379dad 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -54,6 +54,18 @@
>   #include "sequence.h"
> 
>   /**
> + * A flag to ignore space formats and do not validate tuples by
> + * them.
> + */
> +static bool ignore_space_formats = false;
> +
> +void
> +box_set_ignore_space_formats(bool value)
> +{
> +    ignore_space_formats = value;
> +}
> +
> +/**
>    * chap-sha1 of empty string, i.e.
>    * base64_encode(sha1(sha1(""), 0)
>    */
> @@ -533,7 +545,7 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode,
>       const char *space_opts;
>       struct field_def *fields;
>       uint32_t field_count;
> -    if (dd_version_id >= version_id(1, 7, 6)) {
> +    if (dd_version_id >= version_id(1, 7, 6) && !ignore_space_formats) {
>           /* Check space opts. */
>           space_opts =
>               tuple_field_with_type_xc(tuple, BOX_SPACE_FIELD_OPTS,
> diff --git a/src/box/box.h b/src/box/box.h
> index 9dfb3fd..d4043db 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -176,6 +176,7 @@ void box_set_vinyl_timeout(void);
>   void box_set_replication_timeout(void);
>   void box_set_replication_connect_timeout(void);
>   void box_set_replication_connect_quorum(void);
> +void box_set_ignore_space_formats(bool value);
> 
>   extern "C" {
>   #endif /* defined(__cplusplus) */
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 2a7142d..6682d49 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -66,6 +66,7 @@ local default_cfg = {
>       username            = nil,
>       coredump            = false,
>       read_only           = false,
> +    ignore_space_formats= false,
>       hot_standby         = false,
>       checkpoint_interval = 3600,
>       checkpoint_count    = 2,
> @@ -124,6 +125,7 @@ local template_cfg = {
>       checkpoint_interval = 'number',
>       checkpoint_count    = 'number',
>       read_only           = 'boolean',
> +    ignore_space_formats= 'boolean',
>       hot_standby         = 'boolean',
>       worker_pool_threads = 'number',
>       replication_timeout = 'number',
> diff --git a/src/main.cc b/src/main.cc
> index 1682bae..30cc78d 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -521,6 +521,7 @@ load_cfg()
> 
>       title_set_custom(cfg_gets("custom_proc_title"));
>       title_update();
> +    box_set_ignore_space_formats(cfg_geti("ignore_space_formats") != 0);
>       box_cfg();
>   }
> 
> diff --git a/test/app-tap/init_script.result b/test/app-tap/init_script.result
> index eea9f5b..f8b8994 100644
> --- a/test/app-tap/init_script.result
> +++ b/test/app-tap/init_script.result
> @@ -9,41 +9,42 @@ box.cfg
>   4    coredump:false
>   5    force_recovery:false
>   6    hot_standby:false
> -7    listen:port
> -8    log:tarantool.log
> -9    log_format:plain
> -10    log_level:5
> -11    log_nonblock:true
> -12    memtx_dir:.
> -13    memtx_max_tuple_size:1048576
> -14    memtx_memory:107374182
> -15    memtx_min_tuple_size:16
> -16    pid_file:box.pid
> -17    read_only:false
> -18    readahead:16320
> -19    replication_connect_timeout:30
> -20    replication_sync_lag:10
> -21    replication_timeout:1
> -22    rows_per_wal:500000
> -23    slab_alloc_factor:1.05
> -24    too_long_threshold:0.5
> -25    vinyl_bloom_fpr:0.05
> -26    vinyl_cache:134217728
> -27    vinyl_dir:.
> -28    vinyl_max_tuple_size:1048576
> -29    vinyl_memory:134217728
> -30    vinyl_page_size:8192
> -31    vinyl_range_size:1073741824
> -32    vinyl_read_threads:1
> -33    vinyl_run_count_per_level:2
> -34    vinyl_run_size_ratio:3.5
> -35    vinyl_timeout:60
> -36    vinyl_write_threads:2
> -37    wal_dir:.
> -38    wal_dir_rescan_delay:2
> -39    wal_max_size:268435456
> -40    wal_mode:write
> -41    worker_pool_threads:4
> +7    ignore_space_formats:false
> +8    listen:port
> +9    log:tarantool.log
> +10    log_format:plain
> +11    log_level:5
> +12    log_nonblock:true
> +13    memtx_dir:.
> +14    memtx_max_tuple_size:1048576
> +15    memtx_memory:107374182
> +16    memtx_min_tuple_size:16
> +17    pid_file:box.pid
> +18    read_only:false
> +19    readahead:16320
> +20    replication_connect_timeout:30
> +21    replication_sync_lag:10
> +22    replication_timeout:1
> +23    rows_per_wal:500000
> +24    slab_alloc_factor:1.05
> +25    too_long_threshold:0.5
> +26    vinyl_bloom_fpr:0.05
> +27    vinyl_cache:134217728
> +28    vinyl_dir:.
> +29    vinyl_max_tuple_size:1048576
> +30    vinyl_memory:134217728
> +31    vinyl_page_size:8192
> +32    vinyl_range_size:1073741824
> +33    vinyl_read_threads:1
> +34    vinyl_run_count_per_level:2
> +35    vinyl_run_size_ratio:3.5
> +36    vinyl_timeout:60
> +37    vinyl_write_threads:2
> +38    wal_dir:.
> +39    wal_dir_rescan_delay:2
> +40    wal_max_size:268435456
> +41    wal_mode:write
> +42    worker_pool_threads:4
>   --
>   -- Test insert from detached fiber
>   --
> diff --git a/test/box-tap/ignore_formats.test.lua b/test/box-tap/ignore_formats.test.lua
> new file mode 100755
> index 0000000..14211d3
> --- /dev/null
> +++ b/test/box-tap/ignore_formats.test.lua
> @@ -0,0 +1,24 @@
> +#!/usr/bin/env tarantool
> +--
> +-- gh-3605: allow to ignore space formats via box.cfg option.
> +--
> +local tap = require('tap')
> +local test = tap.test("ignore_formats")
> +test:plan(3)
> +
> +box.cfg{ignore_space_formats=true}
> +
> +local format = {}
> +format[1] = {'field1', 'unsigned'}
> +format[2] = {'field2', 'unsigned'}
> +
> +local s = box.schema.create_space('t0', {format = format})
> +local pk = s:create_index('pk')
> +test:ok(pcall(s.replace, s, {1, 'string'}), 'can violate format')
> +-- Is not a dynamic option.
> +local ok, err = pcall(box.cfg, {ignore_space_formats = false})
> +test:ok(not ok, "ignore_space_formats is not a dynamic option")
> +test:ok(err:match("Can't set option"), "error message")
> +
> +s:drop()
> +os.exit(test:check() == true and 0 or 1)
> diff --git a/test/box/admin.result b/test/box/admin.result
> index c3e318a..9200ed8 100644
> --- a/test/box/admin.result
> +++ b/test/box/admin.result
> @@ -30,6 +30,8 @@ cfg_filter(box.cfg)
>       - false
>     - - hot_standby
>       - false
> +  - - ignore_space_formats
> +    - false
>     - - listen
>       - <hidden>
>     - - log
> diff --git a/test/box/cfg.result b/test/box/cfg.result
> index cda7aa0..fad4b87 100644
> --- a/test/box/cfg.result
> +++ b/test/box/cfg.result
> @@ -26,6 +26,8 @@ cfg_filter(box.cfg)
>       - false
>     - - hot_standby
>       - false
> +  - - ignore_space_formats
> +    - false
>     - - listen
>       - <hidden>
>     - - log
> @@ -115,6 +117,8 @@ cfg_filter(box.cfg)
>       - false
>     - - hot_standby
>       - false
> +  - - ignore_space_formats
> +    - false
>     - - listen
>       - <hidden>
>     - - log
>