From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id AB08B2896C for ; Mon, 13 Aug 2018 16:24:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rx5mxpELMuL9 for ; Mon, 13 Aug 2018 16:24:29 -0400 (EDT) Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D6CF227961 for ; Mon, 13 Aug 2018 16:24:28 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes References: <75ebea5a7bc472e83a844cc98fc6fb88369e4b10.1534001739.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <317b019c-61ad-83f4-5a47-3818e90fa4a8@tarantool.org> Date: Mon, 13 Aug 2018 23:24:26 +0300 MIME-Version: 1.0 In-Reply-To: <75ebea5a7bc472e83a844cc98fc6fb88369e4b10.1534001739.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Nikita Pettik 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; >