Re: [Tarantool-patches] [PATCH v2 1/1] sql: enable autoindex optimization

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

Привет! Спасибо за фиксы!

>> То есть если допустим в оригинальном спейсе были колонки A, B, C, D, и я в запросе
>> использовал SELECT A WHERE C, D, то в эфемерном спейсе будут A, C, D, так? 'B' не
>> будет.
>>
> 
> Да, причем порядок так же может измениться. Например:
> CREATE TABLE t (i INT PRIMARY KEY, a INT, b INT, c INT);
> SELECT t.i FROM t, t as t1 WHERE t.a == 1 AND t.b == 2 AND t.c > 1;
> 
> В этом случае эфемерный спейс(автоиндекс) будет содержать все колонки
> оригинального спейса, но порядок будет другой. Причем поиск будет только по
> колонкам a и b оригинального спейса. Возможно, имеет смысл рассмотреть включение
> также и одного неравенства (как это делается для обычных индексов), но не могу
> сказать, что это точно будет лучше, т.к. в этом случае нужно будет разбираться
> с итераторами.

Понял, спасибо.

>>>     У нас нет конфигурации опций сборки SQL. Мы их все выпиливаем, так как билды
>>>     у нас под один конкретный набор опций. Есть runtime опции, но это ничего общего
>>>     с макросами не имеет. Если этот макрос более не указывается, то надо его
>>>     удалить отовсюду.
>>>
>>>
>>> Понял, исправлю. Я думаю в этом случае я добавлю новую опцию в session_settings, которая будет отключать эту оптимизацию.
>>
>> Не надо пока ничего добавлять. Мы либо включаем ее сейчас, либо не
>> включаем. Если автоиндексы так вредны, что надо их выключать через
>> сессию, то не вижу вообще смысла их пушать, пока не доработано.
>> Ничем не будет отличаться от того, что сейчас в мастере, когда они даже
>> не компилируются.
>>
> 
> Убрал макросы. Я предполагаю, что в нынешнем состоянии автоиндексы не
> оптимальны, однако они позволяют сильно ускорить исполнение некоторых запросов
> TPCH. Некоторые другие запросы TPCH могуть при этом замедлиться, однако в итоге
> мы выигрываем. Точные данные я пока не смог получить.

Было бы круто во время выполнения сначала прикидывать размер, а потом решать
автоиндекс или нет. Прямо в вдбе коде чтоб было и то, и другое, и бранчинг
между ними в начале запроса. Выглядит это космически пока. Но во время
парсинга такое проверять тоже плохо - размеры спейсов меняются, а запросы могут
быть запрепейрены в самом начале, пока они были пустые.

>>>>
>>>> 14. Shouldn't there be a test showing 'AUTOMATIC' keyword in the execution plan?
>>> Actually it does, in the test above.
>>
>> Во всем whereF.test.lua файле слово automatic грепается только в каком-то комменте.
>>
> 
> Я добавил тест в sql/misc.test.lua. Этот тест не покажет улучшение скорости,
> только покажет EQP. При этом, т.к. я использовал пустые спейсы, создание
> автоиндекса в данном случае неоправдано с точки зрения оптимальности. Тест
> работает в нынешнем виде пока мы не обозначили в каких случаях он мы должны
> использовать автоиндекс.

Как раз хорошо - значит тест упадет, когда начнем автоиндексы использовать
только по мере надобности, и сразу заметим.

Смотри 4 минорных коммента ниже. Если они ок, то сразу Никите потом посылай
патч, если это нужно.

> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index e9e936856..01bf7603a 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -703,19 +702,52 @@ termCanDriveIndex(WhereTerm * pTerm,	/* WHERE clause term to check */
>  		return 0;
>  	if (pTerm->u.leftColumn < 0)
>  		return 0;
> -	enum field_type type = pSrc->pTab->def->fields[pTerm->u.leftColumn].type;
> +	enum field_type type = pSrc->space->def->fields[pTerm->u.leftColumn].type;
>  	enum field_type expr_type = expr_cmp_mutual_type(pTerm->pExpr);
>  	if (!field_type1_contains_type2(expr_type, type))
>  		return 0;
>  	return 1;
>  }
> -#endif
>  
> -#ifndef SQL_OMIT_AUTOMATIC_INDEX
> +/**
> + * Generate a code that will create a tuple, which will be inserted in the
> + * autoindex. The created tuple consists of rowid and fields described in the
> + * index key description.
> + *
> + * @param parse Parsing context.
> + * @param key_def The index key description.
> + * @param cursor Cursor of space where we will get values for tuple.
> + * @param reg_out Register to which the created tuple will be placed.
> + * @param reg_eph Register holding pointer to autoindex.
> + *
> + * @return Return a register number which is the first in a block of registers
> + * that holds the elements of the created tuple. The block of registers has
> + * already been deallocated by the time this routine returns.
> + */
> +static int
> +emit_autoindex_tuple_creation(struct Parse *parse, struct key_def *key_def,
> +			      int cursor, int reg_out, int reg_eph)

1. Предлагаю быть консистентными и начинать имя с 'vdbe_emit_'. Еще остальные
vdbe_emit функции используют императив во второй части имени, так что это
имя должно было бы быть вида

	vdbe_emit_create_autoindex_tuple

2. Можно добавить const к параметру key_def?

В целом выглядит гораздо лучше, по дизайну больше вопросов нет.

> @@ -4794,18 +4868,37 @@ sqlWhereEnd(WhereInfo * pWInfo)
>  			for (; k < last; k++, pOp++) {
>  				if (pOp->p1 != pLevel->iTabCur)
>  					continue;
> -				if (pOp->opcode == OP_Column) {
> -					int x = pOp->p2;
> -					assert(def == NULL ||
> -					       def->space_id ==
> -					       pTabItem->space->def->id);
> -					if (x >= 0) {
> -						pOp->p2 = x;
> -						pOp->p1 = pLevel->iIdxCur;
> -					}
> -					assert((pLoop->
> -						wsFlags & WHERE_IDX_ONLY) == 0
> -					       || x >= 0);
> +				if (pOp->opcode != OP_Column)
> +					continue;
> +				assert(def == NULL || def->space_id ==
> +						      pTabItem->space->def->id);
> +				int x = pOp->p2;
> +				assert((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 ||
> +				       x >= 0);
> +				if (x < 0)
> +					continue;

3. Как x может быть < 0? Это ведь p2, который у OP_Column, это номер колонки.
Номера колонок всегда >= 0.

> +				pOp->p1 = pLevel->iIdxCur;
> +				if ((pLoop->wsFlags & WHERE_AUTO_INDEX) == 0) {
> +					pOp->p2 = x;
> +					continue;
> +				}
> +				/*
> +				 * In case we are using autoindex, the space
> +				 * that will be used to get the values will be
> +				 * autoindex. Since the opcode OP_Column uses
> +				 * the position of the fields according to the
> +				 * original space, and the fields may be in
> +				 * other positions in the autoindex, we must
> +				 * correct the P2 of OP_Column. To get the
> +				 * positions of these fields in autoindex, we
> +				 * use the index definition we created.
> +				 */
> +				struct key_def *key_def =
> +					pLevel->pWLoop->index_def->key_def;
> +				uint32_t part_count = key_def->part_count;
> +				for (uint32_t i = 0; i < part_count; ++i) {
> +					if ((int)key_def->parts[i].fieldno == x)
> +						pOp->p2 = i;
>  				}
>  			}
>  		}
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 6d8768865..534819d48 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -219,12 +219,12 @@ sqlWhereExplainOneScan(Parse * pParse,	/* Parse context */
>  
>  			assert(!(flags & WHERE_AUTO_INDEX)
>  			       || (flags & WHERE_IDX_ONLY));
> -			if (idx_def->iid == 0) {
> +			if (flags & WHERE_AUTO_INDEX) {

4. Предлагаю '(flags & WHERE_AUTO_INDEX) != 0'.