<HTML><BODY><div>Прошу прощения, я случайно отправил не полностью написанное письмо.<br><br> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 22 сентября 2020, 8:03 +03:00 от Мерген Имеев <imeevma@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_16007509861085704353_BODY"><div class="cl_757201"><div>Привет! Спасибо за ревью! Я прошу прощения, на данный момент я могу только ответить на вопросы ревьюё Все изменения и примеры я смогу отправить завтра.<br> <blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;">Вторник, 22 сентября 2020, 0:37 +03:00 от Vladislav Shpilevoy <<a href="/compose?To=v.shpilevoy@tarantool.org">v.shpilevoy@tarantool.org</a>>:<br> <div id=""><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><style type="text/css"></style><div><div id="style_16007242701448516475_BODY_mr_css_attr">Привет! Спасибо за патч!<br><br>On 20.09.2020 23:17, Mergen Imeev wrote:<br>> Hi! Thank you for the review. My answers, diff and new patch below.<br>><br>> I tried to answer a few qustions, but at the end decided to not include them<br>> in this patch. Here are these questions:<br><br>Давай по-русски. Я че-то не понимаю ничего. Я этих вопросов не задавал. Это<br>твои собственные вопросы для самопроверки, на которые ты решил ответить<br>публично?</div></div></div></div></blockquote></div><div><br>Мне казалось, что эти вопросы имеют значение для этого патча, поэтому я включил их сюда.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> 1) Can we use secondary index instead of an ephemeral space?<br>> This look a bit difficult, and it cannot be used with view since they do not<br>> have an actual index.<br><br>Это не только сложно, но и неправильно. Создавать вторичный индекс на каждый<br>запрос выглядит очень жестко. Кто будет гарантировать их удаление (так как<br>это изменение схемы, а значит попадает в WAL? Как сделать их невидимыми для<br>юзеров, и "видеть" их только в одном запросе? Как быть с винилом? Слишком<br>много вопросов.</div></div></div></div></blockquote></div><div><br>Понял, спасибо. У меня были вопросы другого плана — как работать с этим вторичным индексом что-бы все не сломалось в SQL, но твои вопросы более глобальны. Мне казалось, что создание индекса вместо пересоздания таплов будет выгоднее, но возникает слишком много проблем. Я думаю пока стоит отказаться от этого варианта.</div><div> </div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>Хотя в далекой перспективе звучит круто - "эфемерные индексы". Прямо таки<br>название для дисера. Могло бы помочь в мемтиксе, чтоб "эфемерные индексы"<br>хранили ссылки на таплы, а не копии таплов.<br><br>> 2) Can we get rid of 'viaCoroutine' branches in constructAutomaticIndex()?<br>> As far as I understand, this flag is always false here.<br><br>Не представляю что это. И с патчем походу не связано.</div></div></div></div></blockquote></div><div><br>Не совсем не связано. Дело в том, что если мы удалим бранч с viaCoroutine, т.к. он у нас на данный момент всегда false, у нас упростится код. Например функция generate_index_key() станет void.</div><div> </div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> 3) Can I get rid of OP_Realify?<br>> I think I can, but decided to do this no in this patch.<br><br>Ты пробовал его просто дропнуть? Ломается что-то?</div></div></div></div></blockquote></div><div><br>Пробовал, ничего не ломается. Если посмотреть на все места использования OP_Realify (их два) и посмотреть на старый код, еще до удалаления типа Real из SQL, то там видно, что этот опкод использовался для преобразования INT в REAL. Когда REAL быз заменен на NUMBER это преобразование осталось, хотя, как мне кажется, в нем небыло нужды. После добавления DOUBLE в паре мест этот опкод все еще существует, однако используется неправильно. Я думаю этот опкод больше ненужен. Однако, мне кажется его стоит убрать отдельным патчем вне этой задачи.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> On Wed, Sep 09, 2020 at 11:58:09PM +0200, Vladislav Shpilevoy wrote:<br>>> Hi! Thanks for the patch!<br>>><br>>> See 14 comments below.<br>>><br>>> On 04.09.2020 13:53, <a rel="noopener noreferrer">imeevma@tarantool.org</a> wrote:<br>>>> This patch enables the "auto-index" optimization in SQL. The auto-index<br>>>> is actually an ephemeral space that contain some columns of the original<br>>>> space.<br>>><br>>> 1. In the patch the code calls the index 'covering' meaning that it<br>>> contains all the columns. What is true?<br>> Both, I think, since this index is the PK of mentioned ephemeral space.<br><br>Что значит both? Индекс либо covering (содержит все колонки), либо нет (содержит<br>не все).</div></div></div></div></blockquote></div><div><br>Суть в том, что idx_def содержит не все колонки оригинального спейса, однако индекс создаваемый для эфемерного спейса содержит все колонки. При этом, создание idx_def не приводит к созданию индекса.</div><div> </div><div>Я попробую описать весь механизм:</div><ol><li>Планировщик определяет, что будет использоваться автоматический индекс.</li><li>Создается idx_def, который содержит все использующиеся в запросе колонки оригинального спейса. Не только те, которые используются во where. Это делается для того, что бы больше не обращаться к оригинальному спейсу, а работать только с эфемерным спейсом. Этот idx_def не используется для создания индекса.</li><li>Создается эфемерный спейс на основе созданного ранее idx_def. Помимо колонок оригинального спейса добавляется rowid, т.к. возможны случаи, когда значения во всех колонках совпадает в нескольких записях. При этом, колонки в эфемерном спейсе расположены в том же порядке, в каком они описаны в индексе. Т.е. они, скорее всего, расположены не в том же порядке, в каком они расположены в оригинальном спейсе.</li><li>Для созданного эфемерного спейса создается индекс, которые является covering. Именно поэтому в некоторых местах написано, что создается covering index.</li><li>Т.к. планировщик посчитал, что будет использоваться автоиндекс, то в качестве спейса из которого будут выбраны таплы мы будем использовать созданный нами эфемерный спейс. Однако, во время построения vdbe-кода в качестве fieldno было использовано расположение колонок в оригинальном спейсе. Поэтому, в случае использования автоиндекса мы заменяем fieldno оригинального спейса в OP_Column на fieldno в эфемерном спейсе используя созданный ранее idx_def.</li></ol><div> </div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> We create index definition for the original space, however this index definition<br>> is not used for any actual index. This index definition is used as a connection<br>> between original space and created ephemeral space.<br><br>Не понял ничего. Перефразируй на русском плиз, может так понятнее станет.</div></div></div></div></blockquote></div><div><br>Здесь я имел ввиду то, что описал в п.2 и п.5 выше.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>>>> In some cases, this can speed up execution, since creating a new<br>>>> index and searching using it can be more cost efficient than scanning.<br>>><br>>> 2. Could you provide an example?<br>> At the moment examples that can show that in some cases SQL works faster with<br>> enabled optimization are quite big. One of such examples is Q13 for TPC-H.<br>> Should I provide data generation and query?<br><br>Нет, скрипт слишком большой наверняка и не скажет ничего. Я пытаюсь понять,<br>для каких запросов это применимо. Как эти запросы описать? Как по запросу понять,<br>будет там автоиндекс или нет? Конкретный пример запроса может и поможет, но я<br>хз, я просто не знаю как он выглядит.</div></div></div></div></blockquote></div><div><br>Понял. Я добавлю пример и описание того, котгда автоиндекс применяется. На данный момент могу сказать, что одним из случаев когда он применяется — запросы с использованием join. Этот вопрос я опишу более попдробно чуть позже.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>Вот ты добавил "Auto-index" optimization is now enabled в changelog. Я юзер, и не<br>представляю что это такое. Ты отправишь меня читать код TPC-H бенча, чтобы понять?</div></div></div></div></blockquote></div><div><br>Понял, исправлю.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>>>> Co-authored-by: Mergen Imeev <<a rel="noopener noreferrer">imeevma@gmail.com</a>><br>>>> Co-authored-by: Timur Safin <<a rel="noopener noreferrer">tsafin@tarantool.org</a>><br>>>> ---<br>>>> <a href="https://github.com/tarantool/tarantool/issues/4933" rel="noopener noreferrer" target="_blank">https://github.com/tarantool/tarantool/issues/4933</a><br>>>> <a href="https://github.com/tarantool/tarantool/tree/imeevma/gh-4933-autoindex" rel="noopener noreferrer" target="_blank">https://github.com/tarantool/tarantool/tree/imeevma/gh-4933-autoindex</a><br>>>><br>>>> @ChangeLog<br>>>> - "Auto-index" optimization is now enabled (gh-4933).<br>>>><br>>>> src/box/CMakeLists.txt | 2 +-<br>>>> src/box/sql.c | 2 +-<br>>>> src/box/sql/delete.c | 16 ++--<br>>>> src/box/sql/sqlInt.h | 8 +-<br>>>> src/box/sql/where.c | 170 +++++++++++++++++++++++------------<br>>>> src/box/sql/wherecode.c | 13 +--<br>>>> test/sql-tap/whereF.test.lua | 16 +++-<br>>>> 7 files changed, 151 insertions(+), 76 deletions(-)<br>>>><br>>>> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt<br>>>> index b8b2689d2..7e3ad0e22 100644<br>>>> --- a/src/box/CMakeLists.txt<br>>>> +++ b/src/box/CMakeLists.txt<br>>>> @@ -217,7 +217,7 @@ add_library(box STATIC<br>>>> if(CMAKE_BUILD_TYPE STREQUAL "Debug")<br>>>> add_definitions(-DSQL_DEBUG=1)<br>>>> endif()<br>>>> -add_definitions(-DSQL_OMIT_AUTOMATIC_INDEX=1 -DSQL_TEST=1)<br>>>> +add_definitions(-DSQL_TEST=1)<br>>><br>>> 3. I still see SQL_OMIT_AUTOMATIC_INDEX in src/box/sql/where.c.<br>> I think the original idea was to make an option to disable this optimization.<br>> Since such thing is already exists, I decided to not remove it.<br><br>У нас нет конфигурации опций сборки SQL. Мы их все выпиливаем, так как билды<br>у нас под один конкретный набор опций. Есть runtime опции, но это ничего общего<br>с макросами не имеет. Если этот макрос более не указывается, то надо его<br>удалить отовсюду.</div></div></div></div></blockquote></div><div><br>Понял, исправлю. Я думаю в этом случае я добавлю новую опцию в session_settings, которая будет отключать эту оптимизацию.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>>>> set(EXT_SRC_DIR ${CMAKE_SOURCE_DIR}/extra)<br>>>> set(EXT_BIN_DIR ${CMAKE_BINARY_DIR}/extra)<br>>>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c<br>>>> index 68abd1f58..57478c129 100644<br>>>> --- a/src/box/sql/delete.c<br>>>> +++ b/src/box/sql/delete.c<br>>>> @@ -546,24 +546,25 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,<br>>>> }<br>>>><br>>>> int<br>>>> -sql_generate_index_key(struct Parse *parse, struct index *index, int cursor,<br>>>> - int reg_out, struct index *prev, int reg_prev)<br>>>> +sql_generate_index_key(struct Parse *parse, struct index_def *idx_def,<br>>>> + int cursor, int reg_out, struct index *prev,<br>>>> + int reg_prev, int reg_eph)<br>>><br>>> 4. The function has nothing to do with ephemeral spaces. It just does not<br>>> care whether its space is ephemeral. Its task is to just assemble a key,<br>>> not caring about space type. Why did you make it always work with an<br>>> ephemeral space? Won't this will affect normal spaces - they don't need<br>>> OP_NextIdEphemeral or whatever else is related to ephemerals.<br>>><br>>> In the end of the review I realized the function is never used for anything<br>>> except automatic index. Moreover, prev and reg_prev are NULL and 0 always.<br>>> I suggest to move this function into the file, which needs it; make it<br>>> 'static'; remove 'prev' and 'reg_prev' args; rename it to something closer to<br>>> what it actually does.<br>> Done. I refactored this function a bit while moving. However, I decided to<br>> not remove part with 'OP_Realify', even though I think this is deprecared code.<br>> From what I see, this opcode is outdated and should be removed, but not in this<br>> patch. I will send a new patch later, as a refactoring.<br>><br>>><br>>> 6. The function talks about 'covering' index, but covering makes no<br>>> sense in Tarantool. It is not possible to fetch a part of tuple. All<br>>> indexes in Tarantool, from box API point of view, are covering. So<br>>> why is this concept still here? Can we remove it and simplify things?<br>>><br>> It is true that we can get only a whole tuple, however the main feature of the<br>> covering indexes that it contains all needed information about space field.<br><br>Нет, в терминологии sqlite covering означает именно наличие всех колонок. И в<br>этом смысле оно в коде и осталось. Это было нужно, так как индексы в sqlite<br>хранят только ключевые колонки. Если запрос имел колонки не из индекса, нужно<br>было делать поиск в таблице (!= индекс). После covering индексов делать второй<br>поиск было не нужно.</div></div></div></div></blockquote></div><div><br>Используемый индекс содержит все колонки эфемерного спейса, поэтому он covering.</div><div> </div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>Такой смысл вкладывается covering, и в таком смысле он ничего в тарантуле не<br>значит, так как не-covering есть только в виниле, но<br>1) публичного апи для доступа к сырым индексам винила нет;<br>2) все равно нужен поиск в первичном индексе, так как вторичный индекс может<br>содержать удаленный мусор в случае наших LSM-деревьев.</div></div></div></div></blockquote></div><div><br>Ок, понял. Изучу э</div></div></div></div></div></div></blockquote></div><div><br>Ок, понял. Изучу вопрос с covering в SQL подробнее.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> For example, we do not need to look at the space definition to find field type if<br>> we have covering index.<br><br>Да, но это опять же не связано никак с понятием covering.<br><br>> It may be not so important in Tarantool as it was in<br>> SQLite, however this concept is used quite often in SQL code.<br><br>Это легаси от sqlite. В тарантуле все индексы считаются covering.<br><br>> I do not think<br>> that we should fix this issue here.<br><br>Это зависит от того, насколько это усложняет код, чтобы раздрачивать эти<br>covering/не-covering. Судя по коду я так понимаю, что в эфемерный спейс<br>попадают не все колонки оригинального спейса, а только нужные для индекса?<br>Или для индекса нужны как раз все?</div></div></div></div></blockquote></div><div><br>В эфемерный спейс попадают все используемые в запросе колонки, не только те, что используются во where. При этом это скорее всего не все колонки оригинального спейса.</div><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> Also, even though we get a whole tuple, we actually use only needed columns<br>> from the tuple to create a new tuple and push it to the new ephemeral space.<br><br>Если для тапла в эфемерном спейсе хранятся не все колонки оригинального спейса,<br>то это не covering с точки зрения оригинального спейса.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div><br>Мне кажется с точки зрения индекса это covering. Однако, скоре всего ты прав - с точки зрения индекса это covering, а с точки зрения «auto-index» в целом это будет не covering. Исправлю этот момент.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>>>> + sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph, nKeyCol + 1, 0,<br>>>> + (char *)pk_info, P4_KEYINFO);<br>>>> + sqlVdbeAddOp3(v, OP_IteratorOpen, pLevel->iIdxCur, 0, reg_eph);<br>>>> + VdbeComment((v, "for %s", space->def->name));<br>>>><br>>>> /* Fill the automatic index with content */<br>>>> sqlExprCachePush(pParse);<br>>>> @@ -2841,12 +2885,12 @@ tnt_error:<br>>>> * those objects, since there is no opportunity to add schema<br>>>> * indexes on subqueries and views.<br>>>> */<br>>>> - pNew->rSetup = rLogSize + rSize + 4;<br>>>> - if (!pTab->def->opts.is_view &&<br>>>> - pTab->def->id == 0)<br>>>> - pNew->rSetup += 24;<br>>>> - if (pNew->rSetup < 0)<br>>>> - pNew->rSetup = 0;<br>>>> + pNew->rSetup = rLogSize + rSize;<br>>>> + if (!space->def->opts.is_view &&<br>>>> + space->def->id == 0)<br>>>> + pNew->rSetup += 28;<br>>>> + else<br>>>> + pNew->rSetup -= 10;<br>>><br>>> 10. What is this? What are the numbers?<br>> These numbers I got from SQLite. Actually, we should find these numbers<br>> experementally, but I still do not have a proper way to do this.<br>><br>> There is one more place where we should experemetally find a function<br>> of number of tuples in the space which allows us to decide more carefully when<br>> we want to use the optimization. Here it is:<br>> "rSize = DEFAULT_TUPLE_LOG_COUNT;"<br>><br>> As you can see, this function is a constant for now. This is also from SQLite.<br>><br>> In general, we should use statistics here, but we do not have it at the moment.<br>><br>> These numbers are used to decide when to use auto-index. I think that since at<br>> the moment all these values are constants, auto-index is used always. This is<br>> obviously wrong. I plan to fix it as soon as I find a proper way to test<br>> performance.<br><br>Здесь нужен как минимум комментарий в коде, что это за числа. Тот комментарий, что<br>есть, очевидно устарел. Он говорит, что стоимость для view - 4, а у тебя - -10. То<br>есть для view ты считаешь, что лучше создать авто-индекс, чем не создавать, всегда.<br>Нет? Если так, то отрицательная стоимость выглядит не очень правильно.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Изучу этот вопрос подробнее и отпишусь в следующем письме.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>>>> /* TUNING: Each index lookup yields 20 rows in the table. This<br>>>> * is more than the usual guess of 10 rows, since we have no way<br>>>> * of knowing how selective the index will ultimately be. It would<br>>>> @@ -4794,18 +4838,34 @@ sqlWhereEnd(WhereInfo * pWInfo)<br>>>> + continue;<br>>>> + }<br>>>> + /*<br>>>> + * In case we are using auto-indexing, we have<br>>>> + * to change the column number, because in<br>>>> + * ephemeral space columns are in the same order<br>>>> + * as they described in key_def. So, instead of<br>>>> + * the field number, we have to insert index of<br>>>> + * the part with this fieldno.<br>>><br>>> 12. Why are the columns reordered?<br>> See above. Should I add this explanation somewhere is the patch?<br><br>Если в авто-индексе только ключевые колонки, от откуда в result set попадают<br>неключевые колонки, которые в WHERE не участвовали? Делается обратно лукап<br>в оригинальном спейсе что ли?</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>В эферный спейс попадают все колонки, использующиеся в запросе. Поэтому нам больше нет нужды смотреть в оригинальный спейс.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>>>> + */<br>>>> + struct key_def *key_def =<br>>>> + pLevel->pWLoop->index_def->key_def;<br>>>> + uint32_t part_count = key_def->part_count;<br>>>> + for (uint32_t i = 0; i < part_count; ++i) {<br>>>> + if ((int)key_def->parts[i].fieldno == x)<br>>>> + pOp->p2 = i;<br>>>> }<br>>>> }<br>>>> }<br>>>> diff --git a/test/sql-tap/whereF.test.lua b/test/sql-tap/whereF.test.lua<br>>>> index 5a894b748..3235df437 100755<br>>>> --- a/test/sql-tap/whereF.test.lua<br>>>> +++ b/test/sql-tap/whereF.test.lua<br>>>> @@ -90,10 +90,20 @@ test:do_execsql_test(<br>>>><br>>>> -- for _ in X(0, "X!foreach", [=[["tn sql","\n 1 \"SELECT * FROM t1, t2 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e\"\n 2 \"SELECT * FROM t2, t1 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e\"\n 3 \"SELECT * FROM t2 CROSS JOIN t1 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e\"\n"]]=]) do<br>>>> for tn, sql in ipairs({"SELECT * FROM t1, t2 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e",<br>>>> - "SELECT * FROM t2, t1 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e",<br>>>> - "SELECT * FROM t2 CROSS JOIN t1 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e"}) do<br>>>> + "SELECT * FROM t2, t1 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e"}) do<br>>>> test:do_test(<br>>>> - "2."..tn,<br>>>> + "2.1."..tn,<br>>>> + function()<br>>>> + return test:execsql("EXPLAIN QUERY PLAN "..sql)<br>>>> + end, {<br>>>> + '/SEARCH TABLE T1/',<br>>>> + '/SEARCH TABLE T2/'<br>>>> + })<br>>>> +end<br>>>> +<br>>>> +for tn, sql in ipairs({"SELECT * FROM t2 CROSS JOIN t1 WHERE t1.a>? AND t2.d>t1.c AND t1.b=t2.e"}) do<br>>>> + test:do_test(<br>>>> + "2.2."..tn,<br>>>> function()<br>>>> return test:execsql("EXPLAIN QUERY PLAN "..sql)<br>>>> end, {<br>>><br>>> 14. Shouldn't there be a test showing 'AUTOMATIC' keyword in the execution plan?<br>> Actually it does, in the test above.<br><br>Во всем whereF.test.lua файле слово automatic грепается только в каком-то комменте.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Понял, исправлю и отпишу в следующем письме.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c<br>> index c5f79f908..f6b533ab1 100644<br>> --- a/src/box/sql/where.c<br>> +++ b/src/box/sql/where.c<br>> @@ -712,10 +712,61 @@ termCanDriveIndex(WhereTerm * pTerm, /* WHERE clause term to check */<br>> #endif<br>><br>> #ifndef SQL_OMIT_AUTOMATIC_INDEX<br>> +<br>> +/**<br>> + * Generate code that will assemble an index key, adds rowid and stores it in<br>> + * register reg_out. The key will be for index which is an<br>> + * index on table. cursor is the index of a cursor open on the<br>> + * table table and pointing to the entry that needs indexing.<br>> + * cursor must be the cursor of the PRIMARY KEY index.<br><br>Лимит на ширину комента расширен до 80.<br><br>Кроме того, ты читал этот коммент? Что за 'index which is an index on table'?<br>Какой 'table'? Что за 'cursor open on the table table'? Я как будто<br>контрольные фразы из Бегущего По Лезвию читаю.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Понял, исправлю и отпишусь в следующем письме.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>Параметры у тебя описаны в двух местах - вверху и в @param. Должно быть<br>одно место.<br><br>> + *<br>> + * @param parse Parsing context.<br>> + * @param idx_def The index definition for which to generate a key.<br>> + * @param cursor Cursor number from which to take column data.<br>> + * @param reg_out Put the new key into this register if not NULL.<br>> + * @param reg_eph Register holding adress of ephemeral space.<br>> + *<br>> + * @retval Return a register number which is the first in a block<br><br>@retval принимает два аргумента - значение и описание. То есть ты описал, что<br>функция возвращает значение 'Return' с описанием 'a register number ...'. Для<br>описания без конкретного значения есть @return/@returns.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Понял, исправлю.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> + * of registers that holds the elements of the index key. The<br>> + * block of registers has already been deallocated by the time<br>> + * this routine returns.<br>> + */<br>> +static int<br>> +generate_index_key(struct Parse *parse, struct index_def *idx_def,<br><br>Было бы неплохо в названии функции отразить, что она работает только с эфемерными<br>спейсами.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div><br>Понял, исправлю.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> + int cursor, int reg_out, int reg_eph)<br>> +{<br>> + assert(reg_out != 0);<br>> + struct Vdbe *v = parse->pVdbe;<br>> + int col_cnt = idx_def->key_def->part_count;<br>> + int reg_base = sqlGetTempRange(parse, col_cnt + 1);<br>> + for (int j = 0; j < col_cnt; j++) {<br>> + uint32_t tabl_col = idx_def->key_def->parts[j].fieldno;<br>> + sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j);<br>> + /*<br>> + * If the column type is NUMBER but the number<br>> + * is an integer, then it might be stored in the<br>> + * table as an integer (using a compact<br>> + * representation) then converted to REAL by an<br>> + * OP_Realify opcode. But we are getting<br>> + * ready to store this value back into an index,<br>> + * where it should be converted by to INTEGER<br>> + * again. So omit the OP_Realify opcode if<br>> + * it is present<br>> + */<br>> + sqlVdbeDeletePriorOpcode(v, OP_Realify);<br>> + }<br>> + sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph, reg_base + col_cnt);<br>> + sqlVdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt + 1, reg_out);<br>> +<br>> + sqlReleaseTempRange(parse, reg_base, col_cnt);<br>> + return reg_base;<br>> +}<br>> +<br>> /*<br>> - * Generate code to construct the Index object for an automatic index<br>> - * and to set up the WhereLevel object pLevel so that the code generator<br>> - * makes use of the automatic index.<br>> + * Generate code to construct the ephemeral space that contains used in query<br>> + * fields of one of the tables. The index of this ephemeral space will be known<br>> + * as an "automatic index". Also, this functions set up the WhereLevel object<br>> + * pLevel so that the code generator makes use of the automatic index.<br>> */<br>> static void<br>> constructAutomaticIndex(Parse * pParse, /* The parsing context */<br>> @@ -801,9 +852,11 @@ constructAutomaticIndex(Parse * pParse, /* The parsing context */<br>> n = 0;<br>> idxCols = 0;<br>> uint32_t size = sizeof(struct key_part_def) * nKeyCol;<br>> - struct key_part_def *part_def = malloc(size);<br>> + struct region *region = &fiber()->gc;<br>> + size_t used = region_used(region);<br>> + struct key_part_def *part_def = region_alloc(region, size);<br><br>Это раздолбает с ASANом из-за отсутствия выравнивания. Нужен region_alloc_array.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Исправлю, но мне казалочь, что это временый объект и его выравнивание нигде не проверяется.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>Почему не используешь Parse.region? Файберный обязательно где-нибудь проебется<br>почистить и начнет течь.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Понял, исправлю.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> if (part_def == NULL) {<br>> - diag_set(OutOfMemory, size, "malloc", "part_def");<br>> + diag_set(OutOfMemory, size, "region_alloc", "part_def");<br>> pParse->is_aborted = true;<br>> return;<br>> }<br>> @@ -867,7 +919,7 @@ constructAutomaticIndex(Parse * pParse, /* The parsing context */<br>> assert(n == nKeyCol);<br>><br>> struct key_def *key_def = key_def_new(part_def, nKeyCol, false);<br>> - free(part_def);<br>> + region_truncate(region, used);<br><br>Смысла в этом мало. Либо регион и забить на освобожения, либо куча.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div><br>Понял, уберу после замены региона с файберного на парсерный.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c<br>> index e9e936856..f6b533ab1 100644<br>> --- a/src/box/sql/where.c<br>> +++ b/src/box/sql/where.c<br>> @@ -712,10 +712,61 @@ termCanDriveIndex(WhereTerm * pTerm, /* WHERE clause term to check */<br>> #endif<br>><br>> #ifndef SQL_OMIT_AUTOMATIC_INDEX<br>> +<br>> +/**<br>> + * Generate code that will assemble an index key, adds rowid and stores it in<br>> + * register reg_out. The key will be for index which is an<br>> + * index on table. cursor is the index of a cursor open on the<br>> + * table table and pointing to the entry that needs indexing.<br>> + * cursor must be the cursor of the PRIMARY KEY index.<br>> + *<br>> + * @param parse Parsing context.<br>> + * @param idx_def The index definition for which to generate a key.<br><br>Походу тебе достаточно передавать key_def. Весь idx_def не нужен.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div>Понял, исправлю.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div><br>> + * @param cursor Cursor number from which to take column data.<br>> + * @param reg_out Put the new key into this register if not NULL.<br>> + * @param reg_eph Register holding adress of ephemeral space.<br>> + *<br>> + * @retval Return a register number which is the first in a block<br>> + * of registers that holds the elements of the index key. The<br>> + * block of registers has already been deallocated by the time<br>> + * this routine returns.<br>> + */<br>> +static int<br>> +generate_index_key(struct Parse *parse, struct index_def *idx_def,<br>> + int cursor, int reg_out, int reg_eph)<br>> +{<br>> + assert(reg_out != 0);<br>> + struct Vdbe *v = parse->pVdbe;<br>> + int col_cnt = idx_def->key_def->part_count;<br>> + int reg_base = sqlGetTempRange(parse, col_cnt + 1);<br>> + for (int j = 0; j < col_cnt; j++) {<br>> + uint32_t tabl_col = idx_def->key_def->parts[j].fieldno;<br>> + sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j);<br>> + /*<br>> + * If the column type is NUMBER but the number<br>> + * is an integer, then it might be stored in the<br>> + * table as an integer (using a compact<br>> + * representation) then converted to REAL by an<br>> + * OP_Realify opcode. But we are getting<br>> + * ready to store this value back into an index,<br>> + * where it should be converted by to INTEGER<br>> + * again. So omit the OP_Realify opcode if<br>> + * it is present<br>> + */<br>> + sqlVdbeDeletePriorOpcode(v, OP_Realify);<br>> + }<br>> + sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph, reg_base + col_cnt);<br>> + sqlVdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt + 1, reg_out);<br>> +<br>> + sqlReleaseTempRange(parse, reg_base, col_cnt);<br><br>Выглядит как лютый хак - аллоцируется N + 1 временных регистров, но<br>освобождается только N, чтоб в последем че-то сохранить. То есть намеренная<br>утечка временного регистра на время выполнения запроса. Выглядит не очень.<br>Лучше бы это починить отдельным тикетом.</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div><div><br>Это не хак, а скорее баг. Я исправлю количество освождаемых mem на N + 1. А то, что используется N + 1 мем — это нужно исправить на N + 2. При этом, этот мем используется в только в бранче оператора if, который сейчас всегда пропускается т.к. viaCoroutine всегда false в этой функции. Я пока не смог проверить работоспособность этого бранча.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><div class="cl_757201"><div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>-- <br/>Мерген Имеев</div></div></div><div> </div></div></div></div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>-- <br/>Мерген Имеев</div></div></div><div> </div></div></BODY></HTML>