[tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Aug 13 23:24:26 MSK 2018
Thanks for the patch! I have pushed my review fixes in
a separate commit.
Also see 2 comments below.
On 12/08/2018 17:13, Nikita Pettik wrote:
> After struggling with SQLite legacy ON CONFLICT actions, finally we
> decided to remove them. Still, it is allowed to specify action on NULL
> conflict and on whole query action (e.g. INSERT OR IGNORE).
>
> This patch also fixes behaviour of INSERT/UPDATE OR REPLACE statement.
> Now we are looking at each secondary index to find an entry with given
> key in order to delete it (since original space:replace() provides real
> replace only in PK). Almost the same happens for UPDATE OR IGNORE: as
> far as UPDATE causes implicit deletion of old entry and insertion of
> new one, in case of duplicates in secondary indexes we should omit
> removal of old entry.
>
> Finally, removed extra cursors allocations from UPDATE and INSERT
> processing and moved the rest closer to their real usage.
>
> Closes #3566
> Closes #3565
> Part of #3561
> ---
> src/box/sql.c | 7 -
> src/box/sql/build.c | 90 +---
> src/box/sql/insert.c | 827 ++++++++-----------------------
> src/box/sql/main.c | 45 +-
> src/box/sql/parse.y | 30 +-
> src/box/sql/sqliteInt.h | 131 +++--
> src/box/sql/update.c | 248 +++------
> src/box/sql/vdbe.c | 9 +-
> src/box/sql/vdbeaux.c | 11 +-
> src/box/sql/where.c | 5 -
> test/sql-tap/conflict3.test.lua | 402 ---------------
> test/sql-tap/gh-2931-savepoints.test.lua | 2 +-
> test/sql-tap/gh2140-trans.test.lua | 54 +-
> test/sql-tap/gh2964-abort.test.lua | 11 +-
> test/sql-tap/index1.test.lua | 111 +----
> test/sql-tap/null.test.lua | 6 +-
> test/sql-tap/tkt-4a03edc4c8.test.lua | 6 +-
> test/sql-tap/triggerC.test.lua | 2 +-
> test/sql/on-conflict.result | 134 +++--
> test/sql/on-conflict.test.lua | 79 +--
> 20 files changed, 550 insertions(+), 1660 deletions(-)
> delete mode 100755 test/sql-tap/conflict3.test.lua
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index cb120384a..271886ec6 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> +vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
> + int new_tuple_reg,
> + enum on_conflict_action on_conflict,
> + int ignore_label, int *upd_cols)
> {
> - Vdbe *v; /* VDBE under constrution */
> - Index *pIdx; /* Pointer to one of the indices */
> - Index *pPk = 0; /* The PRIMARY KEY index */
> - sqlite3 *db; /* Database connection */
> - int addr1; /* Address of jump instruction */
> - int seenReplace = 0; /* True if REPLACE is used to resolve INT PK conflict */
> - u8 isUpdate; /* True if this is an UPDATE operation */
> - u8 bAffinityDone = 0; /* True if the OP_Affinity operation has been run */
> struct session *user_session = current_session();
> -
> - isUpdate = regOldData != 0;
> - db = pParse->db;
> - v = sqlite3GetVdbe(pParse);
> - assert(v != 0);
> - struct space_def *def = pTab->def;
> - /* This table is not a VIEW */
> + struct sqlite3 *db = parse_context->db;
> + struct Vdbe *v = sqlite3GetVdbe(parse_context);
> + assert(v != NULL);
> + struct space_def *def = tab->def;
> + /* Insertion into VIEW is prohibited. */
> assert(!def->opts.is_view);
> -
> - pPk = sqlite3PrimaryKeyIndex(pTab);
> -
> - /* Record that this module has started */
> - VdbeModuleComment((v, "BEGIN: GenCnstCks(%d,%d,%d,%d,%d)",
> - iDataCur, iIdxCur, regNewData, regOldData, pkChng));
> -
> - enum on_conflict_action override_error = on_conflict->override_error;
> - enum on_conflict_action on_error;
> - /* Test all NOT NULL constraints.
> - */
> + bool is_update = upd_cols != NULL;
> + /* Test all NOT NULL constraints. */
> for (uint32_t i = 0; i < def->field_count; i++) {
> - if (aiChng && aiChng[i] < 0) {
> - /* Don't bother checking for NOT NULL on columns that do not change */
> + /*
> + * Don't bother checking for NOT NULL on columns
> + * that do not change.
> + */
> + if (is_update && upd_cols[i] < 0)
> continue;
> - }
> - if (def->fields[i].is_nullable || pTab->iAutoIncPKey == (int) i)
> - continue; /* This column is allowed to be NULL */
> -
> - on_error = table_column_nullable_action(pTab, i);
> - if (override_error != ON_CONFLICT_ACTION_DEFAULT)
> - on_error = override_error;
> - /* ABORT is a default error action */
> - if (on_error == ON_CONFLICT_ACTION_DEFAULT)
> - on_error = ON_CONFLICT_ACTION_ABORT;
> -
> - struct Expr *dflt = NULL;
> - dflt = space_column_default_expr(pTab->def->id, i);
> - if (on_error == ON_CONFLICT_ACTION_REPLACE && dflt == 0)
> - on_error = ON_CONFLICT_ACTION_ABORT;
> -
> - assert(on_error != ON_CONFLICT_ACTION_NONE);
> - switch (on_error) {
> + /* This column is allowed to be NULL. */
> + if (def->fields[i].is_nullable || tab->iAutoIncPKey == (int) i)
> + continue;
> + enum on_conflict_action on_conflict_nullable =
> + on_conflict != ON_CONFLICT_ACTION_DEFAULT ?
> + on_conflict : table_column_nullable_action(tab, i);
1. Can you remove table_column_nullable_action? It is the single place of its
usage. You can do space_by_id once at the begin of the function and use
the space explicitly both here and below for space_checks_expr_list (that
also would be removed in such a case).
> + /* ABORT is a default error action. */
> + if (on_conflict_nullable == ON_CONFLICT_ACTION_DEFAULT)
> + on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
> + struct Expr *dflt = space_column_default_expr(tab->def->id, i);
> + if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE &&
> + dflt == NULL)
> + on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
> + switch (on_conflict_nullable) {
> case ON_CONFLICT_ACTION_ABORT:
> - sqlite3MayAbort(pParse);
> - /* Fall through */
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 3fdf5a9af..51cc5cef3 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -152,34 +144,17 @@ sqlite3Update(Parse * pParse, /* The parser context */
> }
>
> struct space_def *def = pTab->def;
> -
> - /* Allocate a cursors for the main database table and for all indices.
> - * The index cursors might not be used, but if they are used they
> - * need to occur right after the database cursor. So go ahead and
> - * allocate enough space, just in case.
> - */
> - pTabList->a[0].iCursor = iBaseCur = iDataCur = pParse->nTab++;
> - iIdxCur = iDataCur + 1;
> - pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab);
> - for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) {
> - if (sql_index_is_primary(pIdx) && pPk != 0) {
> - iDataCur = pParse->nTab;
> - pTabList->a[0].iCursor = iDataCur;
> - }
> - pParse->nTab++;
> - }
> -
> - /* Allocate space for aXRef[], aRegIdx[], and aToOpen[].
> - * Initialize aXRef[] and aToOpen[] to their default values.
> - */
> - aXRef = sqlite3DbMallocRawNN(db, sizeof(int) *
> - (def->field_count + nIdx) + nIdx + 2);
> - if (aXRef == 0)
> + int nIdx = 0;
> + for (struct Index *idx = pTab->pIndex; idx != NULL;
> + idx = idx->pNext, nIdx++);
2. In the same function below we do space_by_id from the
result of which we can get space->index_count. It is not?
> + /* Allocate cursor on primary index. */
> + int pk_cursor = pParse->nTab++;
> + pTabList->a[0].iCursor = pk_cursor;
> + pPk = is_view ? NULL : sqlite3PrimaryKeyIndex(pTab);
> +
> + aXRef = sqlite3DbMallocRawNN(db, sizeof(int) * def->field_count);
> + if (aXRef == NULL)
> goto update_cleanup;
> - aRegIdx = aXRef + def->field_count;
> - aToOpen = (u8 *) (aRegIdx + nIdx);
> - memset(aToOpen, 1, nIdx + 1);
> - aToOpen[nIdx + 1] = 0;
> for (i = 0; i < (int)def->field_count; i++)
> aXRef[i] = -1;
>
More information about the Tarantool-patches
mailing list