From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D88A8469719 for ; Fri, 2 Oct 2020 01:03:30 +0300 (MSK) References: <7aaeb77d6b1bb2eb350e5f2694532dd726328876.1601140429.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <3bb10a2b-dedf-5bdb-8f25-eba0582542cf@tarantool.org> Date: Fri, 2 Oct 2020 00:03:26 +0200 MIME-Version: 1.0 In-Reply-To: <7aaeb77d6b1bb2eb350e5f2694532dd726328876.1601140429.git.imeevma@gmail.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 1/1] sql: enable autoindex optimization List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org, tsafin@tarantool.org, kyukhin@tarantool.org Cc: Mergen Imeev , tarantool-patches@dev.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'.