[tarantool-patches] Re: [PATCH v3 1/1] sql: hold in stat tables space/index id instead of name

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

Hi! Thanks for the fixes! See comments below.

Running box.cfg{}, I see this:

     2018-12-10 13:13:15.077 [6329] main/101/interactive xlog.c:1920 E> can't open tx: bootstrap: has some data after eof marker at 1903

Looks like an artifact of changing initial snapshot with a
wrong editor, or using a wrong editor mode. In vim, as I
remember, you should use -b option. But I can be mistaken.

Also, this upgrade does not work. I've created a
snapshot with old stat format and 2.1.0 version. Then I
tried to start on this snapshot using your version, but
got

2018-12-10 13:22:07.317 [14183] main/101/interactive F> failed to initialize SQL subsystem
Process 14183 exited with status = 1 (0x00000001)

On 08/12/2018 16:10, imeevma@tarantool.org wrote:
> Hi! Thank you for review! New version and my answers below.
> 
> I resend this letters because didn't add links in the last one.
> In addition, it seems that the tabs were replaced with spaces
> there.
> 
> https://github.com/tarantool/tarantool/issues/3242
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3242-hold-ids-in-stat-tables
> > commit 5e1eb513b33ee8ab31901e8e0bf8d8f5a9442b9f
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Sun Dec 2 17:57:19 2018 +0300
> 
>      sql: hold in stat tables space/index id instead of name
>      
>      To avoid problems with table and index renaming it is good idea
>      to save ids of tables and indexes instead of their names. Ids of
>      tables and indexes are fixed values.
>      
>      Closes #3242
>      Closes #2962
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index d6cc821..22208c6 100644
> Binary files a/src/box/bootstrap.snap and b/src/box/bootstrap.snap differ
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 3d9acc9..2767f9b 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -613,6 +613,31 @@ local function upgrade_to_2_1_0()
>       upgrade_priv_to_2_1_0()
>   end
>   
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.1.1
> +--------------------------------------------------------------------------------
> +
> +local function upgrade_sql_stat_to_2_1_1(space_id, parts)
> +    local _sql_stat = box.space[space_id]
> +
> +    local stats = _sql_stat:select()
> +    for _, v in pairs(stats) do _sql_stat:delete{v.tbl, v.idx} end

In this file on line 52 you have function truncate(space).

> +
> +    local format = _sql_stat:format()
> +    format[1].name = 'space_id'
> +    format[1].type = 'unsigned'
> +    format[2].name = 'index_id'
> +    format[2].type = 'unsigned'
> +    _sql_stat.index.primary:alter{parts={3, 'string'}}
> +    _sql_stat:format(format)
> +    _sql_stat.index.primary:alter{parts=parts}
> +end
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 8625d92..844b6c3 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -444,21 +444,19 @@ schema_init()
>   	sc_space_new(BOX_INDEX_ID, "_index", key_parts, 2,
>   		     &alter_space_on_replace_index, &on_stmt_begin_index);
>   
> -	/* _sql_stat1 - a simpler statistics on space, seen in SQL. */
> -	key_parts[0].fieldno = 0; /* space name */
> +	/*
> +	 *_sql_stat1 - a simpler statistics on space, seen in SQL.
> +	 * The real index is defined in the snapshot.
> +	 */
> +	key_parts[0].fieldno = 2;
>   	key_parts[0].type = FIELD_TYPE_STRING;
> -	key_parts[1].fieldno = 1; /* index name */
> -	key_parts[1].type = FIELD_TYPE_STRING;
> -	sc_space_new(BOX_SQL_STAT1_ID, "_sql_stat1", key_parts, 2, NULL, NULL);
> +	sc_space_new(BOX_SQL_STAT1_ID, "_sql_stat1", key_parts, 1, NULL, NULL);
>   
> -	/* _sql_stat4 - extensive statistics on space, seen in SQL. */
> -	key_parts[0].fieldno = 0; /* space name */
> -	key_parts[0].type = FIELD_TYPE_STRING;
> -	key_parts[1].fieldno = 1; /* index name */
> -	key_parts[1].type = FIELD_TYPE_STRING;
> -	key_parts[2].fieldno = 5; /* sample */
> -	key_parts[2].type = FIELD_TYPE_SCALAR;
> -	sc_space_new(BOX_SQL_STAT4_ID, "_sql_stat4", key_parts, 3, NULL, NULL);
> +	/*
> +	 * _sql_stat4 - extensive statistics on space, seen in SQL.
> +	 * The real index is defined in the snapshot.
> +	 */
> +	sc_space_new(BOX_SQL_STAT4_ID, "_sql_stat4", key_parts, 1, NULL, NULL);

I understand why you are trying to remove changed columns from primary
index, but I do not think it will work, because

1) as I showed at the beginning of the email, it actually already does not
    work;

2) if we remove some parts, it can break uniqueness of existing
    tuples before you truncated them. Maybe it is the source of
    point (1).

I've tried to dig how we'd dealt back in the days with the same
problem about other system spaces when upgrading from < 1.7.5 to
1.7.5, but found, that those days only a format was bad, not
index parts. So our problem is 'unique' and new.

I think, that you should leave parts in schema.cc as is if they
are anyway changed by next snapshot rows. But write a comment about
it in schema.cc.

Also I've tried to investigate a reason why even after leaving parts
as is Tarantool still does not start and found that sql_analysis_load
leads to panic() if stat tables contain garbage. I think, that we
should not treat such error so fatal. This issue is a provement of my
words: https://github.com/tarantool/tarantool/issues/3866

By the way, I do not see a test on correct upgrade. sql/upgrade.test.lua
does not contain any analyzes nor 2.1.0 snapshots. Please, add a test.
Maybe it will require some refactoring of sql/upgrade.test.lua so as to
untie it from testing upgrade from 1.10 only. Look at how xlog and vinyl
upgrade scripts are done.

At first, add a test-run option to sql/upgrade.test.lua with a version.
Second, create a folder 2.1.0 near 1.10 in sql/upgrade/. Put a 2.1.0
snapshot into the new folder. It should contain some old analyze data.
Third, add 2.1.0 option to test-run cfg and change upgrade.test.lua so it
starts not from sql/upgrade/1.10, but from
sql/upgrade/<option_got_from_test_run>.

>   
>   	/* _fk_сonstraint - foreign keys constraints. */
>   	key_parts[0].fieldno = 0; /* constraint name */