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

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

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

>     > 3) Can I get rid of OP_Realify?
>     > I think I can, but decided to do this no in this patch.
> 
>     Ты пробовал его просто дропнуть? Ломается что-то?
> 
> 
> Пробовал, ничего не ломается. Если посмотреть на все места использования OP_Realify (их два) и посмотреть на старый код, еще до удалаления типа Real из SQL, то там видно, что этот опкод использовался для преобразования INT в REAL. Когда REAL быз заменен на NUMBER это преобразование осталось, хотя, как мне кажется, в нем небыло нужды. После добавления DOUBLE в паре мест этот опкод все еще существует, однако используется неправильно. Я думаю этот опкод больше ненужен. Однако, мне кажется его стоит убрать отдельным патчем вне этой задачи.

На это можно было бы создать тикет.

>     > On Wed, Sep 09, 2020 at 11:58:09PM +0200, Vladislav Shpilevoy wrote:
>     >> Hi! Thanks for the patch!
>     >>
>     >> See 14 comments below.
>     >>
>     >> On 04.09.2020 13:53, imeevma@tarantool.org </compose?To=imeevma@tarantool.org> wrote:
>     >>> This patch enables the "auto-index" optimization in SQL. The auto-index
>     >>> is actually an ephemeral space that contain some columns of the original
>     >>> space.
>     >>
>     >> 1. In the patch the code calls the index 'covering' meaning that it
>     >> contains all the columns. What is true?
>     > Both, I think, since this index is the PK of mentioned ephemeral space.
> 
>     Что значит both? Индекс либо covering (содержит все колонки), либо нет (содержит
>     не все).
> 
> 
> Суть в том, что idx_def содержит не все колонки оригинального спейса, однако индекс создаваемый для эфемерного спейса содержит все колонки. При этом, создание idx_def не приводит к созданию индекса.
>  
> Я попробую описать весь механизм:
> 
>  1. Планировщик определяет, что будет использоваться автоматический индекс.
>  2. Создается idx_def, который содержит все использующиеся в запросе колонки оригинального спейса. Не только те, которые используются во where. Это делается для того, что бы больше не обращаться к оригинальному спейсу, а работать только с эфемерным спейсом. Этот idx_def не используется для создания индекса.

То есть если допустим в оригинальном спейсе были колонки A, B, C, D, и я в запросе
использовал SELECT A WHERE C, D, то в эфемерном спейсе будут A, C, D, так? 'B' не
будет.

>  3. Создается эфемерный спейс на основе созданного ранее idx_def. Помимо колонок оригинального спейса добавляется rowid, т.к. возможны случаи, когда значения во всех колонках совпадает в нескольких записях. При этом, колонки в эфемерном спейсе расположены в том же порядке, в каком они описаны в индексе.  Т.е. они, скорее всего, расположены не в том же порядке, в каком они расположены в оригинальном спейсе.
>  4. Для созданного эфемерного спейса создается индекс, которые является covering. Именно поэтому в некоторых местах написано, что создается covering index.
>  5. Т.к. планировщик посчитал, что будет использоваться автоиндекс, то в качестве спейса из которого будут выбраны таплы мы будем использовать созданный нами эфемерный спейс. Однако, во время построения vdbe-кода в качестве fieldno было использовано расположение колонок в оригинальном спейсе. Поэтому, в случае использования автоиндекса мы заменяем fieldno оригинального спейса в OP_Column на fieldno в эфемерном спейсе используя созданный ранее idx_def.
> 
>     >>> Co-authored-by: Mergen Imeev <imeevma@gmail.com </compose?To=imeevma@gmail.com>>
>     >>> Co-authored-by: Timur Safin <tsafin@tarantool.org </compose?To=tsafin@tarantool.org>>
>     >>> ---
>     >>> https://github.com/tarantool/tarantool/issues/4933
>     >>> https://github.com/tarantool/tarantool/tree/imeevma/gh-4933-autoindex
>     >>>
>     >>> @ChangeLog
>     >>> - "Auto-index" optimization is now enabled (gh-4933).
>     >>>
>     >>> src/box/CMakeLists.txt | 2 +-
>     >>> src/box/sql.c | 2 +-
>     >>> src/box/sql/delete.c | 16 ++--
>     >>> src/box/sql/sqlInt.h | 8 +-
>     >>> src/box/sql/where.c | 170 +++++++++++++++++++++++------------
>     >>> src/box/sql/wherecode.c | 13 +--
>     >>> test/sql-tap/whereF.test.lua | 16 +++-
>     >>> 7 files changed, 151 insertions(+), 76 deletions(-)
>     >>>
>     >>> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
>     >>> index b8b2689d2..7e3ad0e22 100644
>     >>> --- a/src/box/CMakeLists.txt
>     >>> +++ b/src/box/CMakeLists.txt
>     >>> @@ -217,7 +217,7 @@ add_library(box STATIC
>     >>> if(CMAKE_BUILD_TYPE STREQUAL "Debug")
>     >>> add_definitions(-DSQL_DEBUG=1)
>     >>> endif()
>     >>> -add_definitions(-DSQL_OMIT_AUTOMATIC_INDEX=1 -DSQL_TEST=1)
>     >>> +add_definitions(-DSQL_TEST=1)
>     >>
>     >> 3. I still see SQL_OMIT_AUTOMATIC_INDEX in src/box/sql/where.c.
>     > I think the original idea was to make an option to disable this optimization.
>     > Since such thing is already exists, I decided to not remove it.
> 
>     У нас нет конфигурации опций сборки SQL. Мы их все выпиливаем, так как билды
>     у нас под один конкретный набор опций. Есть runtime опции, но это ничего общего
>     с макросами не имеет. Если этот макрос более не указывается, то надо его
>     удалить отовсюду.
> 
> 
> Понял, исправлю. Я думаю в этом случае я добавлю новую опцию в session_settings, которая будет отключать эту оптимизацию.

Не надо пока ничего добавлять. Мы либо включаем ее сейчас, либо не
включаем. Если автоиндексы так вредны, что надо их выключать через
сессию, то не вижу вообще смысла их пушать, пока не доработано.
Ничем не будет отличаться от того, что сейчас в мастере, когда они даже
не компилируются.

>     >>> set(EXT_SRC_DIR ${CMAKE_SOURCE_DIR}/extra)
>     >>> set(EXT_BIN_DIR ${CMAKE_BINARY_DIR}/extra)
>     >>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>     >>> index 68abd1f58..57478c129 100644
>     >>> --- a/src/box/sql/delete.c
>     >>> +++ b/src/box/sql/delete.c
>     >>> @@ -546,24 +546,25 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
>     >>> }
>     >>>
>     >>> int
>     >>> -sql_generate_index_key(struct Parse *parse, struct index *index, int cursor,
>     >>> - int reg_out, struct index *prev, int reg_prev)
>     >>> +sql_generate_index_key(struct Parse *parse, struct index_def *idx_def,
>     >>> + int cursor, int reg_out, struct index *prev,
>     >>> + int reg_prev, int reg_eph)
>     >>
>     >> 4. The function has nothing to do with ephemeral spaces. It just does not
>     >> care whether its space is ephemeral. Its task is to just assemble a key,
>     >> not caring about space type. Why did you make it always work with an
>     >> ephemeral space? Won't this will affect normal spaces - they don't need
>     >> OP_NextIdEphemeral or whatever else is related to ephemerals.
>     >>
>     >> In the end of the review I realized the function is never used for anything
>     >> except automatic index. Moreover, prev and reg_prev are NULL and 0 always.
>     >> I suggest to move this function into the file, which needs it; make it
>     >> 'static'; remove 'prev' and 'reg_prev' args; rename it to something closer to
>     >> what it actually does.
>     > Done. I refactored this function a bit while moving. However, I decided to
>     > not remove part with 'OP_Realify', even though I think this is deprecared code.
>     > From what I see, this opcode is outdated and should be removed, but not in this
>     > patch. I will send a new patch later, as a refactoring.
>     >
>     >>
>     >> 6. The function talks about 'covering' index, but covering makes no
>     >> sense in Tarantool. It is not possible to fetch a part of tuple. All
>     >> indexes in Tarantool, from box API point of view, are covering. So
>     >> why is this concept still here? Can we remove it and simplify things?
>     >>
>     > It is true that we can get only a whole tuple, however the main feature of the
>     > covering indexes that it contains all needed information about space field.
> 
>     Нет, в терминологии sqlite covering означает именно наличие всех колонок. И в
>     этом смысле оно в коде и осталось. Это было нужно, так как индексы в sqlite
>     хранят только ключевые колонки. Если запрос имел колонки не из индекса, нужно
>     было делать поиск в таблице (!= индекс). После covering индексов делать второй
>     поиск было не нужно.
> 
> 
> Используемый индекс содержит все колонки эфемерного спейса, поэтому он covering.

Да, с точки зрение эфемерного спейса похоже действительно covering.