* [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table @ 2018-08-12 14:12 Nikita Pettik 2018-08-12 14:12 ` [tarantool-patches] [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN Nikita Pettik ` (11 more replies) 0 siblings, 12 replies; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:12 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Branch: https://github.com/tarantool/tarantool/tree/np/cleanup-in-Index-and-Table Issues: https://github.com/tarantool/tarantool/issues/3561 https://github.com/tarantool/tarantool/issues/3565 https://github.com/tarantool/tarantool/issues/3566 Current patch-set is preliminary to finishing DD integration. It gets rid off all functional fields from original SQLite's struct Index and struct Table, which can be substituted with corresponding fields from server structures or be calculated using them. It is worth mentioning ninth patch in series which removes support of ON CONFLICT error actions related to index uniqueness. Firstly, alongside with it INSERT and UPDATE code generation has been slightly simplified: removed a lot of excess register allocations; moved cursors' allocations closer to their usages (auxiliary cursors on secondary indexes are required only to process REPLACE or IGNORE conflict actions); significantly reworked function which generates code to process constraint checks. To sump up, all these changes result in fixing assertion fault during INSERT OR REPLACE and incorrect behavior of UPDATE OR IGNORE statements. * To those who will review * I deliberately separated all commits on removing fields since this way seems to be easier to review and understand provided changes. After review I can squash some of them, if you want so. Nikita Pettik (10): sql: remove suport of ALTER TABLE ADD COLUMN sql: remove string of fields collation from Table sql: remove index hash from struct Table sql: remove flags from struct Table sql: remove affinity string of columns from Index sql: completely remove support of partial indexes sql: remove index type from struct Index sql: use secondary indexes to process OP_Delete sql: disable ON CONFLICT actions for indexes sql: move autoincrement field number to server src/box/space_def.c | 3 + src/box/space_def.h | 5 + src/box/sql.c | 37 +- src/box/sql/alter.c | 185 ------ src/box/sql/analyze.c | 2 +- src/box/sql/build.c | 263 +++------ src/box/sql/delete.c | 20 +- src/box/sql/insert.c | 968 ++++++++----------------------- src/box/sql/main.c | 45 +- src/box/sql/parse.y | 42 +- src/box/sql/pragma.c | 10 +- src/box/sql/prepare.c | 9 +- src/box/sql/select.c | 4 +- src/box/sql/sqliteInt.h | 198 ++++--- src/box/sql/tarantoolInt.h | 16 +- src/box/sql/update.c | 252 ++------ src/box/sql/vdbe.c | 11 +- src/box/sql/vdbeaux.c | 11 +- src/box/sql/vdbemem.c | 3 +- src/box/sql/where.c | 64 +- src/box/sql/wherecode.c | 7 +- 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 ++- 31 files changed, 733 insertions(+), 2229 deletions(-) delete mode 100755 test/sql-tap/conflict3.test.lua -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik @ 2018-08-12 14:12 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:12 ` [tarantool-patches] [PATCH 02/10] sql: remove string of fields collation from Table Nikita Pettik ` (10 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:12 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik We disabled ALTER TABLE ADD COLUMN long ago (i.e. banned in parser ability to process this statement), but internal functions for handling this routine have remained. This patch removes them as well. Part of #3561 --- src/box/sql/alter.c | 185 ------------------------------------------------ src/box/sql/build.c | 16 +---- src/box/sql/parse.y | 12 ++-- src/box/sql/sqliteInt.h | 7 +- 4 files changed, 7 insertions(+), 213 deletions(-) diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index c6463c79e..349589be4 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -37,12 +37,6 @@ #include "src/box/session.h" #include "tarantoolInt.h" -/* - * The code in this file only exists if we are not omitting the - * ALTER TABLE logic from the build. - */ -#ifndef SQLITE_OMIT_ALTERTABLE - /* * Generate code to drop and reload the internal representation of table * pTab from the database, including triggers. @@ -127,183 +121,6 @@ sqlite3AlterRenameTable(Parse * pParse, /* Parser context. */ user_session->sql_flags = savedDbFlags; } -/* - * This function is called after an "ALTER TABLE ... ADD" statement - * has been parsed. Argument pColDef contains the text of the new - * column definition. - * - * The Table structure pParse->pNewTable was extended to include - * the new column during parsing. - */ -void -sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef) -{ - /* This function is not implemented yet #3075. */ - unreachable(); - - Table *pNew; /* Copy of pParse->pNewTable */ - Table *pTab; /* Table being altered */ - const char *zTab; /* Table name */ - Expr *pDflt; /* Default value for the new column */ - sqlite3 *db; /* The database connection; */ - Vdbe *v = pParse->pVdbe; /* The prepared statement under construction */ - - db = pParse->db; - if (pParse->nErr || db->mallocFailed) - return; - assert(v != 0); - pNew = pParse->pNewTable; - assert(pNew); - - /* Skip the "sqlite_altertab_" prefix on the name. */ - zTab = &pNew->def->name[16]; - assert(pNew->def != NULL); - pDflt = space_column_default_expr(pNew->def->id, - pNew->def->field_count - 1); - pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);; - assert(pTab); - - /* If the default value for the new column was specified with a - * literal NULL, then set pDflt to 0. This simplifies checking - * for an SQL NULL default below. - */ - assert(pDflt == 0 || pDflt->op == TK_SPAN); - if (pDflt && pDflt->pLeft->op == TK_NULL) { - pDflt = 0; - } - - /* Check that the new column is not specified as PRIMARY KEY or UNIQUE. - * If there is a NOT NULL constraint, then the default value for the - * column must not be NULL. - */ - struct Index *pk = sqlite3PrimaryKeyIndex(pTab); - assert(pk != NULL); - struct key_def *pk_key_def = pk->def->key_def; - if (key_def_find(pk_key_def, pNew->def->field_count - 1) != NULL) { - sqlite3ErrorMsg(pParse, "Cannot add a PRIMARY KEY column"); - return; - } - if (pNew->pIndex) { - sqlite3ErrorMsg(pParse, "Cannot add a UNIQUE column"); - return; - } - assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable == - action_is_nullable(pNew->def->fields[ - pNew->def->field_count - 1].nullable_action)); - if (!pNew->def->fields[pNew->def->field_count - 1].is_nullable && - pDflt == NULL) { - sqlite3ErrorMsg(pParse, - "Cannot add a NOT NULL column with default value NULL"); - return; - } - - /* Ensure the default expression is something that sqlite3ValueFromExpr() - * can handle (i.e. not CURRENT_TIME etc.) - */ - if (pDflt) { - sqlite3_value *pVal = 0; - int rc; - rc = sqlite3ValueFromExpr(db, pDflt, - AFFINITY_BLOB, &pVal); - assert(rc == SQLITE_OK || rc == SQLITE_NOMEM); - if (rc != SQLITE_OK) { - assert(db->mallocFailed == 1); - return; - } - if (!pVal) { - sqlite3ErrorMsg(pParse, - "Cannot add a column with non-constant default"); - return; - } - sqlite3ValueFree(pVal); - } - - /* Modify the CREATE TABLE statement. */ - /* TODO: Adopt for Tarantool. */ - (void)pColDef; - - /* Reload the schema of the modified table. */ - reloadTableSchema(pParse, pTab, pTab->def->name); -} - -/* - * This function is called by the parser after the table-name in - * an "ALTER TABLE <table-name> ADD" statement is parsed. Argument - * pSrc is the full-name of the table being altered. - * - * This routine makes a (partial) copy of the Table structure - * for the table being altered and sets Parse.pNewTable to point - * to it. Routines called by the parser as the column definition - * is parsed (i.e. sqlite3AddColumn()) add the new Column data to - * the copy. The copy of the Table structure is deleted by tokenize.c - * after parsing is finished. - * - * Routine sqlite3AlterFinishAddColumn() will be called to complete - * coding the "ALTER TABLE ... ADD" statement. - */ -void -sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc) -{ - /* This function is not implemented yet #3075. */ - unreachable(); - - Table *pNew; - Table *pTab; - Vdbe *v; - sqlite3 *db = pParse->db; - - /* Look up the table being altered. */ - assert(pParse->pNewTable == 0); - if (db->mallocFailed) - goto exit_begin_add_column; - pTab = sqlite3LocateTable(pParse, 0, pSrc->a[0].zName); - if (pTab == NULL) - goto exit_begin_add_column; - - /* Make sure this is not an attempt to ALTER a view. */ - if (pTab->def->opts.is_view) { - sqlite3ErrorMsg(pParse, "Cannot add a column to a view"); - goto exit_begin_add_column; - } - - assert(pTab->addColOffset > 0); - - /* Put a copy of the Table struct in Parse.pNewTable for the - * sqlite3AddColumn() function and friends to modify. But modify - * the name by adding an "sqlite_altertab_" prefix. By adding this - * prefix, we insure that the name will not collide with an existing - * table because user table are not allowed to have the "sqlite_" - * prefix on their name. - */ - pNew = (Table *) sqlite3DbMallocZero(db, sizeof(Table)); - if (!pNew) - goto exit_begin_add_column; - pNew->def = space_def_dup(pTab->def); - if (pNew->def == NULL) { - sqlite3DbFree(db, pNew); - sqlite3OomFault(db); - goto exit_begin_add_column; - } - pParse->pNewTable = pNew; - assert(pNew->def->field_count > 0); - /* FIXME: pNew->zName = sqlite3MPrintf(db, "sqlite_altertab_%s", pTab->zName); */ - /* FIXME: if (!pNew->aCol || !pNew->zName) { */ - for (uint32_t i = 0; i < pNew->def->field_count; i++) - pNew->def->fields[i] = field_def_default; - pNew->pSchema = db->pSchema; - pNew->addColOffset = pTab->addColOffset; - pNew->nTabRef = 1; - - /* Begin a transaction. */ - sql_set_multi_write(pParse, false); - v = sqlite3GetVdbe(pParse); - if (!v) - goto exit_begin_add_column; - exit_begin_add_column: - sqlite3SrcListDelete(db, pSrc); - return; -} - /* * This function implements part of the ALTER TABLE command. * The first argument is the text of a CREATE TABLE or CREATE INDEX command. @@ -444,5 +261,3 @@ rename_trigger(sqlite3 *db, char const *sql_stmt, char const *table_name, table_name, tname.z + tname.n); return new_sql_stmt; } - -#endif /* SQLITE_ALTER_TABLE */ diff --git a/src/box/sql/build.c b/src/box/sql/build.c index cdf2bfcc9..f3546b794 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1683,7 +1683,6 @@ resolve_link(struct Parse *parse_context, const struct space_def *def, */ void sqlite3EndTable(Parse * pParse, /* Parse context */ - Token * pCons, /* The ',' token after the last column defn. */ Token * pEnd, /* The ')' before options in the CREATE TABLE */ Select * pSelect /* Select from a "CREATE ... AS SELECT" */ ) @@ -1749,19 +1748,6 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } pParse->pNewTable = NULL; current_session()->sql_flags |= SQLITE_InternChanges; - -#ifndef SQLITE_OMIT_ALTERTABLE - if (!p->def->opts.is_view) { - const char *zName = (const char *)pParse->sNameToken.z; - int nName; - assert(!pSelect && pCons && pEnd); - if (pCons->z == 0) { - pCons = pEnd; - } - nName = (int)((const char *)pCons->z - zName); - p->addColOffset = 13 + sqlite3Utf8CharLen(zName, nName); - } -#endif goto finish; } /* @@ -1931,7 +1917,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin, } /* Use sqlite3EndTable() to add the view to the Tarantool. */ - sqlite3EndTable(parse_context, 0, &end, 0); + sqlite3EndTable(parse_context, &end, 0); create_view_fail: sqlite3DbFree(db, sel_tab); diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 200d57a7d..58a6bf39a 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -176,11 +176,11 @@ createkw(A) ::= CREATE(A). {disableLookaside(pParse);} ifnotexists(A) ::= . {A = 0;} ifnotexists(A) ::= IF NOT EXISTS. {A = 1;} -create_table_args ::= LP columnlist conslist_opt(X) RP(E). { - sqlite3EndTable(pParse,&X,&E,0); +create_table_args ::= LP columnlist conslist_opt RP(E). { + sqlite3EndTable(pParse,&E,0); } create_table_args ::= AS select(S). { - sqlite3EndTable(pParse,0,0,S); + sqlite3EndTable(pParse,0,S); sql_select_delete(pParse->db, S); } columnlist ::= columnlist COMMA columnname carglist. @@ -327,8 +327,8 @@ init_deferred_pred_opt(A) ::= . {A = 0;} init_deferred_pred_opt(A) ::= INITIALLY DEFERRED. {A = 1;} init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE. {A = 0;} -conslist_opt(A) ::= . {A.n = 0; A.z = 0;} -conslist_opt(A) ::= COMMA(A) conslist. +conslist_opt ::= . +conslist_opt ::= COMMA conslist. conslist ::= conslist tconscomma tcons. conslist ::= tcons. tconscomma ::= COMMA. {pParse->constraintName.n = 0;} @@ -1443,7 +1443,6 @@ cmd ::= ANALYZE. {sqlite3Analyze(pParse, 0);} cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... //////////////////////////////// -%ifndef SQLITE_OMIT_ALTERTABLE cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { sqlite3AlterRenameTable(pParse,X,&Z); } @@ -1470,7 +1469,6 @@ cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { /* } */ /* kwcolumn_opt ::= . */ /* kwcolumn_opt ::= COLUMNKW. */ -%endif SQLITE_OMIT_ALTERTABLE //////////////////////// COMMON TABLE EXPRESSIONS //////////////////////////// %type with {With*} diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 189d16150..ddedfbcb4 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1888,9 +1888,6 @@ struct Table { */ LogEst tuple_log_count; u8 tabFlags; /* Mask of TF_* values */ -#ifndef SQLITE_OMIT_ALTERTABLE - int addColOffset; /* Offset in CREATE TABLE stmt to add a new column */ -#endif Schema *pSchema; /* Schema that contains this table */ Table *pNextZombie; /* Next on the Parse.pZombieTab list */ /** Space definition with Tarantool metadata. */ @@ -3501,7 +3498,7 @@ void sqlite3AddCollateType(Parse *, Token *); struct coll * sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id); -void sqlite3EndTable(Parse *, Token *, Token *, Select *); +void sqlite3EndTable(Parse *, Token *, Select *); /** * Create cursor which will be positioned to the space/index. @@ -4525,8 +4522,6 @@ int sqlite3ResolveOrderGroupBy(Parse *, Select *, ExprList *, const char *); void sqlite3ColumnDefault(Vdbe *v, struct space_def *def, int i, int ireg); -void sqlite3AlterFinishAddColumn(Parse *, Token *); -void sqlite3AlterBeginAddColumn(Parse *, SrcList *); char* rename_table(sqlite3 *, const char *, const char *, bool *); char* rename_trigger(sqlite3 *, char const *, char const *, bool *); /** -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN 2018-08-12 14:12 ` [tarantool-patches] [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Hi! Thanks for the patch! I have pushed my review fixes in a separate commit. On 12/08/2018 17:12, Nikita Pettik wrote: > We disabled ALTER TABLE ADD COLUMN long ago (i.e. banned in parser > ability to process this statement), but internal functions for handling > this routine have remained. This patch removes them as well. > > Part of #3561 > --- > src/box/sql/alter.c | 185 ------------------------------------------------ > src/box/sql/build.c | 16 +---- > src/box/sql/parse.y | 12 ++-- > src/box/sql/sqliteInt.h | 7 +- > 4 files changed, 7 insertions(+), 213 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 02/10] sql: remove string of fields collation from Table 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik 2018-08-12 14:12 ` [tarantool-patches] [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN Nikita Pettik @ 2018-08-12 14:12 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:12 ` [tarantool-patches] [PATCH 03/10] sql: remove index hash from struct Table Nikita Pettik ` (9 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:12 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Part of #3561 --- src/box/sql/build.c | 1 - src/box/sql/insert.c | 71 +++++++++++-------------------------------------- src/box/sql/sqliteInt.h | 14 +++++++--- src/box/sql/update.c | 2 +- 4 files changed, 28 insertions(+), 60 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index f3546b794..48ced1766 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -344,7 +344,6 @@ deleteTable(sqlite3 * db, Table * pTable) /* Delete the Table structure itself. */ sqlite3HashClear(&pTable->idxHash); - sqlite3DbFree(db, pTable->zColAff); assert(pTable->def != NULL); /* Do not delete pTable->def allocated on region. */ if (!pTable->def->opts.is_temporary) diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 13cb61ecf..50c30fd82 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -124,59 +124,21 @@ sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) return aff; } -/* - * Compute the affinity string for table pTab, if it has not already been - * computed. As an optimization, omit trailing AFFINITY_BLOB affinities. - * - * If the affinity exists (if it is no entirely AFFINITY_BLOB values) and - * if iReg>0 then code an OP_Affinity opcode that will set the affinities - * for register iReg and following. Or if affinities exists and iReg==0, - * then just set the P4 operand of the previous opcode (which should be - * an OP_MakeRecord) to the affinity string. - * - * A column affinity string has one character per column: - * - * Character Column affinity - * ------------------------------ - * 'A' BLOB - * 'B' TEXT - * 'C' NUMERIC - * 'D' INTEGER - * 'E' REAL - */ void -sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg) +sql_emit_table_affinity(struct Vdbe *v, struct space_def *def, int reg) { - int i; - char *zColAff = pTab->zColAff; - if (zColAff == 0) { - sqlite3 *db = sqlite3VdbeDb(v); - zColAff = - (char *)sqlite3DbMallocRaw(0, - pTab->def->field_count + 1); - if (!zColAff) { - sqlite3OomFault(db); - return; - } - - for (i = 0; i < (int)pTab->def->field_count; i++) { - char affinity = pTab->def->fields[i].affinity; - zColAff[i] = affinity; - } - do { - zColAff[i--] = 0; - } while (i >= 0 && zColAff[i] == AFFINITY_BLOB); - pTab->zColAff = zColAff; - } - i = sqlite3Strlen30(zColAff); - if (i) { - if (iReg) { - sqlite3VdbeAddOp4(v, OP_Affinity, iReg, i, 0, zColAff, - i); - } else { - sqlite3VdbeChangeP4(v, -1, zColAff, i); - } + assert(reg > 0); + sqlite3 *db = sqlite3VdbeDb(v); + uint32_t field_count = def->field_count; + char *colls_aff = (char *)sqlite3DbMallocZero(db, field_count + 1); + if (colls_aff == NULL) { + sqlite3OomFault(db); + return; } + for (uint32_t i = 0; i < field_count; ++i) + colls_aff[i] = def->fields[i].affinity; + sqlite3VdbeAddOp4(v, OP_Affinity, reg, field_count, 0, colls_aff, + P4_DYNAMIC); } /** @@ -704,9 +666,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ * If this is a real table, attempt conversions as required by the * table column affinities. */ - if (!is_view) { - sqlite3TableAffinity(v, pTab, regCols + 1); - } + if (!is_view) + sql_emit_table_affinity(v, pTab->def, regCols + 1); /* Fire BEFORE or INSTEAD OF triggers */ vdbe_code_row_trigger(pParse, trigger, TK_INSERT, 0, @@ -1188,7 +1149,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ if (aRegIdx[ix] == 0) continue; /* Skip indices that do not change */ if (bAffinityDone == 0) { - sqlite3TableAffinity(v, pTab, regNewData+1); + sql_emit_table_affinity(v, pTab->def, regNewData + 1); bAffinityDone = 1; } iThisCur = iIdxCur + ix; @@ -1239,7 +1200,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ pIdx->def->key_def->parts[0].fieldno; reg_pk = regNewData + 1 + fieldno; - if (pTab->zColAff[fieldno] == + if (pTab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { int skip_if_null = sqlite3VdbeMakeLabel(v); if ((pTab->tabFlags & TF_Autoincrement) != 0) { diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index ddedfbcb4..9c861518f 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1874,8 +1874,6 @@ struct Savepoint { */ struct Table { Index *pIndex; /* List of SQL indexes on this table. */ - char *zColAff; /* String defining the affinity of each column */ - /* ... also used as column name list in a VIEW */ Hash idxHash; /* All (named) indices indexed by name */ u32 nTabRef; /* Number of pointers to this Table */ i16 iAutoIncPKey; /* If PK is marked INTEGER PRIMARY KEY AUTOINCREMENT, store @@ -4318,7 +4316,17 @@ const char *sqlite3IndexAffinityStr(sqlite3 *, Index *); char * sql_index_affinity_str(struct sqlite3 *db, struct index_def *def); -void sqlite3TableAffinity(Vdbe *, Table *, int); +/** + * Code an OP_Affinity opcode that will set affinities + * for given range of register starting from @reg. + * + * @param v VDBE. + * @param def Definition of table to be used. + * @param reg Register where affinities will be placed. + */ +void +sql_emit_table_affinity(struct Vdbe *v, struct space_def *def, int reg); + char sqlite3CompareAffinity(Expr * pExpr, char aff2); int sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity); diff --git a/src/box/sql/update.c b/src/box/sql/update.c index 9e9fa3a79..e2324c354 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -494,7 +494,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ * verified. One could argue that this is wrong. */ if (tmask & TRIGGER_BEFORE) { - sqlite3TableAffinity(v, pTab, regNew); + sql_emit_table_affinity(v, pTab->def, regNew); vdbe_code_row_trigger(pParse, trigger, TK_UPDATE, pChanges, TRIGGER_BEFORE, pTab, regOldPk, on_error, labelContinue); -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 02/10] sql: remove string of fields collation from Table 2018-08-12 14:12 ` [tarantool-patches] [PATCH 02/10] sql: remove string of fields collation from Table Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! I have pushed my review fixes in a separate commit. Comment about my fix: you do not need to set oomfault from sqlite malloc wrappers - it is already done inside. On 12/08/2018 17:12, Nikita Pettik wrote: > Part of #3561 > --- > src/box/sql/build.c | 1 - > src/box/sql/insert.c | 71 +++++++++++-------------------------------------- > src/box/sql/sqliteInt.h | 14 +++++++--- > src/box/sql/update.c | 2 +- > 4 files changed, 28 insertions(+), 60 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 03/10] sql: remove index hash from struct Table 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik 2018-08-12 14:12 ` [tarantool-patches] [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN Nikita Pettik 2018-08-12 14:12 ` [tarantool-patches] [PATCH 02/10] sql: remove string of fields collation from Table Nikita Pettik @ 2018-08-12 14:12 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 04/10] sql: remove flags " Nikita Pettik ` (8 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:12 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Part of #3561 --- src/box/sql/build.c | 41 +++++++---------------------------------- src/box/sql/sqliteInt.h | 1 - 2 files changed, 7 insertions(+), 35 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 48ced1766..78206a1ed 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -197,8 +197,11 @@ sqlite3LocateIndex(sqlite3 * db, const char *zName, const char *zTable) if (pTab == NULL) return NULL; - - return sqlite3HashFind(&pTab->idxHash, zName); + for (struct Index *idx = pTab->pIndex; idx != NULL; idx = idx->pNext) { + if (strcmp(zName, idx->def->name) == 0) + return idx; + } + return NULL; } /* @@ -224,12 +227,8 @@ void sqlite3UnlinkAndDeleteIndex(sqlite3 * db, Index * pIndex) { assert(pIndex != 0); - assert(&pIndex->pTable->idxHash); struct session *user_session = current_session(); - - pIndex = sqlite3HashInsert(&pIndex->pTable->idxHash, - pIndex->def->name, 0); if (ALWAYS(pIndex)) { if (pIndex->pTable->pIndex == pIndex) { pIndex->pTable->pIndex = pIndex->pNext; @@ -331,19 +330,8 @@ deleteTable(sqlite3 * db, Table * pTable) /* Delete all indices associated with this table. */ for (pIndex = pTable->pIndex; pIndex; pIndex = pNext) { pNext = pIndex->pNext; - if ((db == 0 || db->pnBytesFreed == 0)) { - char *zName = pIndex->def->name; - TESTONLY(Index * - pOld =) sqlite3HashInsert(&pTable->idxHash, - zName, 0); - assert(pOld == pIndex || pOld == 0); - } freeIndex(db, pIndex); } - - /* Delete the Table structure itself. - */ - sqlite3HashClear(&pTable->idxHash); assert(pTable->def != NULL); /* Do not delete pTable->def allocated on region. */ if (!pTable->def->opts.is_temporary) @@ -463,7 +451,6 @@ sql_table_new(Parse *parser, char *name) table->iAutoIncPKey = -1; table->pSchema = db->pSchema; - sqlite3HashInit(&table->idxHash); table->nTabRef = 1; return table; } @@ -2840,7 +2827,7 @@ sql_create_index(struct Parse *parse, struct Token *token, if (name == NULL) goto exit_create_index; assert(token->z != NULL); - if (sqlite3HashFind(&table->idxHash, name) != NULL) { + if (sqlite3LocateIndex(db, name, table->def->name) != NULL) { if (!if_not_exist) { sqlite3ErrorMsg(parse, "index %s.%s already exists", @@ -3088,14 +3075,6 @@ sql_create_index(struct Parse *parse, struct Token *token, */ assert(parse->nErr == 0); if (db->init.busy) { - struct Index *p = sqlite3HashInsert(&table->idxHash, - index->def->name, index); - if (p != NULL) { - /* Malloc must have failed. */ - assert(p == index); - sqlite3OomFault(db); - goto exit_create_index; - } user_session->sql_flags |= SQLITE_InternChanges; index->def->iid = db->init.index_id; } @@ -3875,7 +3854,6 @@ sqlite3Reindex(Parse * pParse, Token * pName1, Token * pName2) char *z = 0; /* Name of index */ char *zTable = 0; /* Name of indexed table */ Table *pTab; /* A table in the database */ - Index *pIndex; /* An index associated with pTab */ sqlite3 *db = pParse->db; /* The database connection */ assert(db->pSchema != NULL); @@ -3918,12 +3896,7 @@ sqlite3Reindex(Parse * pParse, Token * pName1, Token * pName2) goto exit_reindex; } - pIndex = sqlite3HashFind(&pTab->idxHash, z); - if (pIndex != NULL) { - sql_set_multi_write(pParse, false); - sqlite3RefillIndex(pParse, pIndex); - return; - } + sqlite3ErrorMsg(pParse, "unable to identify the object to be reindexed"); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 9c861518f..5757efe49 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1874,7 +1874,6 @@ struct Savepoint { */ struct Table { Index *pIndex; /* List of SQL indexes on this table. */ - Hash idxHash; /* All (named) indices indexed by name */ u32 nTabRef; /* Number of pointers to this Table */ i16 iAutoIncPKey; /* If PK is marked INTEGER PRIMARY KEY AUTOINCREMENT, store column number here, -1 otherwise Tarantool specifics */ -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 03/10] sql: remove index hash from struct Table 2018-08-12 14:12 ` [tarantool-patches] [PATCH 03/10] sql: remove index hash from struct Table Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! I have pushed my review fixes in a separate commit. On 12/08/2018 17:12, Nikita Pettik wrote: > Part of #3561 > --- > src/box/sql/build.c | 41 +++++++---------------------------------- > src/box/sql/sqliteInt.h | 1 - > 2 files changed, 7 insertions(+), 35 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 04/10] sql: remove flags from struct Table 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (2 preceding siblings ...) 2018-08-12 14:12 ` [tarantool-patches] [PATCH 03/10] sql: remove index hash from struct Table Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 05/10] sql: remove affinity string of columns from Index Nikita Pettik ` (7 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Check on flag TF_HasPrimaryKey replaced with simple PK lookup in the list of table indexes. Check on flag TF_Ephemeral replaced with table->def->id == 0. Check on flag TF_Autoincrement replaced with table->iAutoinc >= 0. Part of #3561 --- src/box/sql/build.c | 11 ++++------- src/box/sql/insert.c | 12 ++++-------- src/box/sql/select.c | 4 +--- src/box/sql/sqliteInt.h | 8 -------- src/box/sql/where.c | 3 +-- 5 files changed, 10 insertions(+), 28 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 78206a1ed..31b91a4e2 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -883,13 +883,12 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ int nTerm; if (pTab == 0) goto primary_key_exit; - if (pTab->tabFlags & TF_HasPrimaryKey) { + if (sqlite3PrimaryKeyIndex(pTab) != NULL) { sqlite3ErrorMsg(pParse, "table \"%s\" has more than one primary key", pTab->def->name); goto primary_key_exit; } - pTab->tabFlags |= TF_HasPrimaryKey; if (pList == 0) { iCol = pTab->def->field_count - 1; nTerm = 1; @@ -918,10 +917,8 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER && sortOrder != SORT_ORDER_DESC) { assert(autoInc == 0 || autoInc == 1); - if (autoInc) { + if (autoInc) pTab->iAutoIncPKey = iCol; - pTab->tabFlags |= TF_Autoincrement; - } struct sqlite3 *db = pParse->db; struct ExprList *list; struct Token token; @@ -1698,7 +1695,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } if (!p->def->opts.is_view) { - if ((p->tabFlags & TF_HasPrimaryKey) == 0) { + if (sqlite3PrimaryKeyIndex(p) == NULL) { sqlite3ErrorMsg(pParse, "PRIMARY KEY missing on table %s", p->def->name); @@ -1767,7 +1764,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ * Check to see if we need to create an _sequence table * for keeping track of autoincrement keys. */ - if ((p->tabFlags & TF_Autoincrement) != 0) { + if (p->iAutoIncPKey >= 0) { assert(reg_space_id != 0); /* Do an insertion into _sequence. */ int reg_seq_id = ++pParse->nMem; diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 50c30fd82..ddd7f932b 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -714,8 +714,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ dflt, iRegStore); } else if (useTempTable) { - if ((pTab->tabFlags & TF_Autoincrement) - && (i == pTab->iAutoIncPKey)) { + if (i == pTab->iAutoIncPKey) { int regTmp = ++pParse->nMem; /* Emit code which doesn't override * autoinc-ed value with select result @@ -734,8 +733,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } } else if (pSelect) { if (regFromSelect != regData) { - if ((pTab->tabFlags & TF_Autoincrement) - && (i == pTab->iAutoIncPKey)) { + if (i == pTab->iAutoIncPKey) { /* Emit code which doesn't override * autoinc-ed value with select result * in case that result is NULL @@ -1022,9 +1020,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ /* Don't bother checking for NOT NULL on columns that do not change */ continue; } - if (def->fields[i].is_nullable || - (pTab->tabFlags & TF_Autoincrement && - pTab->iAutoIncPKey == (int) i)) + 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); @@ -1203,7 +1199,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ if (pTab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { int skip_if_null = sqlite3VdbeMakeLabel(v); - if ((pTab->tabFlags & TF_Autoincrement) != 0) { + if (pTab->iAutoIncPKey >= 0) { sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, skip_if_null); diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 6738ba54f..d22f4e0a9 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -4601,7 +4601,6 @@ withExpand(Walker * pWalker, struct SrcList_item *pFrom) pTab->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT; assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == DEFAULT_TUPLE_LOG_COUNT); - pTab->tabFlags |= TF_Ephemeral; pFrom->pSelect = sqlite3SelectDup(db, pCte->pSelect, 0); if (db->mallocFailed) return SQLITE_NOMEM_BKPT; @@ -4803,7 +4802,6 @@ selectExpander(Walker * pWalker, Select * p) pTab->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT; assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == DEFAULT_TUPLE_LOG_COUNT); - pTab->tabFlags |= TF_Ephemeral; } else { /* * An ordinary table or view name in the @@ -5157,7 +5155,7 @@ selectAddSubqueryTypeInfo(Walker * pWalker, Select * p) for (i = 0, pFrom = pTabList->a; i < pTabList->nSrc; i++, pFrom++) { Table *pTab = pFrom->pTab; assert(pTab != 0); - if ((pTab->tabFlags & TF_Ephemeral) != 0) { + if (pTab->def->id == 0) { /* A sub-query in the FROM clause of a SELECT */ Select *pSel = pFrom->pSelect; if (pSel) { diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 5757efe49..e13e6ad34 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1884,7 +1884,6 @@ struct Table { * can be fetched from space struct. */ LogEst tuple_log_count; - u8 tabFlags; /* Mask of TF_* values */ Schema *pSchema; /* Schema that contains this table */ Table *pNextZombie; /* Next on the Parse.pZombieTab list */ /** Space definition with Tarantool metadata. */ @@ -1901,13 +1900,6 @@ struct Table { LogEst sql_space_tuple_log_count(struct Table *tab); -/* - * Allowed values for Table.tabFlags. - */ -#define TF_Ephemeral 0x02 /* An ephemeral table */ -#define TF_HasPrimaryKey 0x04 /* Table has a primary key */ -#define TF_Autoincrement 0x08 /* Integer primary key is autoincrement */ - /* * Each foreign key constraint is an instance of the following structure. * diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 117357662..db8d0bf80 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -4673,8 +4673,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */ struct SrcList_item *pTabItem = &pTabList->a[pLevel->iFrom]; Table *pTab = pTabItem->pTab; pLoop = pLevel->pWLoop; - if ((pTab->tabFlags & TF_Ephemeral) != 0 || - pTab->def->opts.is_view) { + if (pTab->def->id == 0 || pTab->def->opts.is_view) { /* Do nothing */ } else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 && (wctrlFlags & WHERE_OR_SUBCLAUSE) == 0) { -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 04/10] sql: remove flags from struct Table 2018-08-12 14:13 ` [tarantool-patches] [PATCH 04/10] sql: remove flags " Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! I have pushed my review fixes in a separate commit. On 12/08/2018 17:13, Nikita Pettik wrote: > Check on flag TF_HasPrimaryKey replaced with simple PK lookup in the > list of table indexes. > Check on flag TF_Ephemeral replaced with table->def->id == 0. > Check on flag TF_Autoincrement replaced with table->iAutoinc >= 0. > > Part of #3561 > --- > src/box/sql/build.c | 11 ++++------- > src/box/sql/insert.c | 12 ++++-------- > src/box/sql/select.c | 4 +--- > src/box/sql/sqliteInt.h | 8 -------- > src/box/sql/where.c | 3 +-- > 5 files changed, 10 insertions(+), 28 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 05/10] sql: remove affinity string of columns from Index 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (3 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 04/10] sql: remove flags " Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 06/10] sql: completely remove support of partial indexes Nikita Pettik ` (6 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Part of #3561 --- src/box/sql/build.c | 1 - src/box/sql/delete.c | 3 ++- src/box/sql/insert.c | 42 +++++++++++------------------------------- src/box/sql/sqliteInt.h | 16 +++++++++++----- src/box/sql/update.c | 2 +- src/box/sql/vdbemem.c | 3 +-- src/box/sql/where.c | 21 ++++++++------------- src/box/sql/wherecode.c | 3 ++- 8 files changed, 36 insertions(+), 55 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 31b91a4e2..ead3e4f19 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -213,7 +213,6 @@ freeIndex(sqlite3 * db, Index * p) sql_expr_delete(db, p->pPartIdxWhere, false); if (p->def != NULL) index_def_delete(p->def); - sqlite3DbFree(db, p->zColAff); sqlite3DbFree(db, p); } diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index 0be68830a..cb16a7c0d 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -352,7 +352,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, key_len = 0; struct Index *pk = sqlite3PrimaryKeyIndex(table); const char *zAff = is_view ? NULL : - sqlite3IndexAffinityStr(parse->db, pk); + sql_index_affinity_str(parse->db, + pk->def); sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, reg_key, zAff, pk_len); /* Set flag to save memory allocating one diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index ddd7f932b..2b6100ad5 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -75,34 +75,6 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */ * is managed along with the rest of the Index structure. It will be * released when sqlite3DeleteIndex() is called. */ -const char * -sqlite3IndexAffinityStr(sqlite3 *db, Index *index) -{ - if (index->zColAff != NULL) - return index->zColAff; - /* - * The first time a column affinity string for a - * particular index is required, it is allocated and - * populated here. It is then stored as a member of the - * Index structure for subsequent use. The column affinity - * string will eventually be deleted by - * sqliteDeleteIndex() when the Index structure itself is - * cleaned up. - */ - int column_count = index->def->key_def->part_count; - index->zColAff = (char *) sqlite3DbMallocRaw(0, column_count + 1); - if (index->zColAff == NULL) { - sqlite3OomFault(db); - return NULL; - } - for (int n = 0; n < column_count; n++) { - uint16_t x = index->def->key_def->parts[n].fieldno; - index->zColAff[n] = index->pTable->def->fields[x].affinity; - } - index->zColAff[column_count] = 0; - return index->zColAff; -} - char * sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) { @@ -112,15 +84,23 @@ sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) return NULL; struct space *space = space_by_id(def->space_id); assert(space != NULL); - + /* + * Table may occasionally come from Lua, so lets + * gentle process this case by setting default + * affinity for it. + */ + if (space->def->fields == NULL) { + memset(aff, AFFINITY_BLOB, column_count); + goto exit; + } for (uint32_t i = 0; i < column_count; i++) { uint32_t x = def->key_def->parts[i].fieldno; aff[i] = space->def->fields[x].affinity; if (aff[i] == AFFINITY_UNDEFINED) - aff[i] = 'A'; + aff[i] = AFFINITY_BLOB; } +exit: aff[column_count] = '\0'; - return aff; } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index e13e6ad34..c9c4965f9 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2007,8 +2007,6 @@ enum sql_index_type { struct Index { /** The SQL table being indexed. */ Table *pTable; - /** String defining the affinity of each column. */ - char *zColAff; /** The next index associated with the same table. */ Index *pNext; /** WHERE clause for partial indices. */ @@ -4281,8 +4279,6 @@ int sqlite3VarintLen(u64 v); #define getVarint sqlite3GetVarint #define putVarint sqlite3PutVarint -const char *sqlite3IndexAffinityStr(sqlite3 *, Index *); - /** * Return a pointer to the column affinity string associated with * given index. A column affinity string has one character for @@ -4611,7 +4607,17 @@ void sqlite3Stat4ProbeFree(UnpackedRecord *); int sql_stat4_column(struct sqlite3 *db, const char *record, uint32_t col_num, sqlite3_value **res); -char sqlite3IndexColumnAffinity(sqlite3 *, Index *, int); + +/** + * Return the affinity for a single column of an index. + * + * @param idx Index to be investigated. + * @param partno Affinity of this part to be returned. + * + * @retval Affinity of @partno index part. + */ +enum affinity_type +sql_index_part_affinity(struct index_def *idx, uint32_t partno); /* * The interface to the LEMON-generated parser diff --git a/src/box/sql/update.c b/src/box/sql/update.c index e2324c354..decd216c6 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -347,7 +347,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ regKey = iPk; } else { const char *zAff = is_view ? 0 : - sqlite3IndexAffinityStr(pParse->db, pPk); + sql_index_affinity_str(pParse->db, pPk->def); sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, regKey, zAff, pk_part_count); sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index ec2d8cb6b..64902b133 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -1553,8 +1553,7 @@ sqlite3Stat4ProbeSetValue(Parse * pParse, /* Parse context */ Expr *pElem = (pExpr ? sqlite3VectorFieldSubexpr(pExpr, i) : 0); u8 aff = - sqlite3IndexColumnAffinity(pParse->db, pIdx, - iVal + i); + sql_index_part_affinity(pIdx->def, iVal + i); alloc.iVal = iVal + i; rc = stat4ValueFromExpr(pParse, pElem, aff, &alloc, &pVal); diff --git a/src/box/sql/where.c b/src/box/sql/where.c index db8d0bf80..794db0302 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -1158,18 +1158,14 @@ whereRangeAdjust(WhereTerm * pTerm, LogEst nNew) return nRet; } -/* - * Return the affinity for a single column of an index. - */ -char -sqlite3IndexColumnAffinity(sqlite3 * db, Index * pIdx, int iCol) +enum affinity_type +sql_index_part_affinity(struct index_def *idx, uint32_t partno) { - assert(iCol >= 0 && iCol < (int) pIdx->def->key_def->part_count); - if (!pIdx->zColAff) { - if (sqlite3IndexAffinityStr(db, pIdx) == 0) - return AFFINITY_BLOB; - } - return pIdx->zColAff[iCol]; + assert(partno < idx->key_def->part_count); + struct space *space = space_by_id(idx->space_id); + assert(space != NULL); + uint32_t fieldno = idx->key_def->parts[partno].fieldno; + return space->def->fields[fieldno].affinity; } /* @@ -1224,7 +1220,7 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ int nLower = -1; int nUpper = index->def->opts.stat->sample_count + 1; int rc = SQLITE_OK; - u8 aff = sqlite3IndexColumnAffinity(db, p, nEq); + u8 aff = sql_index_part_affinity(p->def, nEq); sqlite3_value *p1 = 0; /* Value extracted from pLower */ sqlite3_value *p2 = 0; /* Value extracted from pUpper */ @@ -1774,7 +1770,6 @@ whereLoopClearUnion(sqlite3 * db, WhereLoop * p) { if ((p->wsFlags & WHERE_AUTO_INDEX) != 0 && (p->wsFlags & WHERE_AUTO_INDEX) != 0 && p->pIndex != 0) { - sqlite3DbFree(db, p->pIndex->zColAff); sqlite3DbFree(db, p->pIndex); p->pIndex = 0; } diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index 1976583fa..6b3f2f78a 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -725,7 +725,8 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */ char *zAff; if (pIdx != NULL) { zAff = sqlite3DbStrDup(pParse->db, - sqlite3IndexAffinityStr(pParse->db, pIdx)); + sql_index_affinity_str(pParse->db, + pIdx->def)); } else { zAff = sql_index_affinity_str(pParse->db, idx_def); } -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 05/10] sql: remove affinity string of columns from Index 2018-08-12 14:13 ` [tarantool-patches] [PATCH 05/10] sql: remove affinity string of columns from Index Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! I have pushed my review fixes in a separate commit. Also see 3 comments below. On 12/08/2018 17:13, Nikita Pettik wrote: > Part of #3561 > --- > src/box/sql/build.c | 1 - > src/box/sql/delete.c | 3 ++- > src/box/sql/insert.c | 42 +++++++++++------------------------------- > src/box/sql/sqliteInt.h | 16 +++++++++++----- > src/box/sql/update.c | 2 +- > src/box/sql/vdbemem.c | 3 +-- > src/box/sql/where.c | 21 ++++++++------------- > src/box/sql/wherecode.c | 3 ++- > 8 files changed, 36 insertions(+), 55 deletions(-) > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 0be68830a..cb16a7c0d 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -352,7 +352,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > key_len = 0; > struct Index *pk = sqlite3PrimaryKeyIndex(table); > const char *zAff = is_view ? NULL : > - sqlite3IndexAffinityStr(parse->db, pk); > + sql_index_affinity_str(parse->db, > + pk->def); 1. sql_index_affinity_str makes extra lookup in space cache. Here (in sql_table_delete_from) space ptr is known already. Same in fkey_lookup_parent. Maybe it would be better to pass space ptr alongside with index_def? And name it sql_space_index_affinity_str. > sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, > reg_key, zAff, pk_len); > /* Set flag to save memory allocating one > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index ddd7f932b..2b6100ad5 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -112,15 +84,23 @@ sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) > return NULL; > struct space *space = space_by_id(def->space_id); > assert(space != NULL); > - > + /* > + * Table may occasionally come from Lua, so lets > + * gentle process this case by setting default > + * affinity for it. > + */ > + if (space->def->fields == NULL) { > + memset(aff, AFFINITY_BLOB, column_count); > + goto exit; > + } > for (uint32_t i = 0; i < column_count; i++) { > uint32_t x = def->key_def->parts[i].fieldno; > aff[i] = space->def->fields[x].affinity; > if (aff[i] == AFFINITY_UNDEFINED) > - aff[i] = 'A'; > + aff[i] = AFFINITY_BLOB; > } > +exit: 2. Please, try to avoid extra labels when possible. They might help only when there are many error processing code snippets or when it allows to strongly reduce indentation. Otherwise it slightly confuses. I have fixed it on the branch. > aff[column_count] = '\0'; > - > return aff; > } > > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index db8d0bf80..794db0302 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -1224,7 +1220,7 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ > int nLower = -1; > int nUpper = index->def->opts.stat->sample_count + 1; > int rc = SQLITE_OK; > - u8 aff = sqlite3IndexColumnAffinity(db, p, nEq); > + u8 aff = sql_index_part_affinity(p->def, nEq); 3. Same as 1 - extra lookup in the space cache. You have the space few lines above. > > sqlite3_value *p1 = 0; /* Value extracted from pLower */ > sqlite3_value *p2 = 0; /* Value extracted from pUpper */ ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 05/10] sql: remove affinity string of columns from Index 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-21 16:31 ` n.pettik 2018-08-24 21:04 ` Vladislav Shpilevoy 0 siblings, 1 reply; 38+ messages in thread From: n.pettik @ 2018-08-21 16:31 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy >> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c >> index 0be68830a..cb16a7c0d 100644 >> --- a/src/box/sql/delete.c >> +++ b/src/box/sql/delete.c >> @@ -352,7 +352,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, >> key_len = 0; >> struct Index *pk = sqlite3PrimaryKeyIndex(table); >> const char *zAff = is_view ? NULL : >> - sqlite3IndexAffinityStr(parse->db, pk); >> + sql_index_affinity_str(parse->db, >> + pk->def); > > 1. sql_index_affinity_str makes extra lookup in space cache. Here > (in sql_table_delete_from) space ptr is known already. > > Same in fkey_lookup_parent. Maybe it would be better to pass space ptr > alongside with index_def? And name it sql_space_index_affinity_str. Ok, time to pull up struct space: +++ b/src/box/sql/delete.c @@ -352,8 +352,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, key_len = 0; struct Index *pk = sqlite3PrimaryKeyIndex(table); const char *zAff = is_view ? NULL : - sql_index_affinity_str(parse->db, - pk->def); + sql_space_index_affinity_str(parse->db, + space->def, + pk->def); sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, +++ b/src/box/sql/fkey.c @@ -256,7 +256,8 @@ fkey_lookup_parent(struct Parse *parse_context, struct space *parent, struct index *idx = space_index(parent, referenced_idx); assert(idx != NULL); sqlite3VdbeAddOp4(v, OP_MakeRecord, temp_regs, field_count, rec_reg, - sql_index_affinity_str(parse_context->db, idx->def), + sql_space_index_affinity_str(parse_context->db, + parent->def, idx->def), +++ b/src/box/sql/insert.c @@ -59,25 +59,24 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */ } char * -sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) +sql_space_index_affinity_str(struct sqlite3 *db, struct space_def *space_def, + struct index_def *idx_def) { - uint32_t column_count = def->key_def->part_count; + uint32_t column_count = idx_def->key_def->part_count; char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1); if (aff == NULL) return NULL; - struct space *space = space_by_id(def->space_id); - assert(space != NULL); /* * Table may occasionally come from Lua, so lets * gentle process this case by setting default * affinity for it. */ - if (space->def->fields == NULL) { + if (space_def->fields == NULL) { memset(aff, AFFINITY_BLOB, column_count); } else { for (uint32_t i = 0; i < column_count; i++) { - uint32_t x = def->key_def->parts[i].fieldno; - aff[i] = space->def->fields[x].affinity; + uint32_t x = idx_def->key_def->parts[i].fieldno; + aff[i] = space_def->fields[x].affinity; +++ b/src/box/sql/sqliteInt.h @@ -4297,11 +4297,14 @@ int sqlite3VarintLen(u64 v); * is allocated on heap. * * @param db Database handle. - * @param def index_def where from affinity to be extracted. + * @param space_def Definition of space index belongs to. + * @param idx_def Definition of index from which affinity + * to be extracted. * @retval Allocated affinity string, or NULL on OOM. */ char * -sql_index_affinity_str(struct sqlite3 *db, struct index_def *def); +sql_space_index_affinity_str(struct sqlite3 *db, struct space_def *space_def, + struct index_def *idx_def); +++ b/src/box/sql/update.c @@ -347,7 +347,8 @@ sqlite3Update(Parse * pParse, /* The parser context */ regKey = iPk; } else { const char *zAff = is_view ? 0 : - sql_index_affinity_str(pParse->db, pPk->def); + sql_space_index_affinity_str(pParse->db, def, + pPk->def); +++ b/src/box/sql/wherecode.c @@ -724,11 +724,17 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */ char *zAff; if (pIdx != NULL) { + struct space *space = space_by_id(pIdx->def->space_id); + assert(space != NULL); zAff = sqlite3DbStrDup(pParse->db, - sql_index_affinity_str(pParse->db, - pIdx->def)); + sql_space_index_affinity_str(pParse->db, + space->def, + pIdx->def)); } else { - zAff = sql_index_affinity_str(pParse->db, idx_def); + struct space *space = space_by_id(idx_def->space_id); + assert(space != NULL); + zAff = sql_space_index_affinity_str(pParse->db, space->def, + idx_def); >> sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, >> reg_key, zAff, pk_len); >> /* Set flag to save memory allocating one >> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c >> index ddd7f932b..2b6100ad5 100644 >> --- a/src/box/sql/insert.c >> +++ b/src/box/sql/insert.c >> @@ -112,15 +84,23 @@ sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) >> return NULL; >> struct space *space = space_by_id(def->space_id); >> assert(space != NULL); >> - >> + /* >> + * Table may occasionally come from Lua, so lets >> + * gentle process this case by setting default >> + * affinity for it. >> + */ >> + if (space->def->fields == NULL) { >> + memset(aff, AFFINITY_BLOB, column_count); >> + goto exit; >> + } >> for (uint32_t i = 0; i < column_count; i++) { >> uint32_t x = def->key_def->parts[i].fieldno; >> aff[i] = space->def->fields[x].affinity; >> if (aff[i] == AFFINITY_UNDEFINED) >> - aff[i] = 'A'; >> + aff[i] = AFFINITY_BLOB; >> } >> +exit: > > 2. Please, try to avoid extra labels when possible. They might > help only when there are many error processing code snippets or > when it allows to strongly reduce indentation. Otherwise it > slightly confuses. I have fixed it on the branch. Ok, got it. Thx for fix. > >> aff[column_count] = '\0'; >> - >> return aff; >> } >> diff --git a/src/box/sql/where.c b/src/box/sql/where.c >> index db8d0bf80..794db0302 100644 >> --- a/src/box/sql/where.c >> +++ b/src/box/sql/where.c >> @@ -1224,7 +1220,7 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ >> int nLower = -1; >> int nUpper = index->def->opts.stat->sample_count + 1; >> int rc = SQLITE_OK; >> - u8 aff = sqlite3IndexColumnAffinity(db, p, nEq); >> + u8 aff = sql_index_part_affinity(p->def, nEq); > > 3. Same as 1 - extra lookup in the space cache. You have the space > few lines above. Fixed: @@ -4611,13 +4614,15 @@ sql_stat4_column(struct sqlite3 *db, const char *record, uint32_t col_num, /** * Return the affinity for a single column of an index. * + * @param def Definition of space @idx belongs to. * @param idx Index to be investigated. * @param partno Affinity of this part to be returned. * * @retval Affinity of @partno index part. */ enum affinity_type -sql_index_part_affinity(struct index_def *idx, uint32_t partno); +sql_space_index_part_affinity(struct space_def *def, struct index_def *idx, + uint32_t partno); +++ b/src/box/sql/where.c @@ -1159,13 +1159,12 @@ whereRangeAdjust(WhereTerm * pTerm, LogEst nNew) } +++ b/src/box/sql/vdbemem.c @@ -39,6 +39,7 @@ #include "sqliteInt.h" #include "vdbeInt.h" #include "tarantoolInt.h" +#include "box/schema.h" #ifdef SQLITE_DEBUG /* @@ -1548,11 +1549,14 @@ sqlite3Stat4ProbeSetValue(Parse * pParse, /* Parse context */ alloc.pIdx = pIdx; alloc.ppRec = ppRec; + struct space *space = space_by_id(pIdx->def->space_id); + assert(space != NULL); for (i = 0; i < nElem; i++) { sqlite3_value *pVal = 0; Expr *pElem = (pExpr ? sqlite3VectorFieldSubexpr(pExpr, i) : 0); - u8 aff = sql_index_part_affinity(pIdx->def, iVal + i); + u8 aff = sql_space_index_part_affinity(space->def, pIdx->def, + iVal + i); enum affinity_type -sql_index_part_affinity(struct index_def *idx, uint32_t partno) +sql_space_index_part_affinity(struct space_def *def, struct index_def *idx, + uint32_t partno) { assert(partno < idx->key_def->part_count); - struct space *space = space_by_id(idx->space_id); - assert(space != NULL); uint32_t fieldno = idx->key_def->parts[partno].fieldno; - return space->def->fields[fieldno].affinity; + return def->fields[fieldno].affinity; } @@ -1220,7 +1219,7 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ int nLower = -1; int nUpper = index->def->opts.stat->sample_count + 1; int rc = SQLITE_OK; - u8 aff = sql_index_part_affinity(p->def, nEq); + u8 aff = sql_space_index_part_affinity(space->def, p->def, nEq); ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 05/10] sql: remove affinity string of columns from Index 2018-08-21 16:31 ` n.pettik @ 2018-08-24 21:04 ` Vladislav Shpilevoy 2018-08-26 19:45 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-24 21:04 UTC (permalink / raw) To: n.pettik, tarantool-patches Hi! Thanks for the fixes! Please, next time attach new diff at the end of a message. Now I did it below. See 1 comment and a commit on the branch. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index a8423fe95..f27484344 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > char * > -sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) > +sql_space_index_affinity_str(struct sqlite3 *db, struct space_def *space_def, > + struct index_def *idx_def) > { > - uint32_t column_count = def->key_def->part_count; > + uint32_t column_count = idx_def->key_def->part_count; > char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1); > if (aff == NULL) > return NULL; > - struct space *space = space_by_id(def->space_id); > - assert(space != NULL); > - > - for (uint32_t i = 0; i < column_count; i++) { > - uint32_t x = def->key_def->parts[i].fieldno; > - aff[i] = space->def->fields[x].affinity; > - if (aff[i] == AFFINITY_UNDEFINED) > - aff[i] = 'A'; > + /* > + * Table may occasionally come from Lua, so lets Alternatively it could be IProto instead of SQL. Lets just write 'non-SQL'. > + * gentle process this case by setting default > + * affinity for it. > + */ > + if (space_def->fields == NULL) { > + memset(aff, AFFINITY_BLOB, column_count); > + } else { > + for (uint32_t i = 0; i < column_count; i++) { > + uint32_t x = idx_def->key_def->parts[i].fieldno; > + aff[i] = space_def->fields[x].affinity; > + if (aff[i] == AFFINITY_UNDEFINED) > + aff[i] = AFFINITY_BLOB; > + } > } > aff[column_count] = '\0'; > - > return aff; > } > My diff is below: diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index f27484344..ad9302464 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -67,16 +67,16 @@ sql_space_index_affinity_str(struct sqlite3 *db, struct space_def *space_def, if (aff == NULL) return NULL; /* - * Table may occasionally come from Lua, so lets - * gentle process this case by setting default - * affinity for it. + * Table may occasionally come from non-SQL API, so lets + * gentle process this case by setting default affinity + * for it. */ if (space_def->fields == NULL) { memset(aff, AFFINITY_BLOB, column_count); } else { for (uint32_t i = 0; i < column_count; i++) { - uint32_t x = idx_def->key_def->parts[i].fieldno; - aff[i] = space_def->fields[x].affinity; + aff[i] = sql_space_index_part_affinity(space_def, + idx_def, i); if (aff[i] == AFFINITY_UNDEFINED) aff[i] = AFFINITY_BLOB; } ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 05/10] sql: remove affinity string of columns from Index 2018-08-24 21:04 ` Vladislav Shpilevoy @ 2018-08-26 19:45 ` n.pettik 0 siblings, 0 replies; 38+ messages in thread From: n.pettik @ 2018-08-26 19:45 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > Hi! Thanks for the fixes! > Please, next time attach new diff at the > end of a message. Now I did it below. Sorry, I thought fixes are small enough, so I can avoid attaching almost the same diff… Anyway, thx for fix - I have applied it. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 06/10] sql: completely remove support of partial indexes 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (4 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 05/10] sql: remove affinity string of columns from Index Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 07/10] sql: remove index type from struct Index Nikita Pettik ` (5 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik We banned opportunity to create partial indexes long ago, but internal routine implementing this feature still remains. This patch completely removes it. Part of #3561 --- src/box/sql/build.c | 23 ++++++----------------- src/box/sql/delete.c | 17 +++-------------- src/box/sql/insert.c | 12 ------------ src/box/sql/parse.y | 6 +++--- src/box/sql/pragma.c | 5 +---- src/box/sql/sqliteInt.h | 7 ++----- src/box/sql/update.c | 2 +- src/box/sql/where.c | 31 ------------------------------- 8 files changed, 16 insertions(+), 87 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index ead3e4f19..01d4d52a3 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -210,7 +210,6 @@ sqlite3LocateIndex(sqlite3 * db, const char *zName, const char *zTable) static void freeIndex(sqlite3 * db, Index * p) { - sql_expr_delete(db, p->pPartIdxWhere, false); if (p->def != NULL) index_def_delete(p->def); sqlite3DbFree(db, p); @@ -927,9 +926,8 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ &token, 0)); if (list == NULL) goto primary_key_exit; - sql_create_index(pParse, 0, 0, list, onError, 0, 0, - SORT_ORDER_ASC, false, - SQL_INDEX_TYPE_CONSTRAINT_PK); + sql_create_index(pParse, 0, 0, list, onError, 0, SORT_ORDER_ASC, + false, SQL_INDEX_TYPE_CONSTRAINT_PK); if (db->mallocFailed) goto primary_key_exit; } else if (autoInc) { @@ -937,9 +935,8 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ "INTEGER PRIMARY KEY or INT PRIMARY KEY"); goto primary_key_exit; } else { - sql_create_index(pParse, 0, 0, pList, onError, 0, - 0, sortOrder, false, - SQL_INDEX_TYPE_CONSTRAINT_PK); + sql_create_index(pParse, 0, 0, pList, onError, 0, sortOrder, + false, SQL_INDEX_TYPE_CONSTRAINT_PK); pList = 0; if (pParse->nErr > 0) goto primary_key_exit; @@ -2756,8 +2753,8 @@ void sql_create_index(struct Parse *parse, struct Token *token, struct SrcList *tbl_name, struct ExprList *col_list, enum on_conflict_action on_error, struct Token *start, - struct Expr *where, enum sort_order sort_order, - bool if_not_exist, enum sql_index_type idx_type) { + enum sort_order sort_order, bool if_not_exist, + enum sql_index_type idx_type) { /* The index to be created. */ struct Index *index = NULL; /* Name of the index. */ @@ -2917,13 +2914,6 @@ sql_create_index(struct Parse *parse, struct Token *token, index->pTable = table; index->onError = (u8) on_error; index->index_type = idx_type; - /* Tarantool have access to each column by any index. */ - if (where != NULL) { - sql_resolve_self_reference(parse, table, NC_PartIdx, where, - NULL); - index->pPartIdxWhere = where; - where = NULL; - } /* * TODO: Issue a warning if two or more columns of the @@ -3132,7 +3122,6 @@ sql_create_index(struct Parse *parse, struct Token *token, exit_create_index: if (index != NULL) freeIndex(db, index); - sql_expr_delete(db, where, false); sql_expr_list_delete(db, col_list); sqlite3SrcListDelete(db, tbl_name); sqlite3DbFree(db, name); diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index cb16a7c0d..99b49d63c 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -587,22 +587,11 @@ sql_generate_index_key(struct Parse *parse, struct Index *index, int cursor, { struct Vdbe *v = parse->pVdbe; - if (part_idx_label != NULL) { - if (index->pPartIdxWhere != NULL) { - *part_idx_label = sqlite3VdbeMakeLabel(v); - parse->iSelfTab = cursor; - sqlite3ExprCachePush(parse); - sqlite3ExprIfFalseDup(parse, index->pPartIdxWhere, - *part_idx_label, - SQLITE_JUMPIFNULL); - } else { - *part_idx_label = 0; - } - } + if (part_idx_label != NULL) + *part_idx_label = 0; int col_cnt = index->def->key_def->part_count; int reg_base = sqlite3GetTempRange(parse, col_cnt); - if (prev != NULL && (reg_base != reg_prev || - prev->pPartIdxWhere != NULL)) + if (prev != NULL && reg_base != reg_prev) prev = NULL; for (int j = 0; j < col_cnt; j++) { if (prev != NULL && prev->def->key_def->parts[j].fieldno == diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 2b6100ad5..853265ead 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -1131,15 +1131,6 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ iThisCur = iIdxCur + ix; addrUniqueOk = sqlite3VdbeMakeLabel(v); - /* Skip partial indices for which the WHERE clause is not true */ - if (pIdx->pPartIdxWhere) { - sqlite3VdbeAddOp2(v, OP_Null, 0, aRegIdx[ix]); - pParse->ckBase = regNewData + 1; - sqlite3ExprIfFalseDup(pParse, pIdx->pPartIdxWhere, - addrUniqueOk, SQLITE_JUMPIFNULL); - pParse->ckBase = 0; - } - /* Create a record for this index entry as it should appear after * the insert or update. Store that record in the aRegIdx[ix] register */ @@ -1568,9 +1559,6 @@ xferCompatibleIndex(Index * pDest, Index * pSrc) if (src_part->coll != dest_part->coll) return 0; /* Different collating sequences */ } - if (sqlite3ExprCompare(pSrc->pPartIdxWhere, pDest->pPartIdxWhere, -1)) { - return 0; /* Different WHERE clauses */ - } /* If no test above fails then the indices must be compatible */ return 1; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 58a6bf39a..66ad84ce2 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -282,7 +282,7 @@ ccons ::= NULL onconf(R). { ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);} ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I). {sqlite3AddPrimaryKey(pParse,0,R,I,Z);} -ccons ::= UNIQUE index_onconf(R). {sql_create_index(pParse,0,0,0,R,0,0, +ccons ::= UNIQUE index_onconf(R). {sql_create_index(pParse,0,0,0,R,0, SORT_ORDER_ASC, false, SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} ccons ::= CHECK LP expr(X) RP. {sql_add_check_constraint(pParse,&X);} @@ -337,7 +337,7 @@ tcons ::= CONSTRAINT nm(X). {pParse->constraintName = X;} tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP onconf(R). {sqlite3AddPrimaryKey(pParse,X,R,I,0);} tcons ::= UNIQUE LP sortlist(X) RP index_onconf(R). - {sql_create_index(pParse,0,0,X,R,0,0, + {sql_create_index(pParse,0,0,X,R,0, SORT_ORDER_ASC,false, SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} tcons ::= CHECK LP expr(E) RP onconf. @@ -1203,7 +1203,7 @@ cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X) enum on_conflict_action on_error = U ? ON_CONFLICT_ACTION_ABORT : ON_CONFLICT_ACTION_NONE; sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, - on_error, &S, NULL, SORT_ORDER_ASC, NE, U); + on_error, &S, SORT_ORDER_ASC, NE, U); } %type uniqueflag {int} diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index f7e6053eb..0ba651567 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -514,10 +514,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ pIdx->def->opts.is_unique, azOrigin [pIdx-> - index_type], - pIdx-> - pPartIdxWhere - != 0); + index_type]); sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 5); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index c9c4965f9..eb20fb31a 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2009,8 +2009,6 @@ struct Index { Table *pTable; /** The next index associated with the same table. */ Index *pNext; - /** WHERE clause for partial indices. */ - Expr *pPartIdxWhere; /** * Conflict resolution algorithm to employ whenever an * attempt is made to insert a non-unique element in @@ -3591,7 +3589,6 @@ void sqlite3SrcListDelete(sqlite3 *, SrcList *); * @param on_error One of ON_CONFLICT_ACTION_ABORT, _IGNORE, * _REPLACE, or _NONE. * @param start The CREATE token that begins this statement. - * @param where WHERE clause for partial indices. * @param sort_order Sort order of primary key when pList==NULL. * @param if_not_exist Omit error if index already exists. * @param idx_type The index type. @@ -3600,8 +3597,8 @@ void sql_create_index(struct Parse *parse, struct Token *token, struct SrcList *tbl_name, struct ExprList *col_list, enum on_conflict_action on_error, struct Token *start, - struct Expr *where, enum sort_order sort_order, - bool if_not_exist, enum sql_index_type idx_type); + enum sort_order sort_order, bool if_not_exist, + enum sql_index_type idx_type); /** * This routine will drop an existing named index. This routine diff --git a/src/box/sql/update.c b/src/box/sql/update.c index decd216c6..54b30705a 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -239,7 +239,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ for (j = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, j++) { int reg; uint32_t part_count = pIdx->def->key_def->part_count; - if (chngPk || hasFK || pIdx->pPartIdxWhere || pIdx == pPk) { + if (chngPk || hasFK || pIdx == pPk) { reg = ++pParse->nMem; pParse->nMem += part_count; } else { diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 794db0302..a57bad5b7 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -2781,31 +2781,6 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder, return 0; } -/* Check to see if a partial index with pPartIndexWhere can be used - * in the current query. Return true if it can be and false if not. - */ -static int -whereUsablePartialIndex(int iTab, WhereClause * pWC, Expr * pWhere) -{ - int i; - WhereTerm *pTerm; - while (pWhere->op == TK_AND) { - if (!whereUsablePartialIndex(iTab, pWC, pWhere->pLeft)) - return 0; - pWhere = pWhere->pRight; - } - for (i = 0, pTerm = pWC->a; i < pWC->nTerm; i++, pTerm++) { - Expr *pExpr = pTerm->pExpr; - if (sqlite3ExprImpliesExpr(pExpr, pWhere, iTab) - && (!ExprHasProperty(pExpr, EP_FromJoin) - || pExpr->iRightJoinTable == iTab) - ) { - return 1; - } - } - return 0; -} - /* * Add all WhereLoop objects for a single table of the join where the table * is identified by pBuilder->pNew->iTab. @@ -2992,12 +2967,6 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */ /* Loop over all indices */ for (; rc == SQLITE_OK && pProbe; pProbe = pProbe->pNext, iSortIdx++) { - if (pProbe->pPartIdxWhere != 0 - && !whereUsablePartialIndex(pSrc->iCursor, pWC, - pProbe->pPartIdxWhere)) { - testcase(pNew->iTab != pSrc->iCursor); /* See ticket [98d973b8f5] */ - continue; /* Partial index inappropriate for this query */ - } rSize = index_field_tuple_est(pProbe, 0); pNew->nEq = 0; pNew->nBtm = 0; -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 06/10] sql: completely remove support of partial indexes 2018-08-12 14:13 ` [tarantool-patches] [PATCH 06/10] sql: completely remove support of partial indexes Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! I have pushed my review fixes in a separate commit. Please, investigate also is it possible to remove pPartial variable from constructAutomaticIndex? It would allow to remove sql_resolve_part_idx_label alongside. On 12/08/2018 17:13, Nikita Pettik wrote: > We banned opportunity to create partial indexes long ago, but internal > routine implementing this feature still remains. This patch completely > removes it. > > Part of #3561 > --- > src/box/sql/build.c | 23 ++++++----------------- > src/box/sql/delete.c | 17 +++-------------- > src/box/sql/insert.c | 12 ------------ > src/box/sql/parse.y | 6 +++--- > src/box/sql/pragma.c | 5 +---- > src/box/sql/sqliteInt.h | 7 ++----- > src/box/sql/update.c | 2 +- > src/box/sql/where.c | 31 ------------------------------- > 8 files changed, 16 insertions(+), 87 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 06/10] sql: completely remove support of partial indexes 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-21 16:31 ` n.pettik 2018-08-24 21:04 ` Vladislav Shpilevoy 0 siblings, 1 reply; 38+ messages in thread From: n.pettik @ 2018-08-21 16:31 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Thanks for the patch! I have pushed my review fixes in > a separate commit. > > Please, investigate also is it possible to remove pPartial > variable from constructAutomaticIndex? It would allow to > remove sql_resolve_part_idx_label alongside. ConstructAutomatiIndex() is now dead code, so I can’t really check it. Lets now remove it. I doubt that this code can be adapted easily, so it is likely to be re-implemented anyway. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 06/10] sql: completely remove support of partial indexes 2018-08-21 16:31 ` n.pettik @ 2018-08-24 21:04 ` Vladislav Shpilevoy 2018-08-26 19:44 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-24 21:04 UTC (permalink / raw) To: n.pettik, tarantool-patches Thanks for the fixes! See my ones on the branch. On 21/08/2018 19:31, n.pettik wrote: > >> On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: >> >> Thanks for the patch! I have pushed my review fixes in >> a separate commit. >> >> Please, investigate also is it possible to remove pPartial >> variable from constructAutomaticIndex? It would allow to >> remove sql_resolve_part_idx_label alongside. > > ConstructAutomatiIndex() is now dead code, so I can’t really check it. > Lets now remove it. I doubt that this code can be adapted easily, so > it is likely to be re-implemented anyway. > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 00d96d220..901951963 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -51,9 +51,9 @@ * which the index belongs. In each such row, the stat column will be * a string consisting of a list of integers. The first integer in this * list is the number of rows in the index. (This is the same as the - * number of rows in the table, except for partial indices.) The second - * integer is the average number of rows in the index that have the same - * value in the first column of the index. The third integer is the average + * number of rows in the table.) The second integer is the + * average number of rows in the index that have the same value in + * the first column of the index. The third integer is the average * number of rows in the index that have the same value for the first two * columns. The N-th integer (for N>1) is the average number of rows in * the index which have the same value for the first N-1 columns. For diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index a41ea8f13..50505b1d3 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,7 +3700,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target) int iTab = pExpr->iTable; if (iTab < 0) { if (pParse->ckBase > 0) { - /* Generating CHECK constraints or inserting into partial index */ + /* Generating CHECK constraints. */ return pExpr->iColumn + pParse->ckBase; } else { /* Coding an expression that is part of an index where column names diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index 97b642723..9a2d6ff4e 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -514,11 +514,13 @@ notValid(Parse * pParse, /* Leave error message here */ { assert((validMask & ~(NC_IsCheck | NC_IdxExpr)) == 0); if ((pNC->ncFlags & validMask) != 0) { - const char *zIn = "partial index WHERE clauses"; + const char *zIn; if (pNC->ncFlags & NC_IdxExpr) zIn = "index expressions"; else if (pNC->ncFlags & NC_IsCheck) zIn = "CHECK constraints"; + else + unreachable(); sqlite3ErrorMsg(pParse, "%s prohibited in %s", zMsg, zIn); } } diff --git a/src/box/sql/select.c b/src/box/sql/select.c index d22f4e0a9..849c0f871 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -6247,7 +6247,7 @@ sqlite3Select(Parse * pParse, /* The parser context */ * optimized specially. The OP_Count instruction * is executed on the primary key index, * since there is no difference which index - * to choose (except for partial indexes). + * to choose. */ const int cursor = pParse->nTab++; /* diff --git a/src/box/sql/whereInt.h b/src/box/sql/whereInt.h index 548cbcb2a..889a667ae 100644 --- a/src/box/sql/whereInt.h +++ b/src/box/sql/whereInt.h @@ -538,4 +538,3 @@ void sqlite3WhereTabFuncArgs(Parse *, struct SrcList_item *, WhereClause *); #define WHERE_AUTO_INDEX 0x00004000 /* Uses an ephemeral index */ #define WHERE_SKIPSCAN 0x00008000 /* Uses the skip-scan algorithm */ #define WHERE_UNQ_WANTED 0x00010000 /* WHERE_ONEROW would have been helpful */ -#define WHERE_PARTIALIDX 0x00020000 /* The automatic index is partial */ diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index 10a3f2d95..b45ca7cbe 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -231,8 +231,6 @@ sqlite3WhereExplainOneScan(Parse * pParse, /* Parse context */ if (isSearch) { zFmt = "PRIMARY KEY"; } - } else if (flags & WHERE_PARTIALIDX) { - zFmt = "AUTOMATIC PARTIAL COVERING INDEX"; } else if (flags & WHERE_AUTO_INDEX) { zFmt = "AUTOMATIC COVERING INDEX"; } else if (flags & WHERE_IDX_ONLY) { ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 06/10] sql: completely remove support of partial indexes 2018-08-24 21:04 ` Vladislav Shpilevoy @ 2018-08-26 19:44 ` n.pettik 0 siblings, 0 replies; 38+ messages in thread From: n.pettik @ 2018-08-26 19:44 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > Thanks for the fixes! See my ones on the branch. Thx, I squashed them. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 07/10] sql: remove index type from struct Index 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (5 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 06/10] sql: completely remove support of partial indexes Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 08/10] sql: use secondary indexes to process OP_Delete Nikita Pettik ` (4 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Part of #3561 --- src/box/sql.c | 2 +- src/box/sql/analyze.c | 2 +- src/box/sql/build.c | 92 +++++++++++++++++++------------------------------ src/box/sql/insert.c | 14 ++++---- src/box/sql/pragma.c | 7 +--- src/box/sql/prepare.c | 9 +---- src/box/sql/sqliteInt.h | 18 ++++------ src/box/sql/update.c | 2 +- src/box/sql/where.c | 4 +-- src/box/sql/wherecode.c | 4 +-- 10 files changed, 58 insertions(+), 96 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index ae12cae36..a0aced27b 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1511,7 +1511,7 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by * Tarantool. */ - p = enc->encode_bool(p, IsUniqueIndex(index)); + p = enc->encode_bool(p, sql_index_is_unique(index)); p = enc->encode_str(p, "sql", 3); p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); return (int)(p - base); diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 00d96d220..74f5ae827 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -831,7 +831,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */ * names. Thus, for the sake of clarity, use * instead more familiar table name. */ - if (IsPrimaryKeyIndex(pIdx)) + if (sql_index_is_primary(pIdx)) idx_name = pTab->def->name; else idx_name = pIdx->def->name; diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 01d4d52a3..3ef9ea96e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -145,7 +145,7 @@ actualize_on_conflict_actions(struct Parse *parser, struct Table *table) for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) { if (idx->onError == ON_CONFLICT_ACTION_REPLACE && - !IsPrimaryKeyIndex(idx)) + !sql_index_is_primary(idx)) goto non_pk_on_conflict_error; } @@ -421,8 +421,8 @@ Index * sqlite3PrimaryKeyIndex(Table * pTab) { Index *p; - for (p = pTab->pIndex; p && !IsPrimaryKeyIndex(p); p = p->pNext) { - } + for (p = pTab->pIndex; p != NULL && !sql_index_is_primary(p); + p = p->pNext); return p; } @@ -1280,8 +1280,11 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId, SQL_SUBTYPE_MSGPACK,zParts, P4_STATIC); sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 6, iRecord); sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, iRecord); - if (pIndex->index_type == SQL_INDEX_TYPE_NON_UNIQUE || - pIndex->index_type == SQL_INDEX_TYPE_UNIQUE) + /* + * Non-NULL value means that index has been created via + * separate CREATE INDEX statement. + */ + if (zSql != NULL) sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); } @@ -1383,38 +1386,6 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt) sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); } -/* - * Generate code to create implicit indexes in the new table. - * iSpaceId is a register storing the id of the space. - * iCursor is a cursor to access _index. - */ -static void -createImplicitIndices(Parse * pParse, int iSpaceId) -{ - Table *p = pParse->pNewTable; - Index *pIdx, *pPrimaryIdx = sqlite3PrimaryKeyIndex(p); - int i; - - if (pPrimaryIdx) { - /* Tarantool quirk: primary index is created first */ - createIndex(pParse, pPrimaryIdx, iSpaceId, 0, NULL); - } else { - /* - * This branch should not be taken. - * If it is, then the current CREATE TABLE statement fails to - * specify the PRIMARY KEY. The error is reported elsewhere. - */ - unreachable(); - } - - /* (pIdx->i) mapping must be consistent with parseTableSchemaRecord */ - for (pIdx = p->pIndex, i = 0; pIdx; pIdx = pIdx->pNext) { - if (pIdx == pPrimaryIdx) - continue; - createIndex(pParse, pIdx, iSpaceId, ++i, NULL); - } -} - /* * Generate code to emit and parse table schema record. * iSpaceId is a register storing the id of the space. @@ -1436,7 +1407,6 @@ parseTableSchemaRecord(Parse * pParse, int iSpaceId, char *zStmt) sqlite3VdbeAddOp4(v, OP_String8, 0, iTop + 3, 0, zStmt, P4_DYNAMIC); pPrimaryIdx = sqlite3PrimaryKeyIndex(p); - /* (pIdx->i) mapping must be consistent with createImplicitIndices */ for (pIdx = p->pIndex, i = 0; pIdx; pIdx = pIdx->pNext) { if (pIdx == pPrimaryIdx) continue; @@ -1753,8 +1723,12 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ int reg_space_id = getNewSpaceId(pParse); createSpace(pParse, reg_space_id, stmt); /* Indexes aren't required for VIEW's.. */ - if (!p->def->opts.is_view) - createImplicitIndices(pParse, reg_space_id); + if (!p->def->opts.is_view) { + struct Index *idx = sqlite3PrimaryKeyIndex(p); + assert(idx != NULL); + for (uint32_t i = 0; idx != NULL; idx = idx->pNext, i++) + createIndex(pParse, idx, reg_space_id, i, NULL); + } /* * Check to see if we need to create an _sequence table @@ -2553,7 +2527,7 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex) addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0); VdbeCoverage(v); - if (IsUniqueIndex(pIndex)) { + if (sql_index_is_unique(pIndex)) { int j2 = sqlite3VdbeCurrentAddr(v) + 3; sqlite3VdbeGoto(v, j2); addr2 = sqlite3VdbeCurrentAddr(v); @@ -2749,6 +2723,18 @@ constraint_is_named(const char *name) strncmp(name, "unique_unnamed_", strlen("unique_unnamed_")); } +bool +sql_index_is_primary(const struct Index *idx) +{ + return idx->def->iid == 0; +} + +bool +sql_index_is_unique(const struct Index *idx) +{ + return idx->def->opts.is_unique; +} + void sql_create_index(struct Parse *parse, struct Token *token, struct SrcList *tbl_name, struct ExprList *col_list, @@ -2913,7 +2899,6 @@ sql_create_index(struct Parse *parse, struct Token *token, index->pTable = table; index->onError = (u8) on_error; - index->index_type = idx_type; /* * TODO: Issue a warning if two or more columns of the @@ -2939,9 +2924,6 @@ sql_create_index(struct Parse *parse, struct Token *token, * still must have iid == 0. */ uint32_t iid = idx_type != SQL_INDEX_TYPE_CONSTRAINT_PK; - if (db->init.busy) - iid = db->init.index_id; - if (index_fill_def(parse, index, table, iid, name, strlen(name), col_list, idx_type, sql_stmt) != 0) goto exit_create_index; @@ -3039,14 +3021,10 @@ sql_create_index(struct Parse *parse, struct Token *token, bool is_named = constraint_is_named(existing_idx->def->name); /* CREATE TABLE t(a, UNIQUE(a), PRIMARY KEY(a)). */ - if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK) { - if (existing_idx->index_type == - SQL_INDEX_TYPE_CONSTRAINT_UNIQUE && - !is_named) { - existing_idx->index_type = - SQL_INDEX_TYPE_CONSTRAINT_PK; - goto exit_create_index; - } + if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK && + !sql_index_is_primary(existing_idx) && !is_named) { + existing_idx->def->iid = 0; + goto exit_create_index; } /* CREATE TABLE t(a, PRIMARY KEY(a), UNIQUE(a)). */ @@ -3057,10 +3035,12 @@ sql_create_index(struct Parse *parse, struct Token *token, } /* * Link the new Index structure to its table and to the - * other in-memory database structures. + * other in-memory database structures. If index created + * within CREATE TABLE statement, its iid is assigned + * in sql_init_callback(). */ assert(parse->nErr == 0); - if (db->init.busy) { + if (db->init.busy && tbl_name != NULL) { user_session->sql_flags |= SQLITE_InternChanges; index->def->iid = db->init.index_id; } @@ -3752,7 +3732,7 @@ parser_emit_unique_constraint(struct Parse *parser, sqlite3XPrintf(&err_accum, "%s.%s", def->name, col_name); } char *err_msg = sqlite3StrAccumFinish(&err_accum); - sqlite3HaltConstraint(parser, IsPrimaryKeyIndex(index) ? + sqlite3HaltConstraint(parser, sql_index_is_primary(index) ? SQLITE_CONSTRAINT_PRIMARYKEY : SQLITE_CONSTRAINT_UNIQUE, on_error, err_msg, P4_DYNAMIC, P5_ConstraintUnique); diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 853265ead..cb120384a 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -1159,7 +1159,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ bool table_ipk_autoinc = false; int reg_pk = -1; - if (IsPrimaryKeyIndex(pIdx)) { + if (sql_index_is_primary(pIdx)) { /* If PK is marked as INTEGER, use it as strict type, * not as affinity. Emit code for type checking */ if (part_count == 1) { @@ -1197,7 +1197,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ sqlite3VdbeResolveLabel(v, addrUniqueOk); continue; } - if (!IsUniqueIndex(pIdx)) { + if (!sql_index_is_unique(pIdx)) { sqlite3VdbeResolveLabel(v, addrUniqueOk); continue; } @@ -1304,7 +1304,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ int addrJump = sqlite3VdbeCurrentAddr(v) + pk_part_count; int op = OP_Ne; - int regCmp = IsPrimaryKeyIndex(pIdx) ? + int regCmp = sql_index_is_primary(pIdx) ? regIdx : regR; struct key_part *part = pPk->def->key_def->parts; @@ -1483,9 +1483,9 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ * iteration and don't open new index cursor */ - if (isUpdate || /* Condition 1 */ - IsPrimaryKeyIndex(pIdx) || /* Condition 2 */ + if (isUpdate || ! rlist_empty(&space->parent_fkey) || + sql_index_is_primary(pIdx) || /* Condition 4 */ (pIdx->def->opts.is_unique && pIdx->onError != ON_CONFLICT_ACTION_DEFAULT && @@ -1495,7 +1495,7 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ overrideError == ON_CONFLICT_ACTION_ROLLBACK) { int iIdxCur = iBase++; - if (IsPrimaryKeyIndex(pIdx)) { + if (sql_index_is_primary(pIdx)) { if (piDataCur) *piDataCur = iIdxCur; p5 = 0; @@ -1812,7 +1812,7 @@ xferOptimization(Parse * pParse, /* Parser context */ VdbeCoverage(v); sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData); sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData); - if (pDestIdx->index_type == SQL_INDEX_TYPE_CONSTRAINT_PK) + if (sql_index_is_primary(pDestIdx)) sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1); VdbeCoverage(v); diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 0ba651567..822db69ba 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -506,15 +506,10 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ pParse->nMem = 5; for (pIdx = pTab->pIndex, i = 0; pIdx; pIdx = pIdx->pNext, i++) { - const char *azOrigin[] = - { "c", "u", "u", "pk" }; sqlite3VdbeMultiLoad(v, 1, "isisi", i, pIdx->def->name, - pIdx->def->opts.is_unique, - azOrigin - [pIdx-> - index_type]); + pIdx->def->opts.is_unique); sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 5); diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index db5ee5e97..e8b8e94ae 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -120,14 +120,7 @@ sql_init_callback(struct init_data *init, const char *name, struct space *space = space_by_id(space_id); const char *zSpace = space_name(space); pIndex = sqlite3LocateIndex(db, name, zSpace); - if (pIndex == NULL) { - /* This can occur if there exists an index on a TEMP table which - * has the same name as another index on a permanent index. Since - * the permanent table is hidden by the TEMP table, we can also - * safely ignore the index on the permanent table. - */ - /* Do Nothing */ ; - } + assert(pIndex != NULL); pIndex->def->iid = index_id; } return 0; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index eb20fb31a..0ffc8d548 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1987,13 +1987,13 @@ enum sql_index_type { SQL_INDEX_TYPE_CONSTRAINT_PK, }; -/* Return true if index X is a PRIMARY KEY index */ -#define IsPrimaryKeyIndex(X) ((X)->index_type==SQL_INDEX_TYPE_CONSTRAINT_PK) +/** Simple wrapper to test index id on zero. */ +bool +sql_index_is_primary(const struct Index *idx); -/* Return true if index X is a UNIQUE index */ -#define IsUniqueIndex(X) (((X)->index_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE) || \ - ((X)->index_type == SQL_INDEX_TYPE_CONSTRAINT_PK) || \ - ((X)->index_type == SQL_INDEX_TYPE_UNIQUE)) +/** Simple getter around opts.is_unique. */ +bool +sql_index_is_unique(const struct Index *idx); /* * Each SQL index is represented in memory by an @@ -2015,12 +2015,6 @@ struct Index { * unique index. */ u8 onError; - /** - * Index type: non-unique index, unique index, index - * implementing UNIQUE constraint or index implementing - * PK constraint. - */ - enum sql_index_type index_type; /** Index definition. */ struct index_def *def; }; diff --git a/src/box/sql/update.c b/src/box/sql/update.c index 54b30705a..3fdf5a9af 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -162,7 +162,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ iIdxCur = iDataCur + 1; pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab); for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) { - if (IsPrimaryKeyIndex(pIdx) && pPk != 0) { + if (sql_index_is_primary(pIdx) && pPk != 0) { iDataCur = pParse->nTab; pTabList->a[0].iCursor = iDataCur; } diff --git a/src/box/sql/where.c b/src/box/sql/where.c index a57bad5b7..73fe070fd 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -3009,7 +3009,7 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */ * of secondary indexes, because secondary indexes * are not really store any data (only pointers to tuples). */ - int notPkPenalty = IsPrimaryKeyIndex(pProbe) ? 0 : 4; + int notPkPenalty = sql_index_is_primary(pProbe) ? 0 : 4; pNew->rRun = rSize + 16 + notPkPenalty; whereLoopOutputAdjust(pWC, pNew, rSize); rc = whereLoopInsert(pBuilder, pNew); @@ -4684,7 +4684,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */ */ if (idx_def == NULL && pIx == NULL) continue; - bool is_primary = (pIx != NULL && IsPrimaryKeyIndex(pIx)) || + bool is_primary = (pIx != NULL && sql_index_is_primary(pIx)) || (idx_def != NULL && (idx_def->iid == 0)); if (is_primary && (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) { diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index 6b3f2f78a..1c16d5323 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -226,7 +226,7 @@ sqlite3WhereExplainOneScan(Parse * pParse, /* Parse context */ assert(pIdx != NULL || idx_def != NULL); assert(!(flags & WHERE_AUTO_INDEX) || (flags & WHERE_IDX_ONLY)); - if ((pIdx != NULL && IsPrimaryKeyIndex(pIdx)) || + if ((pIdx != NULL && sql_index_is_primary(pIdx)) || (idx_def != NULL && idx_def->iid == 0)) { if (isSearch) { zFmt = "PRIMARY KEY"; @@ -1626,7 +1626,7 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t assert((pSubLoop->wsFlags & WHERE_AUTO_INDEX) == 0); if ((pSubLoop->wsFlags & WHERE_INDEXED) != 0 && (ii == 0 || pSubLoop->pIndex == pCov) - && (!IsPrimaryKeyIndex(pSubLoop->pIndex)) + && (!sql_index_is_primary(pSubLoop->pIndex)) ) { assert(pSubWInfo->a[0]. iIdxCur == iCovCur); -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 07/10] sql: remove index type from struct Index 2018-08-12 14:13 ` [tarantool-patches] [PATCH 07/10] sql: remove index type from struct Index Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! I have pushed my review fixes in a separate commit. The most significant change - I have inlined sql_index_is_unique since index's is_unique flag is intuitive enough to understand what is being checked. Also I have reduced primary index getting to just returning of the first index in pTable->pIndex list like it is done in struct space. And this patch passes the tests. But next started failing. Please, investigate why. Looks like you started inserting primary index not in the list head. On 12/08/2018 17:13, Nikita Pettik wrote: > Part of #3561 > --- > src/box/sql.c | 2 +- > src/box/sql/analyze.c | 2 +- > src/box/sql/build.c | 92 +++++++++++++++++++------------------------------ > src/box/sql/insert.c | 14 ++++---- > src/box/sql/pragma.c | 7 +--- > src/box/sql/prepare.c | 9 +---- > src/box/sql/sqliteInt.h | 18 ++++------ > src/box/sql/update.c | 2 +- > src/box/sql/where.c | 4 +-- > src/box/sql/wherecode.c | 4 +-- > 10 files changed, 58 insertions(+), 96 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 07/10] sql: remove index type from struct Index 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-21 16:31 ` n.pettik 0 siblings, 0 replies; 38+ messages in thread From: n.pettik @ 2018-08-21 16:31 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Thanks for the patch! I have pushed my review fixes in > a separate commit. > > The most significant change - I have inlined sql_index_is_unique > since index's is_unique flag is intuitive enough to understand > what is being checked. > > Also I have reduced primary index getting to just returning > of the first index in pTable->pIndex list like it is done in > struct space. And this patch passes the tests. Actually, tests fail within this patch. I don’t think that this fix worth investigation: I have already removed struct Index and now hold indexes in table->space->index and always get PK as table->space->index[0]. Patch is on its way. > But next started > failing. Please, investigate why. Looks like you started > inserting primary index not in the list head. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 08/10] sql: use secondary indexes to process OP_Delete 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (6 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 07/10] sql: remove index type from struct Index Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 09/10] sql: disable ON CONFLICT actions for indexes Nikita Pettik ` (3 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Before this patch OP_Delete used only primary index to delete entry. Since it already operates on cursor, which in turn can be open on secondary index, lets pass index id to internal routine to use it. This is required in order to remove duplicates from secondary indexes while processing INSERT/UPDATE OR REPLACE statement. --- src/box/sql.c | 17 +++++------------ src/box/sql/tarantoolInt.h | 16 +++++++++++++++- src/box/sql/vdbe.c | 2 +- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index a0aced27b..de617c8b0 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -542,23 +542,15 @@ int tarantoolSqlite3Delete(BtCursor *pCur, u8 flags) &key_size); if (key == NULL) return SQL_TARANTOOL_DELETE_FAIL; - - rc = sql_delete_by_key(pCur->space, key, key_size); + rc = sql_delete_by_key(pCur->space, pCur->index->def->iid, key, + key_size); return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_DELETE_FAIL; } -/** - * Delete entry from space by its key. - * - * @param space Space which contains record to be deleted. - * @param key Key of record to be deleted. - * @param key_size Size of key. - * - * @retval SQLITE_OK on success, SQL_TARANTOOL_DELETE_FAIL otherwise. - */ int -sql_delete_by_key(struct space *space, char *key, uint32_t key_size) +sql_delete_by_key(struct space *space, uint32_t iid, char *key, + uint32_t key_size) { struct request request; struct tuple *unused; @@ -567,6 +559,7 @@ sql_delete_by_key(struct space *space, char *key, uint32_t key_size) request.key = key; request.key_end = key + key_size; request.space_id = space->def->id; + request.index_id = iid; int rc = box_process_rw(&request, space, &unused); return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_DELETE_FAIL; diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h index 94517f608..31dc1b1bc 100644 --- a/src/box/sql/tarantoolInt.h +++ b/src/box/sql/tarantoolInt.h @@ -66,8 +66,22 @@ int tarantoolSqlite3Insert(struct space *space, const char *tuple, int tarantoolSqlite3Replace(struct space *space, const char *tuple, const char *tuple_end); int tarantoolSqlite3Delete(BtCursor * pCur, u8 flags); + +/** + * Delete entry from space by its key. + * + * @param space Space which contains record to be deleted. + * @param iid Index id. + * @param key Key of record to be deleted. + * @param key_size Size of key. + * + * @retval SQLITE_OK on success, SQL_TARANTOOL_DELETE_FAIL + * otherwise. + */ int -sql_delete_by_key(struct space *space, char *key, uint32_t key_size); +sql_delete_by_key(struct space *space, uint32_t iid, char *key, + uint32_t key_size); + int tarantoolSqlite3ClearTable(struct space *space); /** diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dc5146f81..e1eaf91a4 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4344,7 +4344,7 @@ case OP_SDelete: { struct space *space = space_by_id(pOp->p1); assert(space != NULL); assert(space_is_system(space)); - rc = sql_delete_by_key(space, pIn2->z, pIn2->n); + rc = sql_delete_by_key(space, 0, pIn2->z, pIn2->n); if (rc) goto abort_due_to_error; if (pOp->p5 & OPFLAG_NCHANGE) -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 08/10] sql: use secondary indexes to process OP_Delete 2018-08-12 14:13 ` [tarantool-patches] [PATCH 08/10] sql: use secondary indexes to process OP_Delete Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches Thanks for the patch! I have pushed my review fixes in a separate commit. On 12/08/2018 17:13, Nikita Pettik wrote: > Before this patch OP_Delete used only primary index to delete entry. > Since it already operates on cursor, which in turn can be open on > secondary index, lets pass index id to internal routine to use it. > This is required in order to remove duplicates from secondary indexes > while processing INSERT/UPDATE OR REPLACE statement. > --- > src/box/sql.c | 17 +++++------------ > src/box/sql/tarantoolInt.h | 16 +++++++++++++++- > src/box/sql/vdbe.c | 2 +- > 3 files changed, 21 insertions(+), 14 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 09/10] sql: disable ON CONFLICT actions for indexes 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (7 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 08/10] sql: use secondary indexes to process OP_Delete Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 10/10] sql: move autoincrement field number to server Nikita Pettik ` (2 subsequent siblings) 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik 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.c b/src/box/sql.c index de617c8b0..1f59946f0 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1497,13 +1497,6 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, void *buf) p = enc->encode_map(base, 2); /* Mark as unique pk and unique indexes */ p = enc->encode_str(p, "unique", 6); - /* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks - * will be made by Tarantool. However, Tarantool doesn't have ON - * CONFLIT option, so in that case (except ON CONFLICT ABORT, which is - * default behavior) uniqueness will be checked by SQL. - * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by - * Tarantool. - */ p = enc->encode_bool(p, sql_index_is_unique(index)); p = enc->encode_str(p, "sql", 3); p = enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 3ef9ea96e..9d7f0a4e0 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -115,52 +115,6 @@ sql_finish_coding(struct Parse *parse_context) } } -/** - * This is a function which should be called during execution - * of sqlite3EndTable. It set defaults for columns having no - * separate NULL/NOT NULL specifiers and ensures that only - * PRIMARY KEY constraint may have ON CONFLICT REPLACE clause. - * - * @param parser SQL Parser object. - * @param table Space which should be checked. - * @retval -1 on error. Parser SQL_TARANTOOL_ERROR is set. - * @retval 0 on success. - */ -static int -actualize_on_conflict_actions(struct Parse *parser, struct Table *table) -{ - const char *err_msg = NULL; - struct field_def *field = table->def->fields; - struct Index *pk = sqlite3PrimaryKeyIndex(table); - for (uint32_t i = 0; i < table->def->field_count; ++i, ++field) { - if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) { - /* Set default nullability NONE. */ - field->nullable_action = ON_CONFLICT_ACTION_NONE; - field->is_nullable = true; - } - if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE && - (pk == NULL || key_def_find(pk->def->key_def, i) == NULL)) - goto non_pk_on_conflict_error; - } - - for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) { - if (idx->onError == ON_CONFLICT_ACTION_REPLACE && - !sql_index_is_primary(idx)) - goto non_pk_on_conflict_error; - } - - return 0; - -non_pk_on_conflict_error: - err_msg = tt_sprintf("only PRIMARY KEY constraint can have " - "ON CONFLICT REPLACE clause - %s", - table->def->name); - diag_set(ClientError, ER_SQL, err_msg); - parser->rc = SQL_TARANTOOL_ERROR; - parser->nErr++; - return -1; -} - /* * Locate the in-memory structure that describes a particular database * table given the name of that table. Return NULL if not found. @@ -871,7 +825,6 @@ field_def_create_for_pk(struct Parse *parser, struct field_def *field, void sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ ExprList * pList, /* List of field names to be indexed */ - int onError, /* What to do with a uniqueness conflict */ int autoInc, /* True if the AUTOINCREMENT keyword is present */ enum sort_order sortOrder ) @@ -926,7 +879,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ &token, 0)); if (list == NULL) goto primary_key_exit; - sql_create_index(pParse, 0, 0, list, onError, 0, SORT_ORDER_ASC, + sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC, false, SQL_INDEX_TYPE_CONSTRAINT_PK); if (db->mallocFailed) goto primary_key_exit; @@ -935,8 +888,8 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ "INTEGER PRIMARY KEY or INT PRIMARY KEY"); goto primary_key_exit; } else { - sql_create_index(pParse, 0, 0, pList, onError, 0, sortOrder, - false, SQL_INDEX_TYPE_CONSTRAINT_PK); + sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false, + SQL_INDEX_TYPE_CONSTRAINT_PK); pList = 0; if (pParse->nErr > 0) goto primary_key_exit; @@ -1669,8 +1622,19 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } } - if (actualize_on_conflict_actions(pParse, p)) - goto cleanup; + /* + * Actualize conflict action for NOT NULL constraint. + * Set defaults for columns having no separate + * NULL/NOT NULL specifiers. + */ + struct field_def *field = p->def->fields; + for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) { + if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) { + /* Set default nullability NONE. */ + field->nullable_action = ON_CONFLICT_ACTION_NONE; + field->is_nullable = true; + } + } if (db->init.busy) { /* @@ -2738,9 +2702,8 @@ sql_index_is_unique(const struct Index *idx) void sql_create_index(struct Parse *parse, struct Token *token, struct SrcList *tbl_name, struct ExprList *col_list, - enum on_conflict_action on_error, struct Token *start, - enum sort_order sort_order, bool if_not_exist, - enum sql_index_type idx_type) { + struct Token *start, enum sort_order sort_order, + bool if_not_exist, enum sql_index_type idx_type) { /* The index to be created. */ struct Index *index = NULL; /* Name of the index. */ @@ -2898,8 +2861,6 @@ sql_create_index(struct Parse *parse, struct Token *token, goto exit_create_index; index->pTable = table; - index->onError = (u8) on_error; - /* * TODO: Issue a warning if two or more columns of the * index are identical. @@ -3003,21 +2964,6 @@ sql_create_index(struct Parse *parse, struct Token *token, if (k != key_def->part_count) continue; - if (index->onError != existing_idx->onError) { - if (index->onError != - ON_CONFLICT_ACTION_DEFAULT && - existing_idx->onError != - ON_CONFLICT_ACTION_DEFAULT) - sqlite3ErrorMsg(parse, - "conflicting "\ - "ON CONFLICT "\ - "clauses specified"); - - if (existing_idx->onError == - ON_CONFLICT_ACTION_DEFAULT) - existing_idx->onError = index->onError; - } - bool is_named = constraint_is_named(existing_idx->def->name); /* CREATE TABLE t(a, UNIQUE(a), PRIMARY KEY(a)). */ 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 @@ -272,10 +272,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ char *zTab; /* Name of the table into which we are inserting */ int i, j; /* Loop counters */ Vdbe *v; /* Generate code into this virtual machine */ - Index *pIdx; /* For looping over indices of the table */ int nColumn; /* Number of columns in the data */ - int iDataCur = 0; /* VDBE cursor that is the main data repository */ - int iIdxCur = 0; /* First index cursor */ int endOfLoop; /* Label for the end of the insertion loop */ int srcTab = 0; /* Data comes from this temporary cursor if >=0 */ int addrInsTop = 0; /* Jump to label "D" */ @@ -375,11 +372,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } #endif /* SQLITE_OMIT_XFER_OPT */ - /* Allocate registers for holding the tupleid of the new row, - * the content of the new row, and the assembled row record. + /* + * Allocate registers for holding the tupleid of the new + * row (if it isn't required first register will contain + * NULL), the content of the new row, and the assembled + * row record. */ regTupleid = regIns = pParse->nMem + 1; - pParse->nMem += def->field_count + 1; + pParse->nMem += def->field_count + 2; regData = regTupleid + 1; /* If the INSERT statement included an IDLIST term, then make sure @@ -550,27 +550,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */ regRowCount = ++pParse->nMem; sqlite3VdbeAddOp2(v, OP_Integer, 0, regRowCount); } - - /* If this is not a view, open the table and and all indices */ - if (!is_view) { - int nIdx; - nIdx = - sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0, - -1, 0, &iDataCur, &iIdxCur, - on_error, 0); - - aRegIdx = sqlite3DbMallocRawNN(db, sizeof(int) * (nIdx + 1)); - if (aRegIdx == 0) { - goto insert_cleanup; - } - for (i = 0, pIdx = pTab->pIndex; i < nIdx; - pIdx = pIdx->pNext, i++) { - assert(pIdx); - aRegIdx[i] = ++pParse->nMem; - pParse->nMem += pIdx->def->key_def->part_count; - } - } - /* This is the top of the main insertion loop */ if (useTempTable) { /* This block codes the top of loop only. The complete loop is the @@ -764,20 +743,20 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } } - /* Generate code to check constraints and generate index keys - and do the insertion. + /* + * Generate code to check constraints and process + * final insertion. */ - int isReplace; /* Set to true if constraints may cause a replace */ - struct on_conflict on_conflict; - on_conflict.override_error = on_error; - on_conflict.optimized_action = ON_CONFLICT_ACTION_NONE; - sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur, - iIdxCur, regIns, 0, - true, &on_conflict, - endOfLoop, &isReplace, 0); + vdbe_emit_constraint_checks(pParse, pTab, regIns + 1, + on_error, endOfLoop, 0); fkey_emit_check(pParse, pTab, 0, regIns, 0); - vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], - &on_conflict); + int pk_cursor = pParse->nTab++; + struct space *space = space_by_id(pTab->def->id); + assert(space != NULL); + vdbe_emit_open_cursor(pParse, pk_cursor, 0, space); + vdbe_emit_insertion_completion(v, pk_cursor, regIns + 1, + pTab->def->field_count, + on_error); } /* Update the count of rows that are inserted @@ -874,646 +853,239 @@ checkConstraintUnchanged(Expr * pExpr, int *aiChng) return !w.eCode; } -/* - * Generate code to do constraint checks prior to an INSERT or an UPDATE - * on table pTab. - * - * The regNewData parameter is the first register in a range that contains - * the data to be inserted or the data after the update. There will be - * pTab->def->field_count+1 registers in this range. The first register (the - * one that regNewData points to) will contain NULL. The second register - * in the range will contain the content of the first table column. - * The third register will contain the content of the second table column. - * And so forth. - * - * The regOldData parameter is similar to regNewData except that it contains - * the data prior to an UPDATE rather than afterwards. regOldData is zero - * for an INSERT. This routine can distinguish between UPDATE and INSERT by - * checking regOldData for zero. - * - * For an UPDATE, the pkChng boolean is true if the primary key - * might be modified by the UPDATE. If pkChng is false, then the key of - * the iDataCur content table is guaranteed to be unchanged by the UPDATE. - * - * On an INSERT, pkChng will only be true if the INSERT statement provides - * an integer value for INTEGER PRIMARY KEY alias. - * - * The code generated by this routine will store new index entries into - * registers identified by aRegIdx[]. No index entry is created for - * indices where aRegIdx[i]==0. The order of indices in aRegIdx[] is - * the same as the order of indices on the linked list of indices - * at pTab->pIndex. - * - * The caller must have already opened writeable cursors on the main - * table and all applicable indices (that is to say, all indices for which - * aRegIdx[] is not zero). iDataCur is the cursor for the PRIMARY KEY index. - * iIdxCur is the cursor for the first index in the pTab->pIndex list. - * Cursors for other indices are at iIdxCur+N for the N-th element - * of the pTab->pIndex list. - * - * This routine also generates code to check constraints. NOT NULL, - * CHECK, and UNIQUE constraints are all checked. If a constraint fails, - * then the appropriate action is performed. There are five possible - * actions: ROLLBACK, ABORT, FAIL, REPLACE, and IGNORE. - * - * Constraint type Action What Happens - * --------------- ---------- ---------------------------------------- - * any ROLLBACK The current transaction is rolled back and - * sqlite3_step() returns immediately with a - * return code of SQLITE_CONSTRAINT. - * - * any ABORT Back out changes from the current command - * only (do not do a complete rollback) then - * cause sqlite3_step() to return immediately - * with SQLITE_CONSTRAINT. - * - * any FAIL Sqlite3_step() returns immediately with a - * return code of SQLITE_CONSTRAINT. The - * transaction is not rolled back and any - * changes to prior rows are retained. - * - * any IGNORE The attempt in insert or update the current - * row is skipped, without throwing an error. - * Processing continues with the next row. - * (There is an immediate jump to ignoreDest.) - * - * NOT NULL REPLACE The NULL value is replace by the default - * value for that column. If the default value - * is NULL, the action is the same as ABORT. - * - * UNIQUE REPLACE The other row that conflicts with the row - * being inserted is removed. - * - * CHECK REPLACE Illegal. The results in an exception. - * - * Which action to take is determined by the override_error - * parameter in struct on_conflict. - * Or if override_error==ON_CONFLICT_ACTION_DEFAULT, then the - * pParse->onError parameter is used. Or if - * pParse->onError==ON_CONFLICT_ACTION_DEFAULT then the on_error - * value for the constraint is used. - */ void -sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ - Table * pTab, /* The table being inserted or updated */ - int *aRegIdx, /* Use register aRegIdx[i] for index i. 0 for unused */ - int iDataCur, /* Canonical data cursor (main table or PK index) */ - int iIdxCur, /* First index cursor */ - int regNewData, /* First register in a range holding values to insert */ - int regOldData, /* Previous content. 0 for INSERTs */ - u8 pkChng, /* Non-zero if the PRIMARY KEY changed */ - struct on_conflict *on_conflict, - int ignoreDest, /* Jump to this label on an ON_CONFLICT_ACTION_IGNORE resolution */ - int *pbMayReplace, /* OUT: Set to true if constraint may cause a replace */ - int *aiChng) /* column i is unchanged if aiChng[i]<0 */ +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); + /* 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 */ + sqlite3MayAbort(parse_context); case ON_CONFLICT_ACTION_ROLLBACK: case ON_CONFLICT_ACTION_FAIL: { - char *zMsg = - sqlite3MPrintf(db, "%s.%s", def->name, - def->fields[i].name); - sqlite3VdbeAddOp3(v, OP_HaltIfNull, - SQLITE_CONSTRAINT_NOTNULL, - on_error, regNewData + 1 + i); - sqlite3VdbeAppendP4(v, zMsg, P4_DYNAMIC); - sqlite3VdbeChangeP5(v, P5_ConstraintNotNull); - VdbeCoverage(v); - break; - } + char *err_msg = sqlite3MPrintf(db, "%s.%s", def->name, + def->fields[i].name); + sqlite3VdbeAddOp3(v, OP_HaltIfNull, + SQLITE_CONSTRAINT_NOTNULL, + on_conflict_nullable, + new_tuple_reg + i); + sqlite3VdbeAppendP4(v, err_msg, P4_DYNAMIC); + sqlite3VdbeChangeP5(v, P5_ConstraintNotNull); + break; + } case ON_CONFLICT_ACTION_IGNORE: { - sqlite3VdbeAddOp2(v, OP_IsNull, - regNewData + 1 + i, - ignoreDest); - VdbeCoverage(v); - break; - } - default:{ - assert(on_error == ON_CONFLICT_ACTION_REPLACE); - addr1 = - sqlite3VdbeAddOp1(v, OP_NotNull, - regNewData + 1 + i); - VdbeCoverage(v); - sqlite3ExprCode(pParse, dflt, - regNewData + 1 + i); - sqlite3VdbeJumpHere(v, addr1); - break; - } + sqlite3VdbeAddOp2(v, OP_IsNull, new_tuple_reg + i, + ignore_label); + break; + } + case ON_CONFLICT_ACTION_REPLACE: { + int addr1 = sqlite3VdbeAddOp1(v, OP_NotNull, + new_tuple_reg + i); + sqlite3ExprCode(parse_context, dflt, + new_tuple_reg + i); + sqlite3VdbeJumpHere(v, addr1); + break; + } + default: + unreachable(); } } - /* - * Get server checks. - * Test all CHECK constraints. + * For CHECK constraint and for INSERT/UPDATE conflict + * action DEFAULT and ABORT in fact has the same meaning. */ - ExprList *checks = space_checks_expr_list(pTab->def->id); + if (on_conflict == ON_CONFLICT_ACTION_DEFAULT) + on_conflict = ON_CONFLICT_ACTION_ABORT; + /* Test all CHECK constraints. */ + struct ExprList *checks = space_checks_expr_list(tab->def->id); if (checks != NULL && (user_session->sql_flags & SQLITE_IgnoreChecks) == 0) { - pParse->ckBase = regNewData + 1; - if (override_error != ON_CONFLICT_ACTION_DEFAULT) - on_error = override_error; - else - on_error = ON_CONFLICT_ACTION_ABORT; - + parse_context->ckBase = new_tuple_reg; for (int i = 0; i < checks->nExpr; i++) { - int allOk; - Expr *pExpr = checks->a[i].pExpr; - if (aiChng - && checkConstraintUnchanged(pExpr, aiChng)) + struct Expr *expr = checks->a[i].pExpr; + if (is_update && + checkConstraintUnchanged(expr, upd_cols)) continue; - allOk = sqlite3VdbeMakeLabel(v); - sqlite3ExprIfTrue(pParse, pExpr, allOk, + int all_ok = sqlite3VdbeMakeLabel(v); + sqlite3ExprIfTrue(parse_context, expr, all_ok, SQLITE_JUMPIFNULL); - if (on_error == ON_CONFLICT_ACTION_IGNORE) { - sqlite3VdbeGoto(v, ignoreDest); + if (on_conflict == ON_CONFLICT_ACTION_IGNORE) { + sqlite3VdbeGoto(v, ignore_label); } else { - char *zName = checks->a[i].zName; - if (zName == NULL) - zName = def->name; - if (on_error == ON_CONFLICT_ACTION_REPLACE) - on_error = ON_CONFLICT_ACTION_ABORT; - sqlite3HaltConstraint(pParse, + char *name = checks->a[i].zName; + if (name == NULL) + name = def->name; + enum on_conflict_action on_conflict_check = + on_conflict; + if (on_conflict == ON_CONFLICT_ACTION_REPLACE) + on_conflict_check = + ON_CONFLICT_ACTION_ABORT; + sqlite3HaltConstraint(parse_context, SQLITE_CONSTRAINT_CHECK, - on_error, zName, + on_conflict_check, name, P4_TRANSIENT, P5_ConstraintCheck); } - sqlite3VdbeResolveLabel(v, allOk); + sqlite3VdbeResolveLabel(v, all_ok); } } - + sql_emit_table_affinity(v, tab->def, new_tuple_reg); /* - * Test all UNIQUE constraints by creating entries for - * each UNIQUE index and making sure that duplicate entries - * do not already exist. Compute the revised record entries - * for indices as we go. - * - * This loop also handles the case of the PRIMARY KEY index. + * If PK is marked as INTEGER, use it as strict type, + * not as affinity. Emit code for type checking. + * FIXME: should be removed after introducing + * strict typing. */ - pIdx = pTab->pIndex; - for (int ix = 0; pIdx != NULL; pIdx = pIdx->pNext, ix++) { - int regIdx; /* Range of registers hold conent for pIdx */ - int regR; /* Range of registers holding conflicting PK */ - int iThisCur; /* Cursor for this UNIQUE index */ - int addrUniqueOk; /* Jump here if the UNIQUE constraint is satisfied */ - bool uniqueByteCodeNeeded = false; - - /* - * ABORT and DEFAULT error actions can be handled - * by Tarantool facilitites without emitting VDBE - * bytecode. - */ - if ((pIdx->onError != ON_CONFLICT_ACTION_ABORT && - pIdx->onError != ON_CONFLICT_ACTION_DEFAULT) || - (override_error != ON_CONFLICT_ACTION_ABORT && - override_error != ON_CONFLICT_ACTION_DEFAULT)) { - uniqueByteCodeNeeded = true; - } - - if (aRegIdx[ix] == 0) - continue; /* Skip indices that do not change */ - if (bAffinityDone == 0) { - sql_emit_table_affinity(v, pTab->def, regNewData + 1); - bAffinityDone = 1; - } - iThisCur = iIdxCur + ix; - addrUniqueOk = sqlite3VdbeMakeLabel(v); - - /* Create a record for this index entry as it should appear after - * the insert or update. Store that record in the aRegIdx[ix] register - */ - regIdx = aRegIdx[ix] + 1; - uint32_t part_count = pIdx->def->key_def->part_count; - if (uniqueByteCodeNeeded) { - for (uint32_t i = 0; i < part_count; ++i) { - uint32_t fieldno = - pIdx->def->key_def->parts[i].fieldno; - int reg; - /* - * OP_SCopy copies value in - * separate register, which later - * will be used by OP_NoConflict. - * But OP_NoConflict is necessary - * only in cases when bytecode is - * needed for proper UNIQUE - * constraint handling. - */ - reg = fieldno + regNewData + 1; - sqlite3VdbeAddOp2(v, OP_SCopy, reg, regIdx + i); - VdbeComment((v, "%s", - def->fields[fieldno].name)); + struct Index *pk = sqlite3PrimaryKeyIndex(tab); + uint32_t part_count = pk->def->key_def->part_count; + if (part_count == 1) { + uint32_t fieldno = pk->def->key_def->parts[0].fieldno; + int reg_pk = new_tuple_reg + fieldno; + if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { + int skip_if_null = sqlite3VdbeMakeLabel(v); + if (tab->iAutoIncPKey >= 0) { + sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, + skip_if_null); } + sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, 0); + sqlite3VdbeResolveLabel(v, skip_if_null); } - - bool table_ipk_autoinc = false; - int reg_pk = -1; - if (sql_index_is_primary(pIdx)) { - /* If PK is marked as INTEGER, use it as strict type, - * not as affinity. Emit code for type checking */ - if (part_count == 1) { - uint32_t fieldno = - pIdx->def->key_def->parts[0].fieldno; - reg_pk = regNewData + 1 + fieldno; - - if (pTab->def->fields[fieldno].affinity == - AFFINITY_INTEGER) { - int skip_if_null = sqlite3VdbeMakeLabel(v); - if (pTab->iAutoIncPKey >= 0) { - sqlite3VdbeAddOp2(v, OP_IsNull, - reg_pk, - skip_if_null); - table_ipk_autoinc = true; - } - sqlite3VdbeAddOp2(v, OP_MustBeInt, - reg_pk, - 0); - sqlite3VdbeResolveLabel(v, skip_if_null); - } - } - - sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1, - def->field_count, aRegIdx[ix]); - VdbeComment((v, "for %s", pIdx->def->name)); - } - - /* In an UPDATE operation, if this index is the PRIMARY KEY - * index and there has been no change the primary key, then no - * collision is possible. The collision detection - * logic below can all be skipped. - */ - if (isUpdate && pPk == pIdx && pkChng == 0) { - sqlite3VdbeResolveLabel(v, addrUniqueOk); + } + /* + * Other actions except for REPLACE and UPDATE OR IGNORE + * can be handled by setting appropriate flag in OP_Halt. + */ + if (!(on_conflict == ON_CONFLICT_ACTION_IGNORE && is_update) && + on_conflict != ON_CONFLICT_ACTION_REPLACE) + return; + /* Calculate MAX range of register we may occupy. */ + uint32_t reg_count = 0; + for (struct Index *idx = tab->pIndex; idx != NULL; idx = idx->pNext) { + if (idx->def->key_def->part_count > reg_count) + reg_count = idx->def->key_def->part_count; + } + int idx_key_reg = ++parse_context->nMem; + parse_context->nMem += reg_count; + /* + * To handle INSERT OR REPLACE statement we should check + * all unique secondary indexes on containing entry with + * the same key. If index contains it, we must invoke + * ON DELETE trigger and remove entry. + * For UPDATE OR IGNORE we must check that no entries + * exist in indexes which contain updated columns. + * Otherwise, we should skip removal of old entry and + * insertion of new one. + */ + for (struct Index *idx = tab->pIndex; idx != NULL; idx = idx->pNext) { + /* Conflicts may occur only in UNIQUE indexes. */ + if (! sql_index_is_unique(idx)) continue; - } - if (!sql_index_is_unique(pIdx)) { - sqlite3VdbeResolveLabel(v, addrUniqueOk); + if (on_conflict == ON_CONFLICT_ACTION_IGNORE) { + /* + * We are interested only in indexes which + * contain updated columns. + */ + struct key_def *kd = idx->def->key_def; + for (uint32_t i = 0; i < kd->part_count; ++i) { + if (upd_cols[kd->parts[i].fieldno] >= 0) + goto process_index; + } continue; } - - on_error = pIdx->onError; +process_index: ; + int cursor = parse_context->nTab++; + vdbe_emit_open_cursor(parse_context, cursor, idx->def->iid, + space_by_id(tab->def->id)); /* - * If we are doing INSERT OR IGNORE, - * INSERT OR FAIL, then error action will - * be the same for all space indexes and - * uniqueness can be ensured by Tarantool. + * If there is no conflict in current index, just + * jump to the start of next iteration. Label is + * used for REPLACE action only. */ - if (override_error == ON_CONFLICT_ACTION_FAIL || - override_error == ON_CONFLICT_ACTION_IGNORE || - override_error == ON_CONFLICT_ACTION_ABORT) { - sqlite3VdbeResolveLabel(v, addrUniqueOk); - continue; - } - - 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; - + int skip_index = sqlite3VdbeMakeLabel(v); /* - * Collision detection may be omitted if all of - * the following are true: - * (1) The conflict resolution algorithm is - * REPLACE or IGNORE. - * (2) There are no secondary indexes on the - * table. - * (3) No delete triggers need to be fired if - * there is a conflict. - * (4) No FK constraint counters need to be - * updated if a conflict occurs. + * Copy index key to continuous range of + * registers. Initially whole tuple is located at + * [new_tuple_reg ... new_tuple_reg + field_count] + * We are copying key to [reg ... reg + part_count] */ - bool no_secondary_indexes = (ix == 0 && - pIdx->pNext == 0); - bool proper_error_action = - (on_error == ON_CONFLICT_ACTION_REPLACE || - on_error == ON_CONFLICT_ACTION_IGNORE); - bool no_delete_triggers = - (user_session->sql_flags & SQLITE_RecTriggers) == 0 || - sql_triggers_exist(pTab, TK_DELETE, NULL, NULL) == NULL; - struct space *space = space_by_id(pTab->def->id); - assert(space != NULL); - bool no_foreign_keys = - (user_session->sql_flags & SQLITE_ForeignKeys) == 0 || - (rlist_empty(&space->child_fkey) && - ! rlist_empty(&space->parent_fkey)); - - if (no_secondary_indexes && no_foreign_keys && - proper_error_action && no_delete_triggers) { - /* - * Save that possible optimized error - * action, which can be used later - * during execution of - * vdbe_emit_insertion_completion(). - */ - on_conflict->optimized_action = on_error; - sqlite3VdbeResolveLabel(v, addrUniqueOk); - continue; - } - - /* Check to see if the new index entry will be unique */ - if (table_ipk_autoinc) - sqlite3VdbeAddOp2(v, OP_IsNull, - reg_pk, - addrUniqueOk); - - if (uniqueByteCodeNeeded) { - sqlite3VdbeAddOp4Int(v, OP_NoConflict, iThisCur, - addrUniqueOk, regIdx, - pIdx->def->key_def->part_count); + uint32_t part_count = idx->def->key_def->part_count; + for (uint32_t i = 0; i < part_count; ++i) { + uint32_t fieldno = idx->def->key_def->parts[i].fieldno; + int reg = fieldno + new_tuple_reg; + sqlite3VdbeAddOp2(v, OP_SCopy, reg, idx_key_reg + i); } - VdbeCoverage(v); - - uint32_t pk_part_count = pPk->def->key_def->part_count; - /* Generate code to handle collisions */ - regR = pIdx == pPk ? regIdx : - sqlite3GetTempRange(pParse, pk_part_count); - if (isUpdate || on_error == ON_CONFLICT_ACTION_REPLACE) { - int x; - /* Extract the PRIMARY KEY from the end of the index entry and - * store it in registers regR..regR+nPk-1 - */ - if (pIdx != pPk) { - for (uint32_t i = 0; i < pk_part_count; i++) { - x = pPk->def->key_def->parts[i].fieldno; - sqlite3VdbeAddOp3(v, OP_Column, - iThisCur, x, regR + i); - VdbeComment((v, "%s.%s", def->name, - def->fields[x].name)); - } - } - if (isUpdate && uniqueByteCodeNeeded) { - /* Only conflict if the new PRIMARY KEY - * values are actually different from the old. - * - * For a UNIQUE index, only conflict if the PRIMARY KEY values - * of the matched index row are different from the original PRIMARY - * KEY values of this row before the update. - */ - int addrJump = sqlite3VdbeCurrentAddr(v) + - pk_part_count; - int op = OP_Ne; - int regCmp = sql_index_is_primary(pIdx) ? - regIdx : regR; - struct key_part *part = - pPk->def->key_def->parts; - for (uint32_t i = 0; i < pk_part_count; - ++i, ++part) { - char *p4 = (char *) part->coll; - x = part->fieldno; - if (pTab->def->id == 0) - x = -1; - if (i == (pk_part_count - 1)) { - addrJump = addrUniqueOk; - op = OP_Eq; - } - sqlite3VdbeAddOp4(v, op, - regOldData + 1 + x, - addrJump, regCmp + i, - p4, P4_COLLSEQ); - sqlite3VdbeChangeP5(v, SQLITE_NOTNULL); - VdbeCoverageIf(v, op == OP_Eq); - VdbeCoverageIf(v, op == OP_Ne); - } - } - } - - /* - * Generate bytecode which executes when entry is - * not unique for constraints with following - * error actions: - * 1) ROLLBACK. - * 2) FAIL. - * 3) IGNORE. - * 4) REPLACE. - * - * ON CONFLICT ABORT is a default error action - * for constraints and therefore can be handled - * by Tarantool facilities. - */ - assert(on_error != ON_CONFLICT_ACTION_NONE); - switch (on_error) { - case ON_CONFLICT_ACTION_FAIL: - case ON_CONFLICT_ACTION_ROLLBACK: - parser_emit_unique_constraint(pParse, on_error, pIdx); - break; - case ON_CONFLICT_ACTION_ABORT: - break; - case ON_CONFLICT_ACTION_IGNORE: - sqlite3VdbeGoto(v, ignoreDest); - break; - default: { - struct sql_trigger *trigger = NULL; - assert(on_error == ON_CONFLICT_ACTION_REPLACE); - sql_set_multi_write(pParse, true); - if (user_session->sql_flags & SQLITE_RecTriggers) { - trigger = sql_triggers_exist(pTab, TK_DELETE, - NULL, NULL); - } - sql_generate_row_delete(pParse, pTab, trigger, - iDataCur, regR, pk_part_count, + if (on_conflict == ON_CONFLICT_ACTION_IGNORE) { + sqlite3VdbeAddOp4Int(v, OP_Found, cursor, + ignore_label, idx_key_reg, + part_count); + } else { + assert(on_conflict == ON_CONFLICT_ACTION_REPLACE); + sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, + skip_index, idx_key_reg, + part_count); + sql_set_multi_write(parse_context, true); + struct sql_trigger *trigger = + sql_triggers_exist(tab, TK_DELETE, NULL, NULL); + sql_generate_row_delete(parse_context, tab, trigger, + cursor, idx_key_reg, part_count, false, ON_CONFLICT_ACTION_REPLACE, - pIdx == pPk ? ONEPASS_SINGLE : - ONEPASS_OFF, -1); - seenReplace = 1; - break; + ONEPASS_SINGLE, -1); + sqlite3VdbeResolveLabel(v, skip_index); } - } - sqlite3VdbeResolveLabel(v, addrUniqueOk); - if (regR != regIdx) - sqlite3ReleaseTempRange(pParse, regR, pk_part_count); } - - *pbMayReplace = seenReplace; - VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace)); } void -vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, - struct on_conflict *on_conflict) +vdbe_emit_insertion_completion(struct Vdbe *v, int cursor_id, int raw_data_reg, + uint32_t tuple_len, + enum on_conflict_action on_conflict) { assert(v != NULL); - int opcode; - enum on_conflict_action override_error = on_conflict->override_error; - enum on_conflict_action optimized_action = - on_conflict->optimized_action; - - if (override_error == ON_CONFLICT_ACTION_REPLACE || - (optimized_action == ON_CONFLICT_ACTION_REPLACE && - override_error == ON_CONFLICT_ACTION_DEFAULT)) - opcode = OP_IdxReplace; - else - opcode = OP_IdxInsert; - + int opcode = OP_IdxInsert; u16 pik_flags = OPFLAG_NCHANGE; - if (override_error == ON_CONFLICT_ACTION_IGNORE) - pik_flags |= OPFLAG_OE_IGNORE; - else if (override_error == ON_CONFLICT_ACTION_IGNORE || - (optimized_action == ON_CONFLICT_ACTION_IGNORE && - override_error == ON_CONFLICT_ACTION_DEFAULT)) + if (on_conflict == ON_CONFLICT_ACTION_IGNORE) pik_flags |= OPFLAG_OE_IGNORE; - else if (override_error == ON_CONFLICT_ACTION_FAIL) + else if (on_conflict == ON_CONFLICT_ACTION_FAIL) pik_flags |= OPFLAG_OE_FAIL; - - sqlite3VdbeAddOp2(v, opcode, cursor_id, tuple_id); + else if (on_conflict == ON_CONFLICT_ACTION_ROLLBACK) + pik_flags |= OPFLAG_OE_ROLLBACK; + sqlite3VdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len, + raw_data_reg + tuple_len); + sqlite3VdbeAddOp2(v, opcode, cursor_id, raw_data_reg + tuple_len); sqlite3VdbeChangeP5(v, pik_flags); } -/* - * Allocate cursors for the pTab table and all its indices and generate - * code to open and initialized those cursors. - * - * The cursor for the object that contains the complete data (index) - * is returned in *piDataCur. The first index cursor is - * returned in *piIdxCur. The number of indices is returned. - * - * Use iBase as the first cursor (the first index) if it is non-negative. - * If iBase is negative, then allocate the next available cursor. - * - * *piDataCur will be somewhere in the range - * of *piIdxCurs, depending on where the PRIMARY KEY index appears on the - * pTab->pIndex list. - */ -int -sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ - Table * pTab, /* Table to be opened */ - int op, /* OP_OpenRead or OP_OpenWrite */ - u8 p5, /* P5 value for OP_Open* opcodes */ - int iBase, /* Use this for the table cursor, if there is one */ - u8 * aToOpen, /* If not NULL: boolean for each table and index */ - int *piDataCur, /* Write the database source cursor number here */ - int *piIdxCur, /* Write the first index cursor number here */ - u8 overrideError, /* Override error action for indexes */ - u8 isUpdate) /* Opened for udpate or not */ -{ - int i; - int iDataCur; - Index *pIdx; - Vdbe *v; - - assert(op == OP_OpenRead || op == OP_OpenWrite); - assert(op == OP_OpenWrite || p5 == 0); - v = sqlite3GetVdbe(pParse); - assert(v != 0); - if (iBase < 0) - iBase = pParse->nTab; - iDataCur = iBase++; - if (piDataCur) - *piDataCur = iDataCur; - if (piIdxCur) - *piIdxCur = iBase; - struct space *space = space_by_id(pTab->def->id); - assert(space != NULL); - /* One iteration of this cycle adds OpenRead/OpenWrite which - * opens cursor for current index. - * - * For update cursor on index is required, however if insertion - * is done by Tarantool only, cursor is not needed so don't - * open it. - */ - for (i = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, i++) { - /* Cursor is needed: - * 1) For indexes in UPDATE statement - * 2) For PRIMARY KEY index - * 3) For table mentioned in FOREIGN KEY constraint - * 4) For an index which has ON CONFLICT action which require - * VDBE bytecode - ROLLBACK, IGNORE, FAIL, REPLACE: - * 1. If user specified non-default ON CONFLICT (not - * ON_CONFLICT_ACTION_NONE or _Abort) clause - * for an non-primary unique index, then bytecode is needed - * for proper error action. - * 2. INSERT/UPDATE OR IGNORE/ABORT/FAIL/REPLACE - - * Tarantool is able handle by itself. - * INSERT/UPDATE OR ROLLBACK - sql - * bytecode is needed in this case. - * - * If all conditions from list above are false then skip - * iteration and don't open new index cursor - */ - - if (isUpdate || - ! rlist_empty(&space->parent_fkey) || - sql_index_is_primary(pIdx) || - /* Condition 4 */ - (pIdx->def->opts.is_unique && - pIdx->onError != ON_CONFLICT_ACTION_DEFAULT && - /* Condition 4.1 */ - pIdx->onError != ON_CONFLICT_ACTION_ABORT) || - /* Condition 4.2 */ - overrideError == ON_CONFLICT_ACTION_ROLLBACK) { - - int iIdxCur = iBase++; - if (sql_index_is_primary(pIdx)) { - if (piDataCur) - *piDataCur = iIdxCur; - p5 = 0; - } - if (aToOpen == 0 || aToOpen[i + 1]) { - sqlite3VdbeAddOp4(v, op, iIdxCur, pIdx->def->iid, - 0, (void *) space, - P4_SPACEPTR); - sqlite3VdbeChangeP5(v, p5); - VdbeComment((v, "%s", pIdx->def->name)); - } - } - } - if (iBase > pParse->nTab) - pParse->nTab = iBase; - return i; -} - #ifdef SQLITE_TEST /* * The following global variable is incremented whenever the @@ -1545,9 +1117,6 @@ xferCompatibleIndex(Index * pDest, Index * pSrc) uint32_t src_idx_part_count = pSrc->def->key_def->part_count; if (dest_idx_part_count != src_idx_part_count) return 0; - if (pDest->onError != pSrc->onError) { - return 0; /* Different conflict resolution strategies */ - } struct key_part *src_part = pSrc->def->key_def->parts; struct key_part *dest_part = pDest->def->key_def->parts; for (uint32_t i = 0; i < src_idx_part_count; diff --git a/src/box/sql/main.c b/src/box/sql/main.c index a9a0385d6..69b2fec80 100644 --- a/src/box/sql/main.c +++ b/src/box/sql/main.c @@ -726,10 +726,9 @@ sqlite3_close_v2(sqlite3 * db) * but are "saved" in case the table pages are moved around. */ void -sqlite3RollbackAll(Vdbe * pVdbe, int tripCode) +sqlite3RollbackAll(Vdbe * pVdbe) { sqlite3 *db = pVdbe->db; - (void)tripCode; /* If one has been configured, invoke the rollback-hook callback */ if (db->xRollbackCallback && (!pVdbe->auto_commit)) { @@ -761,9 +760,6 @@ sqlite3ErrName(int rc) case SQLITE_ABORT: zName = "SQLITE_ABORT"; break; - case SQLITE_ABORT_ROLLBACK: - zName = "SQLITE_ABORT_ROLLBACK"; - break; case SQLITE_BUSY: zName = "SQLITE_BUSY"; break; @@ -963,20 +959,9 @@ sqlite3ErrStr(int rc) /* SQL_TARANTOOL_ERROR */ "SQL-/Tarantool error", }; const char *zErr = "unknown error"; - switch (rc) { - case SQLITE_ABORT_ROLLBACK:{ - zErr = "abort due to ROLLBACK"; - break; - } - default:{ - rc &= 0xff; - if (ALWAYS(rc >= 0) && rc < ArraySize(aMsg) - && aMsg[rc] != 0) { - zErr = aMsg[rc]; - } - break; - } - } + rc &= 0xff; + if (ALWAYS(rc >= 0) && rc < ArraySize(aMsg) && aMsg[rc] != 0) + zErr = aMsg[rc]; return zErr; } @@ -999,28 +984,6 @@ sqliteDefaultBusyCallback(void *ptr, /* Database connection */ return 1; } -/* - * Invoke the given busy handler. - * - * This routine is called when an operation failed with a lock. - * If this routine returns non-zero, the lock is retried. If it - * returns 0, the operation aborts with an SQLITE_BUSY error. - */ -int -sqlite3InvokeBusyHandler(BusyHandler * p) -{ - int rc; - if (NEVER(p == 0) || p->xFunc == 0 || p->nBusy < 0) - return 0; - rc = p->xFunc(p->pArg, p->nBusy); - if (rc == 0) { - p->nBusy = -1; - } else { - p->nBusy++; - } - return rc; -} - /* * This routine sets the busy callback for an Sqlite database to the * given callback function with the given argument. diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 66ad84ce2..e0e2a4ea1 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -280,11 +280,11 @@ ccons ::= NULL onconf(R). { sql_column_add_nullable_action(pParse, R); } ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);} -ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I). - {sqlite3AddPrimaryKey(pParse,0,R,I,Z);} -ccons ::= UNIQUE index_onconf(R). {sql_create_index(pParse,0,0,0,R,0, - SORT_ORDER_ASC, false, - SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} +ccons ::= PRIMARY KEY sortorder(Z) autoinc(I). + {sqlite3AddPrimaryKey(pParse,0,I,Z);} +ccons ::= UNIQUE. {sql_create_index(pParse,0,0,0,0, + SORT_ORDER_ASC, false, + SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} ccons ::= CHECK LP expr(X) RP. {sql_add_check_constraint(pParse,&X);} ccons ::= REFERENCES nm(T) eidlist_opt(TA) refargs(R). {sql_create_foreign_key(pParse, NULL, NULL, NULL, &T, TA, false, R);} @@ -334,12 +334,12 @@ conslist ::= tcons. tconscomma ::= COMMA. {pParse->constraintName.n = 0;} tconscomma ::= . tcons ::= CONSTRAINT nm(X). {pParse->constraintName = X;} -tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP onconf(R). - {sqlite3AddPrimaryKey(pParse,X,R,I,0);} -tcons ::= UNIQUE LP sortlist(X) RP index_onconf(R). - {sql_create_index(pParse,0,0,X,R,0, - SORT_ORDER_ASC,false, - SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} +tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP. + {sqlite3AddPrimaryKey(pParse,X,I,0);} +tcons ::= UNIQUE LP sortlist(X) RP. + {sql_create_index(pParse,0,0,X,0, + SORT_ORDER_ASC,false, + SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} tcons ::= CHECK LP expr(E) RP onconf. {sql_add_check_constraint(pParse,&E);} tcons ::= FOREIGN KEY LP eidlist(FA) RP @@ -357,8 +357,6 @@ defer_subclause_opt(A) ::= defer_subclause(A). %type index_onconf {int} %type orconf {int} %type resolvetype {int} -index_onconf(A) ::= . {A = ON_CONFLICT_ACTION_DEFAULT;} -index_onconf(A) ::= ON CONFLICT resolvetype(X). {A = X;} onconf(A) ::= . {A = ON_CONFLICT_ACTION_ABORT;} onconf(A) ::= ON CONFLICT resolvetype(X). {A = X;} orconf(A) ::= . {A = ON_CONFLICT_ACTION_DEFAULT;} @@ -1200,10 +1198,8 @@ paren_exprlist(A) ::= LP exprlist(X) RP. {A = X;} // cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X) ON nm(Y) LP sortlist(Z) RP. { - enum on_conflict_action on_error = - U ? ON_CONFLICT_ACTION_ABORT : ON_CONFLICT_ACTION_NONE; - sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, - on_error, &S, SORT_ORDER_ASC, NE, U); + sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, &S, + SORT_ORDER_ASC, NE, U); } %type uniqueflag {int} diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 0ffc8d548..45dab0be4 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -649,7 +649,6 @@ sqlite3_exec(sqlite3 *, /* An open database */ #define SQLITE_IOERR_GETTEMPPATH (SQLITE_IOERR | (25<<8)) #define SQLITE_IOERR_CONVPATH (SQLITE_IOERR | (26<<8)) #define SQLITE_IOERR_VNODE (SQLITE_IOERR | (27<<8)) -#define SQLITE_ABORT_ROLLBACK (SQLITE_ABORT | (2<<8)) #define SQLITE_CONSTRAINT_CHECK (SQLITE_CONSTRAINT | (1<<8)) #define SQLITE_CONSTRAINT_FOREIGNKEY (SQLITE_CONSTRAINT | (3<<8)) #define SQLITE_CONSTRAINT_FUNCTION (SQLITE_CONSTRAINT | (4<<8)) @@ -675,30 +674,6 @@ enum sql_subtype { SQL_SUBTYPE_MSGPACK = 77, }; -/** - * Structure for internal usage during INSERT/UPDATE - * statements compilation. - */ -struct on_conflict { - /** - * Represents an error action in queries like - * INSERT/UPDATE OR <override_error>, which - * overrides all space constraints error actions. - * That kind of error action is strictly specified by - * user and therefore have highest priority. - */ - enum on_conflict_action override_error; - /** - * Represents an ON CONFLICT action which can be - * optimized and executed without VDBE bytecode, by - * Tarantool facilities. If optimization is not available, - * then the value is ON_CONFLICT_ACTION_NONE, otherwise - * it is ON_CONFLICT_ACTON_IGNORE or - * ON_CONFLICT_ACTION_REPLACE. - */ - enum on_conflict_action optimized_action; -}; - void * sqlite3_user_data(sqlite3_context *); @@ -2009,12 +1984,6 @@ struct Index { Table *pTable; /** The next index associated with the same table. */ Index *pNext; - /** - * Conflict resolution algorithm to employ whenever an - * attempt is made to insert a non-unique element in - * unique index. - */ - u8 onError; /** Index definition. */ struct index_def *def; }; @@ -2904,6 +2873,7 @@ struct Parse { #define OPFLAG_EPHEM 0x01 /* OP_Column: Ephemeral output is ok */ #define OPFLAG_OE_IGNORE 0x200 /* OP_IdxInsert: Ignore flag */ #define OPFLAG_OE_FAIL 0x400 /* OP_IdxInsert: Fail flag */ +#define OPFLAG_OE_ROLLBACK 0x800 /* OP_IdxInsert: Rollback flag. */ #define OPFLAG_LENGTHARG 0x40 /* OP_Column only used for length() */ #define OPFLAG_TYPEOFARG 0x80 /* OP_Column only used for typeof() */ #define OPFLAG_SEEKEQ 0x02 /* OP_Open** cursor uses EQ seek only */ @@ -3452,7 +3422,7 @@ void sql_column_add_nullable_action(struct Parse *parser, enum on_conflict_action nullable_action); -void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order); +void sqlite3AddPrimaryKey(Parse *, ExprList *, int, enum sort_order); /** * Add a new CHECK constraint to the table currently under @@ -3580,8 +3550,6 @@ void sqlite3SrcListDelete(sqlite3 *, SrcList *); * @param token Index name. May be NULL. * @param tbl_name Table to index. Use pParse->pNewTable ifNULL. * @param col_list A list of columns to be indexed. - * @param on_error One of ON_CONFLICT_ACTION_ABORT, _IGNORE, - * _REPLACE, or _NONE. * @param start The CREATE token that begins this statement. * @param sort_order Sort order of primary key when pList==NULL. * @param if_not_exist Omit error if index already exists. @@ -3590,9 +3558,8 @@ void sqlite3SrcListDelete(sqlite3 *, SrcList *); void sql_create_index(struct Parse *parse, struct Token *token, struct SrcList *tbl_name, struct ExprList *col_list, - enum on_conflict_action on_error, struct Token *start, - enum sort_order sort_order, bool if_not_exist, - enum sql_index_type idx_type); + struct Token *start, enum sort_order sort_order, + bool if_not_exist, enum sql_index_type idx_type); /** * This routine will drop an existing named index. This routine @@ -3760,7 +3727,7 @@ Vdbe *sqlite3GetVdbe(Parse *); void sqlite3PrngSaveState(void); void sqlite3PrngRestoreState(void); #endif -void sqlite3RollbackAll(Vdbe *, int); +void sqlite3RollbackAll(Vdbe *); /** * Generate opcodes which start new Tarantool transaction. @@ -3909,26 +3876,88 @@ sql_generate_index_key(struct Parse *parse, struct Index *index, int cursor, void sql_resolve_part_idx_label(struct Parse *parse, int label); -void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int, - int, u8, struct on_conflict *, int, int *, - int *); +/** + * Generate code to do constraint checks prior to an INSERT or + * an UPDATE on the given table. + * + * The @new_tuple_reg is the first register in a range that + * contains the data to be inserted or the data after the update. + * There will be field_count registers in this range. + * The first register in the range will contains the content of + * the first table column, and so forth. + * + * To test NULL, CHECK and statement (except for REPLACE) + * constraints we can avoid opening cursors on secondary indexes. + * However, to implement INSERT OR REPLACE or UPDATE OR REPLACE, + * we should + * + * Constraint + * type Action What Happens + * ---------- ---------- -------------------------------------- + * any ROLLBACK The current transaction is rolled + * back and VDBE stops immediately + * with return code of SQLITE_CONSTRAINT. + * + * any ABORT Back out changes from the current + * command only (do not do a complete + * rollback) then cause VDBE to return + * immediately with SQLITE_CONSTRAINT. + * + * any FAIL VDBE returns immediately with a + * return code of SQLITE_CONSTRAINT. The + * transaction is not rolled back and any + * changes to prior rows are retained. + * + * any IGNORE The attempt in insert or update the + * current row is skipped, without + * throwing an error. Processing + * continues with the next row. + * + * NOT NULL REPLACE The NULL value is replace by the + * default value for that column. If the + * default value is NULL, the action is + * the same as ABORT. + * + * UNIQUE REPLACE The other row that conflicts with the + * row being inserted is removed. + * Triggers are fired, foreign keys + * constraints are checked. + * + * CHECK REPLACE Illegal. Results in an exception. + * + * @param parse_context Current parsing context. + * @param tab The table being inserted or updated. + * @param new_tuple_reg First register in a range holding values + * to insert. + * @param on_conflict On conflict error action of INSERT or + * UPDATE statement (for example INSERT OR REPLACE). + * @param ignore_label Jump to this label on an IGNORE resolution. + * @param upd_cols Columns to be updated with the size of table's + * field count. NULL for INSERT operation. + */ +void 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); /** * This routine generates code to finish the INSERT or UPDATE * operation that was started by a prior call to - * sqlite3GenerateConstraintChecks. + * sqlite3GenerateConstraintChecks. It encodes raw data which + * is held in a range of registers starting from @raw_data_reg + * and length of @tuple_len and inserts this record to space + * using given @cursor_id. + * * @param v Virtual database engine. * @param cursor_id Primary index cursor. - * @param tuple_id Register with data to insert. - * @param on_conflict Structure, which contains override error - * action on failed insert/replaceand and optimized error - * action after generating bytecode for constraints checks. + * @param tuple_reg Register with raw data to insert. + * @param tuple_len Number of registers to hold the tuple. + * @param on_conflict On conflict action. */ void -vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, - struct on_conflict *on_conflict); +vdbe_emit_insertion_completion(struct Vdbe *v, int cursor_id, int raw_data_reg, + uint32_t tuple_len, + enum on_conflict_action on_conflict); -int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *, - int *, u8, u8); void sql_set_multi_write(Parse *, bool); void sqlite3MayAbort(Parse *); @@ -4535,8 +4564,6 @@ void sqlite3Analyze(Parse *, Token *); ssize_t sql_index_tuple_size(struct space *space, struct index *idx); -int sqlite3InvokeBusyHandler(BusyHandler *); - /** * Load the content of the _sql_stat1 and sql_stat4 tables. The * contents of _sql_stat1 are used to populate the tuple_stat1[] 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 @@ -84,20 +84,12 @@ sqlite3Update(Parse * pParse, /* The parser context */ int addrTop = 0; /* VDBE instruction address of the start of the loop */ WhereInfo *pWInfo; /* Information about the WHERE clause */ Vdbe *v; /* The virtual database engine */ - Index *pIdx; /* For looping over indices */ Index *pPk; /* The PRIMARY KEY index */ - int nIdx; /* Number of indices that need updating */ - int iBaseCur; /* Base cursor number */ - int iDataCur; /* Cursor for the canonical data btree */ - int iIdxCur; /* Cursor for the first index */ sqlite3 *db; /* The database structure */ - int *aRegIdx = 0; /* One register assigned to each index to be updated */ int *aXRef = 0; /* aXRef[i] is the index in pChanges->a[] of the * an expression for the i-th column of the table. * aXRef[i]==-1 if the i-th column is not changed. */ - u8 *aToOpen; /* 1 for tables and indices to be opened */ - u8 chngPk; /* PRIMARY KEY changed */ NameContext sNC; /* The name-context to resolve expressions in */ int okOnePass; /* True for one-pass algorithm without the FIFO */ int hasFK; /* True if foreign key processing is required */ @@ -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++); + /* 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; @@ -192,7 +167,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ * of the UPDATE statement. Also find the column index * for each column to be updated in the pChanges array. */ - chngPk = 0; + bool is_pk_modified = false; for (i = 0; i < pChanges->nExpr; i++) { if (sqlite3ResolveExprNames(&sNC, pChanges->a[i].pExpr)) { goto update_cleanup; @@ -201,7 +176,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ if (strcmp(def->fields[j].name, pChanges->a[i].zName) == 0) { if (pPk && table_column_is_in_pk(pTab, j)) { - chngPk = 1; + is_pk_modified = true; } if (aXRef[j] != -1) { sqlite3ErrorMsg(pParse, @@ -220,8 +195,6 @@ sqlite3Update(Parse * pParse, /* The parser context */ goto update_cleanup; } } - assert(chngPk == 0 || chngPk == 1); - /* * The SET expressions are not actually used inside the * WHERE loop. So reset the colUsed mask. @@ -230,35 +203,6 @@ sqlite3Update(Parse * pParse, /* The parser context */ hasFK = fkey_is_required(pTab->def->id, aXRef); - /* There is one entry in the aRegIdx[] array for each index on the table - * being updated. Fill in aRegIdx[] with a register number that will hold - * the key for accessing each index. - * - * FIXME: Be smarter about omitting indexes that use expressions. - */ - for (j = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, j++) { - int reg; - uint32_t part_count = pIdx->def->key_def->part_count; - if (chngPk || hasFK || pIdx == pPk) { - reg = ++pParse->nMem; - pParse->nMem += part_count; - } else { - reg = 0; - for (uint32_t i = 0; i < part_count; i++) { - uint32_t fieldno = - pIdx->def->key_def->parts[i].fieldno; - if (aXRef[fieldno] >= 0) { - reg = ++pParse->nMem; - pParse->nMem += part_count; - break; - } - } - } - if (reg == 0) - aToOpen[j + 1] = 0; - aRegIdx[j] = reg; - } - /* Begin generating code. */ v = sqlite3GetVdbe(pParse); if (v == NULL) @@ -269,7 +213,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* Allocate required registers. */ regOldPk = regNewPk = ++pParse->nMem; - if (chngPk != 0 || trigger != NULL || hasFK != 0) { + if (is_pk_modified || trigger != NULL || hasFK != 0) { regOld = pParse->nMem + 1; pParse->nMem += def->field_count; regNewPk = ++pParse->nMem; @@ -280,10 +224,15 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* If we are trying to update a view, realize that view into * an ephemeral table. */ + uint32_t pk_part_count; if (is_view) { - sql_materialize_view(pParse, def->name, pWhere, iDataCur); + sql_materialize_view(pParse, def->name, pWhere, pk_cursor); /* Number of columns from SELECT plus ID.*/ - nKey = def->field_count + 1; + pk_part_count = nKey = def->field_count + 1; + } else { + vdbe_emit_open_cursor(pParse, pk_cursor, 0, + space_by_id(pTab->def->id)); + pk_part_count = pPk->def->key_def->part_count; } /* Resolve the column names in all the expressions in the @@ -292,49 +241,28 @@ sqlite3Update(Parse * pParse, /* The parser context */ if (sqlite3ResolveExprNames(&sNC, pWhere)) { goto update_cleanup; } - /* Begin the database scan - * The only difference between VIEW and ordinary table is the fact that - * VIEW is held in ephemeral table and doesn't have explicit PK. - * In this case we have to manually load columns in order to make tuple. - */ int iPk; /* First of nPk memory cells holding PRIMARY KEY value */ - /* Number of components of the PRIMARY KEY. */ - uint32_t pk_part_count; int addrOpen; /* Address of the OpenEphemeral instruction */ - if (is_view) { - pk_part_count = nKey; - } else { - assert(pPk != 0); - pk_part_count = pPk->def->key_def->part_count; - } iPk = pParse->nMem + 1; pParse->nMem += pk_part_count; regKey = ++pParse->nMem; iEph = pParse->nTab++; sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); - if (is_view) { - addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, - nKey); - } else { - addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, - pk_part_count); - sql_vdbe_set_p4_key_def(pParse, pPk); - } - + addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, pk_part_count); pWInfo = sqlite3WhereBegin(pParse, pTabList, pWhere, 0, 0, - WHERE_ONEPASS_DESIRED, iIdxCur); + WHERE_ONEPASS_DESIRED, pk_cursor); if (pWInfo == 0) goto update_cleanup; okOnePass = sqlite3WhereOkOnePass(pWInfo, aiCurOnePass); if (is_view) { for (i = 0; i < (int) pk_part_count; i++) { - sqlite3VdbeAddOp3(v, OP_Column, iDataCur, i, iPk + i); + sqlite3VdbeAddOp3(v, OP_Column, pk_cursor, i, iPk + i); } } else { for (i = 0; i < (int) pk_part_count; i++) { - sqlite3ExprCodeGetColumnOfTable(v, def, iDataCur, + sqlite3ExprCodeGetColumnOfTable(v, def, pk_cursor, pPk->def->key_def-> parts[i].fieldno, iPk + i); @@ -366,57 +294,22 @@ sqlite3Update(Parse * pParse, /* The parser context */ regRowCount = ++pParse->nMem; sqlite3VdbeAddOp2(v, OP_Integer, 0, regRowCount); } - labelBreak = sqlite3VdbeMakeLabel(v); - if (!is_view) { - /* - * Open every index that needs updating. Note that if any - * index could potentially invoke a REPLACE conflict resolution - * action, then we need to open all indices because we might need - * to be deleting some records. - */ - if (on_error == ON_CONFLICT_ACTION_REPLACE) { - memset(aToOpen, 1, nIdx + 1); - } else { - for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) { - if (pIdx->onError == ON_CONFLICT_ACTION_REPLACE) { - memset(aToOpen, 1, nIdx + 1); - break; - } - } - } - if (okOnePass) { - if (aiCurOnePass[0] >= 0) - aToOpen[aiCurOnePass[0] - iBaseCur] = 0; - if (aiCurOnePass[1] >= 0) - aToOpen[aiCurOnePass[1] - iBaseCur] = 0; - } - sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0, - iBaseCur, aToOpen, 0, 0, - on_error, 1); - } - /* Top of the update loop */ if (okOnePass) { labelContinue = labelBreak; - sqlite3VdbeAddOp2(v, OP_IsNull, regKey, - labelBreak); - if (aToOpen[iDataCur - iBaseCur] && !is_view) { + sqlite3VdbeAddOp2(v, OP_IsNull, regKey, labelBreak); + if (!is_view) { assert(pPk); - sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, - labelBreak, regKey, nKey); - VdbeCoverageNeverTaken(v); + sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, + labelBreak, regKey, pk_part_count); } - VdbeCoverageIf(v, pPk == 0); - VdbeCoverageIf(v, pPk != 0); } else { labelContinue = sqlite3VdbeMakeLabel(v); sqlite3VdbeAddOp2(v, OP_Rewind, iEph, labelBreak); - VdbeCoverage(v); addrTop = sqlite3VdbeAddOp2(v, OP_RowData, iEph, regKey); - sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelContinue, + sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, labelContinue, regKey, 0); - VdbeCoverage(v); } /* If the record number will change, set register regNewPk to @@ -424,14 +317,13 @@ sqlite3Update(Parse * pParse, /* The parser context */ * then regNewPk is the same register as regOldPk, which is * already populated. */ - assert(chngPk != 0 || trigger != NULL || hasFK != 0 || + assert(is_pk_modified || trigger != NULL || hasFK != 0 || regOldPk == regNewPk); - /* Compute the old pre-UPDATE content of the row being changed, if that * information is needed */ - if (chngPk != 0 || hasFK != 0 || trigger != NULL) { + if (is_pk_modified || hasFK != 0 || trigger != NULL) { struct space *space = space_by_id(pTab->def->id); assert(space != NULL); u32 oldmask = hasFK ? space->fkey_mask : 0; @@ -444,7 +336,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ || table_column_is_in_pk(pTab, i)) { testcase(oldmask != 0xffffffff && i == 31); sqlite3ExprCodeGetColumnOfTable(v, def, - iDataCur, i, + pk_cursor, i, regOld + i); } else { sqlite3VdbeAddOp2(v, OP_Null, 0, regOld + i); @@ -483,8 +375,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ testcase(i == 31); testcase(i == 32); sqlite3ExprCodeGetColumnToReg(pParse, def, i, - iDataCur, - regNew + i); + pk_cursor, regNew + i); } else { sqlite3VdbeAddOp2(v, OP_Null, 0, regNew + i); } @@ -506,13 +397,13 @@ sqlite3Update(Parse * pParse, /* The parser context */ * documentation. */ if (!is_view) { - sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, + sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, labelContinue, regKey, nKey); - VdbeCoverage(v); } else { - sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, - labelContinue, regKey - nKey, nKey); - VdbeCoverage(v); + sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, + labelContinue, + regKey - pk_part_count, + pk_part_count); } /* If it did not delete it, the row-trigger may still have modified @@ -523,62 +414,39 @@ sqlite3Update(Parse * pParse, /* The parser context */ for (i = 0; i < (int)def->field_count; i++) { if (aXRef[i] < 0) { sqlite3ExprCodeGetColumnOfTable(v, def, - iDataCur, i, + pk_cursor, i, regNew + i); } } } if (!is_view) { - int addr1 = 0; /* Address of jump instruction */ - int bReplace = 0; /* True if REPLACE conflict resolution might happen */ - - struct on_conflict on_conflict; - on_conflict.override_error = on_error; - on_conflict.optimized_action = ON_CONFLICT_ACTION_NONE; - - /* Do constraint checks. */ assert(regOldPk > 0); - sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur, - iIdxCur, regNewPk, - regOldPk, chngPk, &on_conflict, - labelContinue, &bReplace, - aXRef); - + vdbe_emit_constraint_checks(pParse, pTab, regNewPk + 1, + on_error, labelContinue, aXRef); /* Do FK constraint checks. */ if (hasFK) fkey_emit_check(pParse, pTab, regOldPk, 0, aXRef); - - /* Delete the index entries associated with the current record. */ - if (bReplace || chngPk) { - addr1 = - sqlite3VdbeAddOp4Int(v, OP_NotFound, - iDataCur, 0, regKey, - nKey); - VdbeCoverageNeverTaken(v); - } - - /* If changing the PK value, or if there are foreign key constraints - * to process, delete the old record. + int addr1 = 0; + /* + * Delete the index entries associated with the + * current record. It can be already be removed + * by trigger or REPLACE conflict action. */ + addr1 = sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, 0, + regKey, nKey); assert(regNew == regNewPk + 1); - if (hasFK || chngPk || pPk != 0) { - sqlite3VdbeAddOp2(v, OP_Delete, iDataCur, 0); - } - if (bReplace || chngPk) { - sqlite3VdbeJumpHere(v, addr1); - } - + sqlite3VdbeAddOp2(v, OP_Delete, pk_cursor, 0); + sqlite3VdbeJumpHere(v, addr1); if (hasFK) fkey_emit_check(pParse, pTab, 0, regNewPk, aXRef); - - /* Insert the new index entries and the new record. */ - vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], - &on_conflict); - - /* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to - * handle rows (possibly in other tables) that refer via a foreign key - * to the row just updated. + vdbe_emit_insertion_completion(v, pk_cursor, regNew, + pTab->def->field_count, + on_error); + /* + * Do any ON CASCADE, SET NULL or SET DEFAULT + * operations required to handle rows that refer + * via a foreign key to the row just updated. */ if (hasFK) fkey_emit_actions(pParse, pTab, regOldPk, aXRef); @@ -617,7 +485,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } update_cleanup: - sqlite3DbFree(db, aXRef); /* Also frees aRegIdx[] and aToOpen[] */ + sqlite3DbFree(db, aXRef); sqlite3SrcListDelete(db, pTabList); sql_expr_list_delete(db, pChanges); sql_expr_delete(db, pWhere, false); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index e1eaf91a4..46beae24b 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4227,7 +4227,9 @@ case OP_Next: /* jump */ * @param P2 Index of a register with MessagePack data to insert. * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE * accounts the change in a case of successful insertion in - * nChange counter. + * nChange counter. If P5 contains OPFLAG_OE_IGNORE, then + * we are processing INSERT OR INGORE statement. Thus, in + * case of conflict we don't raise an error. */ /* Opcode: IdxReplace P1 P2 * * P5 * Synopsis: key=r[P2] @@ -4291,10 +4293,9 @@ case OP_IdxInsert: { /* in2 */ } } else if (pOp->p5 & OPFLAG_OE_FAIL) { p->errorAction = ON_CONFLICT_ACTION_FAIL; + } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) { + p->errorAction = ON_CONFLICT_ACTION_ROLLBACK; } - - assert(p->errorAction == ON_CONFLICT_ACTION_ABORT || - p->errorAction == ON_CONFLICT_ACTION_FAIL); if (rc) goto abort_due_to_error; break; } diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 242a6448e..5ab519683 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -2381,8 +2381,7 @@ sqlite3VdbeHalt(Vdbe * p) */ box_txn_rollback(); closeCursorsAndFree(p); - sqlite3RollbackAll(p, - SQLITE_ABORT_ROLLBACK); + sqlite3RollbackAll(p); sqlite3CloseSavepoints(p); p->nChange = 0; } @@ -2433,7 +2432,7 @@ sqlite3VdbeHalt(Vdbe * p) p->rc = rc; box_txn_rollback(); closeCursorsAndFree(p); - sqlite3RollbackAll(p, SQLITE_OK); + sqlite3RollbackAll(p); p->nChange = 0; } else { sqlite3CommitInternalChanges(); @@ -2441,7 +2440,7 @@ sqlite3VdbeHalt(Vdbe * p) } else { box_txn_rollback(); closeCursorsAndFree(p); - sqlite3RollbackAll(p, SQLITE_OK); + sqlite3RollbackAll(p); p->nChange = 0; } p->anonymous_savepoint = NULL; @@ -2453,7 +2452,7 @@ sqlite3VdbeHalt(Vdbe * p) } else { box_txn_rollback(); closeCursorsAndFree(p); - sqlite3RollbackAll(p, SQLITE_ABORT_ROLLBACK); + sqlite3RollbackAll(p); sqlite3CloseSavepoints(p); p->nChange = 0; } @@ -2476,7 +2475,7 @@ sqlite3VdbeHalt(Vdbe * p) p->zErrMsg = 0; } closeCursorsAndFree(p); - sqlite3RollbackAll(p, SQLITE_ABORT_ROLLBACK); + sqlite3RollbackAll(p); sqlite3CloseSavepoints(p); p->nChange = 0; } diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 73fe070fd..1c205b664 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -2858,7 +2858,6 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */ */ Index *pFirst; /* First of real indices on the table */ memset(&fake_index, 0, sizeof(Index)); - fake_index.onError = ON_CONFLICT_ACTION_REPLACE; fake_index.pTable = pTab; struct key_def *key_def = key_def_new(1); @@ -4666,10 +4665,6 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */ struct space *space = space_cache_find(pTabItem->pTab->def->id); int iIndexCur; int op = OP_OpenRead; - /* iAuxArg is always set if to a positive value if ONEPASS is possible */ - assert(iAuxArg != 0 - || (pWInfo-> - wctrlFlags & WHERE_ONEPASS_DESIRED) == 0); /* Check if index is primary. Either of * points should be true: * 1. struct Index is non-NULL and is diff --git a/test/sql-tap/conflict3.test.lua b/test/sql-tap/conflict3.test.lua deleted file mode 100755 index 9b555501e..000000000 --- a/test/sql-tap/conflict3.test.lua +++ /dev/null @@ -1,402 +0,0 @@ -#!/usr/bin/env tarantool -test = require("sqltester") -test:plan(29) - ---!./tcltestrunner.lua --- 2013-11-05 --- --- The author disclaims copyright to this source code. In place of --- a legal notice, here is a blessing: --- --- May you do good and not evil. --- May you find forgiveness for yourself and forgive others. --- May you share freely, never taking more than you give. --- -------------------------------------------------------------------------- --- This file implements regression tests for SQLite library. --- --- This file implements tests for the conflict resolution extension --- to SQLite. --- --- This file focuses on making sure that combinations of REPLACE, --- IGNORE, and FAIL conflict resolution play well together. --- --- ["set","testdir",[["file","dirname",["argv0"]]]] --- ["source",[["testdir"],"\/tester.tcl"]] - - --- MUST_WORK_TEST -test:do_execsql_test( - "conflict-1.1", - [[ - CREATE TABLE t1( - a INTEGER PRIMARY KEY ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-1.1> - 1, 2, 3, 2, 3, 4 - -- </conflict-1.1> - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-1.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-1.2> - 1, 2, 3, 2, 3, 4 - -- </conflict-1.2> - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-1.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- <conflict-1.3> - 1, "UNIQUE constraint failed: T1.C" - -- </conflict-1.3> - }) - -test:do_execsql_test( - "conflict-1.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-1.4> - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- </conflict-1.4> - }) - --- Replete the tests above, but this time on a table non-INTEGER primary key. --- -test:do_execsql_test( - "conflict-2.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - a INT PRIMARY KEY ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-2.1> - 1, 2, 3, 2, 3, 4 - -- </conflict-2.1> - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-2.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-2.2> - 1, 2, 3, 2, 3, 4 - -- </conflict-2.2> - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-2.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- <conflict-2.3> - 1, "UNIQUE constraint failed: T1.C" - -- </conflict-2.3> - }) - -test:do_execsql_test( - "conflict-2.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-2.4> - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- </conflict-2.4> - }) - --- Replete again --- -test:do_execsql_test( - "conflict-3.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - a INT PRIMARY KEY ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-3.1> - 1, 2, 3, 2, 3, 4 - -- </conflict-3.1> - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-3.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-3.2> - 1, 2, 3, 2, 3, 4 - -- </conflict-3.2> - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-3.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- <conflict-3.3> - 1, "UNIQUE constraint failed: T1.C" - -- </conflict-3.3> - }) - -test:do_execsql_test( - "conflict-3.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-3.4> - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- </conflict-3.4> - }) - --- Arrange the table rows in a different order and repeat. --- -test:do_execsql_test( - "conflict-4.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - b UNIQUE ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL, - a INT PRIMARY KEY ON CONFLICT REPLACE - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-4.1> - 1, 2, 3, 2, 3, 4 - -- </conflict-4.1> - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-4.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-4.2> - 1, 2, 3, 2, 3, 4 - -- </conflict-4.2> - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-4.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- <conflict-4.3> - 1, "UNIQUE constraint failed: T1.C" - -- </conflict-4.3> - }) - -test:do_execsql_test( - "conflict-4.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-4.4> - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- </conflict-4.4> - }) - --- Arrange the table rows in a different order and repeat. --- -test:do_execsql_test( - "conflict-5.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - b UNIQUE ON CONFLICT IGNORE, - a INT PRIMARY KEY ON CONFLICT REPLACE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-5.1> - 1, 2, 3, 2, 3, 4 - -- </conflict-5.1> - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-5.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-5.2> - 1, 2, 3, 2, 3, 4 - -- </conflict-5.2> - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-5.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- <conflict-5.3> - 1, "UNIQUE constraint failed: T1.C" - -- </conflict-5.3> - }) - -test:do_execsql_test( - "conflict-5.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-5.4> - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- </conflict-5.4> - }) - --- Arrange the table rows in a different order and repeat. --- -test:do_execsql_test( - "conflict-6.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - c UNIQUE ON CONFLICT FAIL, - a INT PRIMARY KEY ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT IGNORE - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-6.1> - 1, 2, 3, 2, 3, 4 - -- </conflict-6.1> - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-6.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-6.2> - 1, 2, 3, 2, 3, 4 - -- </conflict-6.2> - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-6.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- <conflict-6.3> - 1, "UNIQUE constraint failed: T1.C" - -- </conflict-6.3> - }) - -test:do_execsql_test( - "conflict-6.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- <conflict-6.4> - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- </conflict-6.4> - }) - -test:do_catchsql_test( - "conflict-7.1", - [[ - CREATE TABLE t3(a PRIMARY KEY ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT REPLACE); - ]], { - 1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" - }) - -test:do_catchsql_test( - "conflict-7.2", - [[ - CREATE TABLE t3(a PRIMARY KEY, - b UNIQUE ON CONFLICT REPLACE); - ]], { - 1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" - }) - -test:do_catchsql_test( - "conflict-7.3", - [[ - CREATE TABLE t3(a PRIMARY KEY, - b UNIQUE ON CONFLICT REPLACE, - c UNIQUE ON CONFLICT REPLACE); - ]], { - 1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" - }) - -test:do_catchsql_test( - "conflict-7.4", - [[ - CREATE TABLE t3(a PRIMARY KEY, - b NOT NULL ON CONFLICT REPLACE DEFAULT 1488); - ]], { - 1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" - }) - -test:do_catchsql_test( - "conflict-7.5", - [[ - CREATE TABLE t3(a PRIMARY KEY ON CONFLICT REPLACE, - b NOT NULL ON CONFLICT REPLACE DEFAULT 1488); - ]], { - 1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" - }) - -test:finish_test() diff --git a/test/sql-tap/gh-2931-savepoints.test.lua b/test/sql-tap/gh-2931-savepoints.test.lua index 8491a3b69..2ce41459e 100755 --- a/test/sql-tap/gh-2931-savepoints.test.lua +++ b/test/sql-tap/gh-2931-savepoints.test.lua @@ -86,7 +86,7 @@ local testcases = { {0,{1,2,10,11,1,2,4,10,11}}}, {"16", [[insert or rollback into t1 values(4);]], - {1,"UNIQUE constraint failed: T2.A"}}, + {1,"Duplicate key exists in unique index 'pk_unnamed_T2_1' in space 'T2'"}}, {"17", -- should work as transaction is rolled back [[insert or rollback into t1 values(4); select * from t1 union all select * from t2;]], diff --git a/test/sql-tap/gh2140-trans.test.lua b/test/sql-tap/gh2140-trans.test.lua index 1ec162057..3c6f9042f 100755 --- a/test/sql-tap/gh2140-trans.test.lua +++ b/test/sql-tap/gh2140-trans.test.lua @@ -28,39 +28,33 @@ test:do_execsql_test('rollback1_check', {1, 1, 2, 2}) for _, verb in ipairs({'ROLLBACK', 'ABORT'}) do - box.sql.execute('DELETE FROM t2') - if verb == 'ROLLBACK' then - answer = 'UNIQUE constraint failed: T1.S1' - else - answer = "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'" - end - test:do_catchsql_test('insert1_'..verb, - [[START TRANSACTION; - INSERT INTO t2 VALUES (2, 2); - INSERT OR ]]..verb..[[ INTO t1 VALUES (1,1); - ]], - {1, answer}) + box.sql.execute('DELETE FROM t2') + answer = "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'" + test:do_catchsql_test('insert1_'..verb, + [[START TRANSACTION; + INSERT INTO t2 VALUES (2, 2); + INSERT OR ]]..verb..[[ INTO t1 VALUES (1,1); + ]], + {1, answer}) - local expect = {} - if verb == 'ABORT' then - box.sql.execute('COMMIT') - expect = {2, 2} - end - test:do_execsql_test('insert1_'..verb..'_check', - 'SELECT * FROM t2', - expect) + local expect = {} + if verb == 'ABORT' then + box.sql.execute('COMMIT') + expect = {2, 2} + end + test:do_execsql_test('insert1_'..verb..'_check', + 'SELECT * FROM t2', expect) - box.sql.execute('DELETE FROM t2') - test:do_catchsql_test('update1_'..verb, - [[START TRANSACTION; - INSERT INTO t2 VALUES (2, 2); - UPDATE OR ]]..verb..[[ t1 SET s1 = 1 WHERE s1 = 2; - ]], - {1, answer}) + box.sql.execute('DELETE FROM t2') + test:do_catchsql_test('update1_'..verb, + [[START TRANSACTION; + INSERT INTO t2 VALUES (2, 2); + UPDATE OR ]]..verb..[[ t1 SET s1 = 1 WHERE s1 = 2; + ]], + {1, answer}) - test:do_execsql_test('update1_'..verb..'check', - 'SELECT * FROM t2', - expect) + test:do_execsql_test('update1_'..verb..'check', + 'SELECT * FROM t2', expect) end box.sql.execute('COMMIT') diff --git a/test/sql-tap/gh2964-abort.test.lua b/test/sql-tap/gh2964-abort.test.lua index a596b0110..d4fcebc1b 100755 --- a/test/sql-tap/gh2964-abort.test.lua +++ b/test/sql-tap/gh2964-abort.test.lua @@ -12,17 +12,16 @@ test:do_catchsql_test( test_prefix.."1.0.2", "CREATE TABLE t2 (a int primary key);") -local insert_err = {1, "UNIQUE constraint failed: T2.A"} -local insert_err_PK = {1, "Duplicate key exists in unique index 'pk_unnamed_T2_1' in space 'T2'"} +local insert_err = {1, "Duplicate key exists in unique index 'pk_unnamed_T2_1' in space 'T2'"} local data = { --id|TRIG TYPE|INSERT TYPE|insert error|commit error| result - {1, "AFTER", "or abort", insert_err_PK, {0}, {1,1,2}}, + {1, "AFTER", "or abort", insert_err, {0}, {1,1,2}}, {2, "AFTER", "or rollback",insert_err, {1, "/no transaction is active/"}, {}}, - {3, "AFTER", "or fail", insert_err_PK, {0}, {1,2,1,2}}, + {3, "AFTER", "or fail", insert_err, {0}, {1,2,1,2}}, {4, "AFTER", "or ignore", {0} , {0}, {1,2,1,2}}, - {5, "BEFORE","or abort", insert_err_PK, {0}, {1,1,2}}, + {5, "BEFORE","or abort", insert_err, {0}, {1,1,2}}, {6, "BEFORE","or rollback",insert_err, {1, "/no transaction is active/"}, {}}, - {7, "BEFORE","or fail", insert_err_PK, {0}, {1,1,2}}, + {7, "BEFORE","or fail", insert_err, {0}, {1,1,2}}, {8, "BEFORE","or ignore", {0} , {0}, {1,2,1,2}} } diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua index 731ecf0af..fdce2683c 100755 --- a/test/sql-tap/index1.test.lua +++ b/test/sql-tap/index1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(78) +test:plan(70) --!./tcltestrunner.lua -- 2001 September 15 @@ -1048,115 +1048,6 @@ test:do_execsql_test( ]], { }) - - -- These tests ensure that if multiple table definition constraints are - -- implemented by a single indice, the correct ON CONFLICT policy applies. - test:do_execsql_test( - "index-19.1", - [[ - CREATE TABLE t7(a UNIQUE PRIMARY KEY); - CREATE TABLE t8(a UNIQUE PRIMARY KEY ON CONFLICT ROLLBACK); - INSERT INTO t7 VALUES(1); - INSERT INTO t8 VALUES(1); - ]], { - -- <index-19.1> - - -- </index-19.1> - }) - - test:do_catchsql_test( - "index-19.2", - [[ - START TRANSACTION; - INSERT INTO t7 VALUES(1); - ]], { - -- <index-19.2> - 1, "Duplicate key exists in unique index 'unique_unnamed_T7_1' in space 'T7'" - -- </index-19.2> - }) - - test:do_catchsql_test( - "index-19.3", - [[ - START TRANSACTION; - ]], { - -- <index-19.3> - 1, "Operation is not permitted when there is an active transaction " - -- </index-19.3> - }) - - test:do_catchsql_test( - "index-19.4", - [[ - INSERT INTO t8 VALUES(1); - ]], { - -- <index-19.4> - 1, "UNIQUE constraint failed: T8.A" - -- </index-19.4> - }) - - test:do_catchsql_test( - "index-19.5", - [[ - START TRANSACTION; - COMMIT; - ]], { - -- <index-19.5> - 0 - -- </index-19.5> - }) - - test:do_catchsql_test( - "index-19.6", - [[ - DROP TABLE t7; - DROP TABLE t8; - CREATE TABLE t7( - a PRIMARY KEY ON CONFLICT FAIL, - UNIQUE(a) ON CONFLICT IGNORE - ); - ]], { - -- <index-19.6> - 1, "conflicting ON CONFLICT clauses specified" - -- </index-19.6> - }) - --- MUST_WORK_TEST -if (0 > 0) then -test:do_execsql_test( - "index-19.7", - [[ - REINDEX - ]], { - -- <index-19.7> - - -- </index-19.7> - }) -end - --- integrity_check index-19.8 --- Drop index with a quoted name. Ticket #695. -test:do_execsql_test( - "index-20.1", - [[ - CREATE INDEX "t6i2" ON t6(c); - DROP INDEX "t6i2" ON t6; - ]], { - -- <index-20.1> - - -- </index-20.1> - }) - -test:do_execsql_test( - "index-20.2", - [[ - DROP INDEX t6i1 ON t6; - ]], { - -- <index-20.2> - - -- </index-20.2> - }) - -- Try to create a TEMP index on a non-TEMP table. */ -- test:do_catchsql_test( diff --git a/test/sql-tap/null.test.lua b/test/sql-tap/null.test.lua index 65d57207e..5e1f8666e 100755 --- a/test/sql-tap/null.test.lua +++ b/test/sql-tap/null.test.lua @@ -316,11 +316,10 @@ test:do_catchsql_test( test:do_execsql_test( "null-7.1", [[ - create table t2(a primary key, b unique on conflict ignore); + create table t2(a primary key, b unique); insert into t2 values(1,1); insert into t2 values(2,null); insert into t2 values(3,null); - insert into t2 values(4,1); select a from t2 order by a; ]], { -- <null-7.1> @@ -331,11 +330,10 @@ test:do_execsql_test( test:do_execsql_test( "null-7.2", [[ - create table t3(a primary key, b, c, unique(b,c) on conflict ignore); + create table t3(a primary key, b, c, unique(b,c)); insert into t3 values(1,1,1); insert into t3 values(2,null,1); insert into t3 values(3,null,1); - insert into t3 values(4,1,1); select a from t3 order by a; ]], { -- <null-7.2> diff --git a/test/sql-tap/tkt-4a03edc4c8.test.lua b/test/sql-tap/tkt-4a03edc4c8.test.lua index cbee2b459..ea42d78c9 100755 --- a/test/sql-tap/tkt-4a03edc4c8.test.lua +++ b/test/sql-tap/tkt-4a03edc4c8.test.lua @@ -25,8 +25,8 @@ test:do_test( function() test:execsql [[ CREATE TABLE t1( - a INTEGER PRIMARY KEY ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT FAIL + a INTEGER PRIMARY KEY, + b UNIQUE ); INSERT INTO t1 VALUES(1, 1); INSERT INTO t1 VALUES(2, 2); @@ -38,7 +38,7 @@ test:do_test( ]] end, { -- <tkt-4a03ed-1.1> - 1, "UNIQUE constraint failed: T1.B" + 1, "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'" -- </tkt-4a03ed-1.1> }) diff --git a/test/sql-tap/triggerC.test.lua b/test/sql-tap/triggerC.test.lua index d1fc82842..06e6e5bd2 100755 --- a/test/sql-tap/triggerC.test.lua +++ b/test/sql-tap/triggerC.test.lua @@ -242,7 +242,7 @@ test:do_catchsql_test( UPDATE OR ROLLBACK t1 SET a=100; ]], { -- <triggerC-1.15> - 1, "UNIQUE constraint failed: T1.A" + 1, "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'" -- </triggerC-1.15> }) diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result index eed06d582..4b5d2cd7e 100644 --- a/test/sql/on-conflict.result +++ b/test/sql/on-conflict.result @@ -1,121 +1,159 @@ test_run = require('test_run').new() --- ... +--- +... +--- +... engine = test_run:get_cfg('engine') --- ... +--- +... +--- +... box.sql.execute('pragma sql_default_engine=\''..engine..'\'') --- ... --- Create space +--- +... +--- +... +-- Check that original SQLite ON CONFLICT clause is really +-- disabled. +-- box.sql.execute("CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT ABORT)") --- +- error: keyword "ON" is reserved ... box.sql.execute("CREATE TABLE q (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT FAIL)") --- +- error: keyword "ON" is reserved ... box.sql.execute("CREATE TABLE p (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT IGNORE)") --- +- error: keyword "ON" is reserved +... +box.sql.execute("CREATE TABLE g (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT REPLACE)") +--- +- error: keyword "ON" is reserved ... box.sql.execute("CREATE TABLE e (id INTEGER PRIMARY KEY ON CONFLICT REPLACE, v INTEGER)") --- +- error: keyword "ON" is reserved ... --- Insert values and select them -box.sql.execute("INSERT INTO t values (1, 1), (2, 2), (3, 1)") +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)") --- -- error: Duplicate key exists in unique index 'unique_unnamed_T_2' in space 'T' +- error: keyword "ON" is reserved ... -box.sql.execute("SELECT * FROM t") +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)") --- -- [] +- error: keyword "ON" is reserved ... -box.sql.execute("INSERT INTO q values (1, 1), (2, 2), (3, 1)") +-- CHECK constraint is illegal with REPLACE option. +-- +box.sql.execute("CREATE TABLE t (id INTEGER PRIMARY KEY, a CHECK (a > 5) ON CONFLICT REPLACE);") --- -- error: 'UNIQUE constraint failed: Q.V' +- error: keyword "ON" is reserved ... -box.sql.execute("SELECT * FROM q") +-- +-- gh-3473: Primary key can't be declared with NULL. +-- +box.sql.execute("CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);") --- -- - [1, 1] - - [2, 2] +- error: Primary index of the space 'TE17' can not contain nullable parts ... -box.sql.execute("INSERT INTO p values (1, 1), (2, 2), (3, 1), (4, 5)") +box.sql.execute("CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);") --- +- error: Primary index of the space 'TE17' can not contain nullable parts ... -box.sql.execute("SELECT * FROM p") +box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);") --- -- - [1, 1] - - [2, 2] - - [4, 5] +- error: 'SQL error: NULL declaration for column ''B'' of table ''TEST'' has been + already set to ''none''' ... -box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (1, 3)") +box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") --- +- error: Primary index of the space 'TEST' can not contain nullable parts ... -box.sql.execute("SELECT * FROM e") +-- Several NOT NULL REPLACE constraints work +-- +box.sql.execute("CREATE TABLE a (id INT PRIMARY KEY, a NOT NULL ON CONFLICT REPLACE DEFAULT 1, b NOT NULL ON CONFLICT REPLACE DEFAULT 2);") --- -- - [1, 3] - - [2, 2] ... -box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)") +box.sql.execute("INSERT INTO a VALUES(1, NULL, NULL);") --- ... -box.sql.execute("INSERT INTO t1 VALUES (9)") +box.sql.execute("INSERT INTO a VALUES(2, NULL, NULL);") --- ... -box.sql.execute("INSERT INTO t1 VALUES (9)") +box.sql.execute("SELECT * FROM a;") --- +- - [1, 1, 2] + - [2, 1, 2] ... -box.sql.execute("SELECT * FROM t1") +box.sql.execute("DROP TABLE a;") --- -- - [9] ... -box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)") +-- gh-3566: UPDATE OR IGNORE causes deletion of old entry. +-- +box.sql.execute("CREATE TABLE tj (s1 INT PRIMARY KEY, s2 INT);") --- ... -box.sql.execute("INSERT INTO t2 VALUES (9)") +box.sql.execute("INSERT INTO tj VALUES (1, 2), (2, 3);") --- ... -box.sql.execute("INSERT INTO t2 VALUES (9)") +box.sql.execute("CREATE UNIQUE INDEX i ON tj (s2);") --- ... -box.sql.execute("SELECT * FROM t2") +box.sql.execute("UPDATE OR IGNORE tj SET s1 = s1 + 1;") --- -- - [9] ... -box.sql.execute('DROP TABLE t') +box.sql.execute("SELECT * FROM tj;") --- +- - [1, 2] + - [3, 3] ... -box.sql.execute('DROP TABLE q') +box.sql.execute("UPDATE OR IGNORE tj SET s2 = s2 + 1;") --- ... -box.sql.execute('DROP TABLE p') +box.sql.execute("SELECT * FROM tj;") --- +- - [1, 2] + - [3, 4] ... -box.sql.execute('DROP TABLE e') +-- gh-3565: INSERT OR REPLACE causes assertion fault. +-- +box.sql.execute("DROP TABLE tj;") --- ... -box.sql.execute('DROP TABLE t1') +box.sql.execute("CREATE TABLE tj (s1 INT PRIMARY KEY, s2 INT);") --- ... -box.sql.execute('DROP TABLE t2') +box.sql.execute("INSERT INTO tj VALUES (1, 2),(2, 3);") --- ... --- --- gh-3473: Primary key can't be declared with NULL. --- -box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);') +box.sql.execute("CREATE UNIQUE INDEX i ON tj (s2);") --- -- error: Primary index of the space 'TE17' can not contain nullable parts ... -box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);') +box.sql.execute("REPLACE INTO tj VALUES (1, 3);") --- -- error: Primary index of the space 'TE17' can not contain nullable parts ... -box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);") +box.sql.execute("SELECT * FROM tj;") --- -- error: 'SQL error: NULL declaration for column ''B'' of table ''TEST'' has been - already set to ''none''' +- - [1, 3] ... -box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") +box.sql.execute("INSERT INTO tj VALUES (2, 4), (3, 5);") +--- +... +box.sql.execute("UPDATE OR REPLACE tj SET s2 = s2 + 1;") +--- +... +box.sql.execute("SELECT * FROM tj;") +--- +- - [1, 4] + - [3, 6] +... +box.sql.execute("DROP TABLE tj;") --- -- error: Primary index of the space 'TEST' can not contain nullable parts ... diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua index 347245a8c..ab73249ef 100644 --- a/test/sql/on-conflict.test.lua +++ b/test/sql/on-conflict.test.lua @@ -1,48 +1,63 @@ test_run = require('test_run').new() +--- +... engine = test_run:get_cfg('engine') +--- +... box.sql.execute('pragma sql_default_engine=\''..engine..'\'') - --- Create space +--- +... +-- Check that original SQLite ON CONFLICT clause is really +-- disabled. +-- box.sql.execute("CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT ABORT)") box.sql.execute("CREATE TABLE q (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT FAIL)") box.sql.execute("CREATE TABLE p (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT IGNORE)") +box.sql.execute("CREATE TABLE g (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT REPLACE)") box.sql.execute("CREATE TABLE e (id INTEGER PRIMARY KEY ON CONFLICT REPLACE, v INTEGER)") - --- Insert values and select them -box.sql.execute("INSERT INTO t values (1, 1), (2, 2), (3, 1)") -box.sql.execute("SELECT * FROM t") - -box.sql.execute("INSERT INTO q values (1, 1), (2, 2), (3, 1)") -box.sql.execute("SELECT * FROM q") - -box.sql.execute("INSERT INTO p values (1, 1), (2, 2), (3, 1), (4, 5)") -box.sql.execute("SELECT * FROM p") - -box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (1, 3)") -box.sql.execute("SELECT * FROM e") - box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)") -box.sql.execute("INSERT INTO t1 VALUES (9)") -box.sql.execute("INSERT INTO t1 VALUES (9)") -box.sql.execute("SELECT * FROM t1") - box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)") -box.sql.execute("INSERT INTO t2 VALUES (9)") -box.sql.execute("INSERT INTO t2 VALUES (9)") -box.sql.execute("SELECT * FROM t2") - -box.sql.execute('DROP TABLE t') -box.sql.execute('DROP TABLE q') -box.sql.execute('DROP TABLE p') -box.sql.execute('DROP TABLE e') -box.sql.execute('DROP TABLE t1') -box.sql.execute('DROP TABLE t2') +-- CHECK constraint is illegal with REPLACE option. +-- +box.sql.execute("CREATE TABLE t (id INTEGER PRIMARY KEY, a CHECK (a > 5) ON CONFLICT REPLACE);") -- -- gh-3473: Primary key can't be declared with NULL. -- -box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);') -box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);') +box.sql.execute("CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);") +box.sql.execute("CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);") box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);") box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") + +-- Several NOT NULL REPLACE constraints work +-- +box.sql.execute("CREATE TABLE a (id INT PRIMARY KEY, a NOT NULL ON CONFLICT REPLACE DEFAULT 1, b NOT NULL ON CONFLICT REPLACE DEFAULT 2);") +box.sql.execute("INSERT INTO a VALUES(1, NULL, NULL);") +box.sql.execute("INSERT INTO a VALUES(2, NULL, NULL);") +box.sql.execute("SELECT * FROM a;") +box.sql.execute("DROP TABLE a;") + +-- gh-3566: UPDATE OR IGNORE causes deletion of old entry. +-- +box.sql.execute("CREATE TABLE tj (s1 INT PRIMARY KEY, s2 INT);") +box.sql.execute("INSERT INTO tj VALUES (1, 2), (2, 3);") +box.sql.execute("CREATE UNIQUE INDEX i ON tj (s2);") +box.sql.execute("UPDATE OR IGNORE tj SET s1 = s1 + 1;") +box.sql.execute("SELECT * FROM tj;") +box.sql.execute("UPDATE OR IGNORE tj SET s2 = s2 + 1;") +box.sql.execute("SELECT * FROM tj;") + +-- gh-3565: INSERT OR REPLACE causes assertion fault. +-- +box.sql.execute("DROP TABLE tj;") +box.sql.execute("CREATE TABLE tj (s1 INT PRIMARY KEY, s2 INT);") +box.sql.execute("INSERT INTO tj VALUES (1, 2),(2, 3);") +box.sql.execute("CREATE UNIQUE INDEX i ON tj (s2);") +box.sql.execute("REPLACE INTO tj VALUES (1, 3);") +box.sql.execute("SELECT * FROM tj;") +box.sql.execute("INSERT INTO tj VALUES (2, 4), (3, 5);") +box.sql.execute("UPDATE OR REPLACE tj SET s2 = s2 + 1;") +box.sql.execute("SELECT * FROM tj;") + +box.sql.execute("DROP TABLE tj;") -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes 2018-08-12 14:13 ` [tarantool-patches] [PATCH 09/10] sql: disable ON CONFLICT actions for indexes Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: tarantool-patches, 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; > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-21 16:31 ` n.pettik 2018-08-24 21:04 ` Vladislav Shpilevoy 0 siblings, 1 reply; 38+ messages in thread From: n.pettik @ 2018-08-21 16:31 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy >> 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). Ok, surely. Space is not even required here, we can fetch nullable action from table->def: +++ b/src/box/sql/insert.c @@ -859,7 +859,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, continue; enum on_conflict_action on_conflict_nullable = on_conflict != ON_CONFLICT_ACTION_DEFAULT ? - on_conflict : table_column_nullable_action(tab, i); + on_conflict : tab->def->fields[i].nullable_action; @@ -4796,9 +4858,6 @@ extern int sqlSubProgramsRemaining; extern int sqlite3InitDatabase(sqlite3 * db); -enum on_conflict_action -table_column_nullable_action(struct Table *tab, uint32_t column); - +++ b/src/box/sql/vdbeaux.c @@ -3869,30 +3869,3 @@ sqlite3VdbeRecordUnpackMsgpack(struct key_def *key_def, /* Information about the pMem++; } } - -/** - * Return action on nullable constraint violation of given column in given table. - * FIXME: This is implemented in expensive way. For each invocation table lookup - * is performed. In future, first param will be replaced with pointer to struct - * space. - * - * @param tab pointer to the table - * @param column column number for which action to be returned - * @retval return action found for given column - */ -enum on_conflict_action -table_column_nullable_action(struct Table *tab, uint32_t column) -{ - struct space *space = space_cache_find(tab->def->id); - - assert(space != NULL); - - struct tuple_format *format = space->format; - - assert(format != NULL); - assert(format->field_count > column); - - struct tuple_field field = format->fields[column]; - - return field.nullable_action; -} >> --- 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? Well, it is useless code, actually. I forgot to remove it: +++ b/src/box/sql/update.c @@ -144,9 +144,6 @@ sqlite3Update(Parse * pParse, /* The parser context */ } struct space_def *def = pTab->def; - int nIdx = 0; - for (struct Index *idx = pTab->pIndex; idx != NULL; - idx = idx->pNext, nIdx++); ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes 2018-08-21 16:31 ` n.pettik @ 2018-08-24 21:04 ` Vladislav Shpilevoy 2018-08-26 19:44 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-24 21:04 UTC (permalink / raw) To: n.pettik, tarantool-patches Thanks for the fixes! See 1 comment below and a commit on the branch. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 33d243414..9da971415 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -354,11 +351,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > } > #endif /* SQLITE_OMIT_XFER_OPT */ > > - /* Allocate registers for holding the tupleid of the new row, > - * the content of the new row, and the assembled row record. > + /* > + * Allocate registers for holding the tupleid of the new > + * row (if it isn't required first register will contain > + * NULL), the content of the new row, and the assembled > + * row record. > */ > regTupleid = regIns = pParse->nMem + 1; > - pParse->nMem += def->field_count + 1; > + pParse->nMem += def->field_count + 2; Why +2? My presumption is because nMem was not incremented one line above here 'regTupleid = regIns = pParse->nMem + 1;', am I right? Why did it work before? Then could you do it slightly more clear, like this? regTupleid = regIns = ++pParse->nMem; pParse->nMem += def->field_count + 1; ... > regData = regTupleid + 1; > My diff: ==================================================== commit afd06f5dcd83715db8bf57553b784db5104c9b49 Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Date: Fri Aug 24 23:41:42 2018 +0300 Review fixes diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 9da971415..975360f21 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -994,10 +994,10 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, for (uint32_t i = 0; i < kd->part_count; ++i) { if (upd_cols[kd->parts[i].fieldno] >= 0) goto process_index; - } + } continue; } -process_index: ; +process_index: ; int cursor = parse_context->nTab++; vdbe_emit_open_cursor(parse_context, cursor, idx->def->iid, space_by_id(tab->def->id)); @@ -1047,7 +1047,6 @@ vdbe_emit_insertion_completion(struct Vdbe *v, int cursor_id, int raw_data_reg, enum on_conflict_action on_conflict) { assert(v != NULL); - int opcode = OP_IdxInsert; u16 pik_flags = OPFLAG_NCHANGE; if (on_conflict == ON_CONFLICT_ACTION_IGNORE) pik_flags |= OPFLAG_OE_IGNORE; @@ -1057,7 +1056,7 @@ vdbe_emit_insertion_completion(struct Vdbe *v, int cursor_id, int raw_data_reg, pik_flags |= OPFLAG_OE_ROLLBACK; sqlite3VdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len, raw_data_reg + tuple_len); - sqlite3VdbeAddOp2(v, opcode, cursor_id, raw_data_reg + tuple_len); + sqlite3VdbeAddOp2(v, OP_IdxInsert, cursor_id, raw_data_reg + tuple_len); sqlite3VdbeChangeP5(v, pik_flags); } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 9a45acedd..7340941c0 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3916,10 +3916,10 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, /** * This routine generates code to finish the INSERT or UPDATE * operation that was started by a prior call to - * sqlite3GenerateConstraintChecks. It encodes raw data which - * is held in a range of registers starting from @raw_data_reg - * and length of @tuple_len and inserts this record to space - * using given @cursor_id. + * vdbe_emit_constraint_checks. It encodes raw data which is held + * in a range of registers starting from @raw_data_reg and length + * of @tuple_len and inserts this record to space using given + * @cursor_id. * * @param v Virtual database engine. * @param cursor_id Primary index cursor. diff --git a/src/box/sql/update.c b/src/box/sql/update.c index 459ee3540..3e39688a5 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -148,12 +148,13 @@ sqlite3Update(Parse * pParse, /* The parser context */ 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) + i = sizeof(int) * def->field_count; + aXRef = (int *) region_alloc(&pParse->region, i); + if (aXRef == NULL) { + diag_set(OutOfMemory, i, "region_alloc", "aXRef"); goto update_cleanup; - for (i = 0; i < (int)def->field_count; i++) - aXRef[i] = -1; + } + memset(aXRef, -1, i); /* Initialize the name-context */ memset(&sNC, 0, sizeof(sNC)); @@ -238,16 +239,16 @@ sqlite3Update(Parse * pParse, /* The parser context */ if (sqlite3ResolveExprNames(&sNC, pWhere)) { goto update_cleanup; } - int iPk; /* First of nPk memory cells holding PRIMARY KEY value */ - int addrOpen; /* Address of the OpenEphemeral instruction */ - - iPk = pParse->nMem + 1; + /* First of nPk memory cells holding PRIMARY KEY value. */ + int iPk = pParse->nMem + 1; pParse->nMem += pk_part_count; regKey = ++pParse->nMem; iEph = pParse->nTab++; sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); - addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, pk_part_count); + /* Address of the OpenEphemeral instruction. */ + int addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, + pk_part_count); pWInfo = sqlite3WhereBegin(pParse, pTabList, pWhere, 0, 0, WHERE_ONEPASS_DESIRED, pk_cursor); if (pWInfo == 0) @@ -425,14 +426,13 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* Do FK constraint checks. */ if (hasFK) fkey_emit_check(pParse, pTab, regOldPk, 0, aXRef); - int addr1 = 0; /* * Delete the index entries associated with the - * current record. It can be already be removed - * by trigger or REPLACE conflict action. + * current record. It can be already removed by + * trigger or REPLACE conflict action. */ - addr1 = sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, 0, - regKey, nKey); + int addr1 = sqlite3VdbeAddOp4Int(v, OP_NotFound, pk_cursor, 0, + regKey, nKey); assert(regNew == regNewPk + 1); sqlite3VdbeAddOp2(v, OP_Delete, pk_cursor, 0); sqlite3VdbeJumpHere(v, addr1); @@ -483,7 +483,6 @@ sqlite3Update(Parse * pParse, /* The parser context */ } update_cleanup: - sqlite3DbFree(db, aXRef); sqlite3SrcListDelete(db, pTabList); sql_expr_list_delete(db, pChanges); sql_expr_delete(db, pWhere, false); diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result index 4b5d2cd7e..63fe48e79 100644 --- a/test/sql/on-conflict.result +++ b/test/sql/on-conflict.result @@ -1,24 +1,13 @@ test_run = require('test_run').new() --- ... ---- -... ---- -... engine = test_run:get_cfg('engine') --- ... ---- -... ---- -... box.sql.execute('pragma sql_default_engine=\''..engine..'\'') --- ... ---- -... ---- -... +-- -- Check that original SQLite ON CONFLICT clause is really -- disabled. -- diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua index ab73249ef..b2d8e0589 100644 --- a/test/sql/on-conflict.test.lua +++ b/test/sql/on-conflict.test.lua @@ -1,12 +1,7 @@ test_run = require('test_run').new() ---- -... engine = test_run:get_cfg('engine') ---- -... box.sql.execute('pragma sql_default_engine=\''..engine..'\'') ---- -... +-- -- Check that original SQLite ON CONFLICT clause is really -- disabled. -- ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes 2018-08-24 21:04 ` Vladislav Shpilevoy @ 2018-08-26 19:44 ` n.pettik 2018-08-27 17:24 ` Vladislav Shpilevoy 0 siblings, 1 reply; 38+ messages in thread From: n.pettik @ 2018-08-26 19:44 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy >> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c >> index 33d243414..9da971415 100644 >> --- a/src/box/sql/insert.c >> +++ b/src/box/sql/insert.c >> @@ -354,11 +351,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ >> } >> #endif /* SQLITE_OMIT_XFER_OPT */ >> - /* Allocate registers for holding the tupleid of the new row, >> - * the content of the new row, and the assembled row record. >> + /* >> + * Allocate registers for holding the tupleid of the new >> + * row (if it isn't required first register will contain >> + * NULL), the content of the new row, and the assembled >> + * row record. >> */ >> regTupleid = regIns = pParse->nMem + 1; >> - pParse->nMem += def->field_count + 1; >> + pParse->nMem += def->field_count + 2; > > Why +2? My presumption is because nMem was not > incremented one line above here 'regTupleid = regIns = pParse->nMem + 1;', > am I right? Why did it work before? As comments says: field_count for raw data to be encoded into tuple, one memory cell for encoded tuple, and the last one for rowid (or tupleid): it is used for ephemeral spaces to distinguish entries. If it is not required, then mem will contain NULL. Tuple id (regTupleId) comes first, then raw tuple and in the end - encoded msgpack. > > Then could you do it slightly more clear, like this? > > regTupleid = regIns = ++pParse->nMem; > pParse->nMem += def->field_count + 1; As you wish. Simply applied your code: +++ b/src/box/sql/insert.c @@ -357,8 +357,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ * NULL), the content of the new row, and the assembled * row record. */ - regTupleid = regIns = pParse->nMem + 1; - pParse->nMem += def->field_count + 2; + regTupleid = regIns = ++pParse->nMem; + pParse->nMem += def->field_count + 1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 09/10] sql: disable ON CONFLICT actions for indexes 2018-08-26 19:44 ` n.pettik @ 2018-08-27 17:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-27 17:24 UTC (permalink / raw) To: n.pettik, tarantool-patches On 26/08/2018 16:44, n.pettik wrote: > >>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c >>> index 33d243414..9da971415 100644 >>> --- a/src/box/sql/insert.c >>> +++ b/src/box/sql/insert.c >>> @@ -354,11 +351,14 @@ sqlite3Insert(Parse * pParse, /* Parser context */ >>> } >>> #endif /* SQLITE_OMIT_XFER_OPT */ >>> - /* Allocate registers for holding the tupleid of the new row, >>> - * the content of the new row, and the assembled row record. >>> + /* >>> + * Allocate registers for holding the tupleid of the new >>> + * row (if it isn't required first register will contain >>> + * NULL), the content of the new row, and the assembled >>> + * row record. >>> */ >>> regTupleid = regIns = pParse->nMem + 1; >>> - pParse->nMem += def->field_count + 1; >>> + pParse->nMem += def->field_count + 2; >> >> Why +2? My presumption is because nMem was not >> incremented one line above here 'regTupleid = regIns = pParse->nMem + 1;', >> am I right? Why did it work before? > > As comments says: field_count for raw data to be encoded into tuple, > one memory cell for encoded tuple, and the last one for rowid (or tupleid): > it is used for ephemeral spaces to distinguish entries. If it is not required, > then mem will contain NULL. Tuple id (regTupleId) comes first, then > raw tuple and in the end - encoded msgpack. Thanks for the explanation. I understand the comment. But I do not understand why nMem is changed to +1 in comparison with the previous version. Your comment is actually the same as it was, but nMem is increased. The comment is this in short: "Allocate registers for holding the tupleid, content, assembled record.". Both before and after your patch. But nMem is now +1. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] [PATCH 10/10] sql: move autoincrement field number to server 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (8 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 09/10] sql: disable ON CONFLICT actions for indexes Nikita Pettik @ 2018-08-12 14:13 ` Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-27 17:24 ` [tarantool-patches] Re: [PATCH 00/10] sql: cleanup in struct Index and struct Table Vladislav Shpilevoy 2018-08-29 14:11 ` Kirill Yukhin 11 siblings, 1 reply; 38+ messages in thread From: Nikita Pettik @ 2018-08-12 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik During INSERT SQL statement execution we may need to know field which stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to get rid of struct Table, lets move this member to space's opts. Part of #3561 --- src/box/space_def.c | 3 +++ src/box/space_def.h | 5 +++++ src/box/sql.c | 11 +++++++++-- src/box/sql/build.c | 6 +++--- src/box/sql/insert.c | 20 +++++++++++++------- src/box/sql/sqliteInt.h | 2 -- 6 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/box/space_def.c b/src/box/space_def.c index f5ca0b59a..d81546413 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -56,6 +56,7 @@ const struct space_opts space_opts_default = { /* .view = */ false, /* .sql = */ NULL, /* .checks = */ NULL, + /* .sql_autoinc_fieldno = */ UINT32_MAX, }; const struct opt_def space_opts_reg[] = { @@ -65,6 +66,8 @@ const struct opt_def space_opts_reg[] = { OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql), OPT_DEF_ARRAY("checks", struct space_opts, checks, checks_array_decode), + OPT_DEF("sql_autoinc", OPT_UINT32, struct space_opts, + sql_autoinc_fieldno), OPT_END, }; diff --git a/src/box/space_def.h b/src/box/space_def.h index 0d1e90233..c5d305bf6 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -68,6 +68,11 @@ struct space_opts { char *sql; /** SQL Checks expressions list. */ struct ExprList *checks; + /** + * If SQL table is created with AUTOINCREMENT primary + * key then this member contains its ordinal number. + */ + uint32_t sql_autoinc_fieldno; }; extern const struct space_opts space_opts_default; diff --git a/src/box/sql.c b/src/box/sql.c index 1f59946f0..d169b18ed 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -749,7 +749,7 @@ sql_rename_table(uint32_t space_id, const char *new_name, char **sql_stmt) if (sql_stmt_map == NULL || mp_typeof(*sql_stmt_map) != MP_MAP) goto rename_fail; uint32_t map_size = mp_decode_map(&sql_stmt_map); - if (map_size != 1) + if (map_size == 0) goto rename_fail; const char *sql_str = mp_decode_str(&sql_stmt_map, &key_len); @@ -1358,8 +1358,11 @@ tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf) const struct Enc *enc = get_enc(buf); bool is_view = pTable != NULL && pTable->def->opts.is_view; bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL; + bool has_autoinc = pTable != NULL && + pTable->def->opts.sql_autoinc_fieldno != UINT32_MAX; int checks_cnt = has_checks ? pTable->def->opts.checks->nExpr : 0; - char *p = enc->encode_map(buf, 1 + is_view + (checks_cnt > 0)); + char *p = enc->encode_map(buf, 1 + is_view + has_autoinc + + (checks_cnt > 0)); p = enc->encode_str(p, "sql", 3); p = enc->encode_str(p, zSql, strlen(zSql)); @@ -1367,6 +1370,10 @@ tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf) p = enc->encode_str(p, "view", 4); p = enc->encode_bool(p, true); } + if (has_autoinc) { + p = enc->encode_str(p, "sql_autoinc", strlen("sql_autoinc")); + p = enc->encode_uint(p, pTable->def->opts.sql_autoinc_fieldno); + } if (checks_cnt == 0) return p - buf; /* Encode checks. */ diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 9d7f0a4e0..dbd911585 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -401,7 +401,7 @@ sql_table_new(Parse *parser, char *name) strcpy(table->def->engine_name, sql_storage_engine_strs[current_session()->sql_default_engine]); - table->iAutoIncPKey = -1; + table->def->opts.sql_autoinc_fieldno = UINT32_MAX; table->pSchema = db->pSchema; table->nTabRef = 1; return table; @@ -869,7 +869,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ sortOrder != SORT_ORDER_DESC) { assert(autoInc == 0 || autoInc == 1); if (autoInc) - pTab->iAutoIncPKey = iCol; + pTab->def->opts.sql_autoinc_fieldno = iCol; struct sqlite3 *db = pParse->db; struct ExprList *list; struct Token token; @@ -1698,7 +1698,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ * Check to see if we need to create an _sequence table * for keeping track of autoincrement keys. */ - if (p->iAutoIncPKey >= 0) { + if (p->def->opts.sql_autoinc_fieldno != UINT32_MAX) { assert(reg_space_id != 0); /* Do an insertion into _sequence. */ int reg_seq_id = ++pParse->nMem; diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 271886ec6..e53cbf05a 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -595,7 +595,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } if ((!useTempTable && !pList) || (pColumn && j >= pColumn->nId)) { - if (i == pTab->iAutoIncPKey) { + if (i == + (int) pTab->def->opts.sql_autoinc_fieldno) { sqlite3VdbeAddOp2(v, OP_Integer, -1, regCols + i + 1); } else { @@ -658,7 +659,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } if (j < 0 || nColumn == 0 || (pColumn && j >= pColumn->nId)) { - if (i == pTab->iAutoIncPKey) { + if (i == + (int) pTab->def->opts.sql_autoinc_fieldno) { sqlite3VdbeAddOp2(v, OP_NextAutoincValue, pTab->def->id, @@ -673,7 +675,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ dflt, iRegStore); } else if (useTempTable) { - if (i == pTab->iAutoIncPKey) { + if (i == + (int) pTab->def->opts.sql_autoinc_fieldno) { int regTmp = ++pParse->nMem; /* Emit code which doesn't override * autoinc-ed value with select result @@ -692,7 +695,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } } else if (pSelect) { if (regFromSelect != regData) { - if (i == pTab->iAutoIncPKey) { + if (i == + (int) pTab->def->opts.sql_autoinc_fieldno) { /* Emit code which doesn't override * autoinc-ed value with select result * in case that result is NULL @@ -714,7 +718,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } } else { - if (i == pTab->iAutoIncPKey) { + if (i == + (int) pTab->def->opts.sql_autoinc_fieldno) { if (pList->a[j].pExpr->op == TK_NULL) { sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore); continue; @@ -876,7 +881,8 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, if (is_update && upd_cols[i] < 0) continue; /* This column is allowed to be NULL. */ - if (def->fields[i].is_nullable || tab->iAutoIncPKey == (int) i) + if (def->fields[i].is_nullable || + def->opts.sql_autoinc_fieldno == i) continue; enum on_conflict_action on_conflict_nullable = on_conflict != ON_CONFLICT_ACTION_DEFAULT ? @@ -973,7 +979,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, int reg_pk = new_tuple_reg + fieldno; if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { int skip_if_null = sqlite3VdbeMakeLabel(v); - if (tab->iAutoIncPKey >= 0) { + if (def->opts.sql_autoinc_fieldno != UINT32_MAX) { sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, skip_if_null); } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 45dab0be4..2d29a10f1 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1850,8 +1850,6 @@ struct Savepoint { struct Table { Index *pIndex; /* List of SQL indexes on this table. */ u32 nTabRef; /* Number of pointers to this Table */ - i16 iAutoIncPKey; /* If PK is marked INTEGER PRIMARY KEY AUTOINCREMENT, store - column number here, -1 otherwise Tarantool specifics */ /** * Estimated number of entries in table. * Used only when table represents temporary objects, -- 2.15.1 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server 2018-08-12 14:13 ` [tarantool-patches] [PATCH 10/10] sql: move autoincrement field number to server Nikita Pettik @ 2018-08-13 20:24 ` Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-13 20:24 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches Are you sure it is the only way to pass autoinc fieldno? And that it can not be dropped and calculated from other fields without significant problems? Now this flag looks very ugly inside _space tuples. I think, autoinc is rather primary index option than the space, and that it can be detected via checking pk->part_count == 1 and space->sequence != NULL then pk->parts[0].fieldno is autoinc field. It is not? On 12/08/2018 17:13, Nikita Pettik wrote: > During INSERT SQL statement execution we may need to know field which > stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to get rid > of struct Table, lets move this member to space's opts. > > Part of #3561 > --- > src/box/space_def.c | 3 +++ > src/box/space_def.h | 5 +++++ > src/box/sql.c | 11 +++++++++-- > src/box/sql/build.c | 6 +++--- > src/box/sql/insert.c | 20 +++++++++++++------- > src/box/sql/sqliteInt.h | 2 -- > 6 files changed, 33 insertions(+), 14 deletions(-) > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-21 16:31 ` n.pettik 2018-08-24 21:03 ` Vladislav Shpilevoy 0 siblings, 1 reply; 38+ messages in thread From: n.pettik @ 2018-08-21 16:31 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Are you sure it is the only way to pass autoinc fieldno? And that it can > not be dropped and calculated from other fields without significant problems? > > Now this flag looks very ugly inside _space tuples. > > I think, autoinc is rather primary index option than the space, and that it > can be detected via checking > > pk->part_count == 1 and space->sequence != NULL > > then pk->parts[0].fieldno is autoinc field. It is not? Okay, it seems to be possible. Check this out: sql: remove autoincrement field number from Table During INSERT SQL statement execution we may need to know field which stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to get rid of struct Table, lets remove this member from struct Table. Instead, it can be calculated using struct space: if PK consists from one part and sequence not NULL - table features autoincrement. Part of #3561 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index b38a934a1..dc00b5d8c 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -401,7 +401,6 @@ sql_table_new(Parse *parser, char *name) strcpy(table->def->engine_name, sql_storage_engine_strs[current_session()->sql_default_engine]); - table->iAutoIncPKey = -1; table->pSchema = db->pSchema; table->nTabRef = 1; return table; @@ -868,8 +867,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER && sortOrder != SORT_ORDER_DESC) { assert(autoInc == 0 || autoInc == 1); - if (autoInc) - pTab->iAutoIncPKey = iCol; + pParse->is_new_table_autoinc = autoInc; struct sqlite3 *db = pParse->db; struct ExprList *list; struct Token token; @@ -1698,7 +1696,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ * Check to see if we need to create an _sequence table * for keeping track of autoincrement keys. */ - if (p->iAutoIncPKey >= 0) { + if (pParse->is_new_table_autoinc) { assert(reg_space_id != 0); /* Do an insertion into _sequence. */ int reg_seq_id = ++pParse->nMem; diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index d9e230aee..7780bf749 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -100,6 +100,22 @@ sql_emit_table_affinity(struct Vdbe *v, struct space_def *def, int reg) P4_DYNAMIC); } +/** + * In SQL table can be created with AUTOINCREMENT. + * In Tarantool it can be detected as primary key which consists + * from one field with not NULL space's sequence. + */ +static uint32_t +sql_space_autoinc_fieldno(struct space *space) +{ + assert(space != NULL); + struct index *pk = space_index(space, 0); + if (pk == NULL || pk->def->key_def->part_count != 1 || + space->sequence == NULL) + return UINT32_MAX; + return pk->def->key_def->parts[0].fieldno; +} + /** * This routine is used to see if a statement of the form * "INSERT INTO <table> SELECT ..." can run for the results of the @@ -556,7 +572,9 @@ sqlite3Insert(Parse * pParse, /* Parser context */ sqlite3VdbeAddOp1(v, OP_Yield, dest.iSDParm); VdbeCoverage(v); } - + struct space *space = space_by_id(pTab->def->id); + assert(space != NULL); + uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space); /* Run the BEFORE and INSTEAD OF triggers, if there are any */ endOfLoop = sqlite3VdbeMakeLabel(v); @@ -574,7 +592,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } if ((!useTempTable && !pList) || (pColumn && j >= pColumn->nId)) { - if (i == pTab->iAutoIncPKey) { + if (i == (int) autoinc_fieldno) { sqlite3VdbeAddOp2(v, OP_Integer, -1, regCols + i + 1); } else { @@ -637,7 +655,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } if (j < 0 || nColumn == 0 || (pColumn && j >= pColumn->nId)) { - if (i == pTab->iAutoIncPKey) { + if (i == (int) autoinc_fieldno) { sqlite3VdbeAddOp2(v, OP_NextAutoincValue, pTab->def->id, @@ -652,7 +670,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ dflt, iRegStore); } else if (useTempTable) { - if (i == pTab->iAutoIncPKey) { + if (i == (int) autoinc_fieldno) { int regTmp = ++pParse->nMem; /* Emit code which doesn't override * autoinc-ed value with select result @@ -671,7 +689,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } } else if (pSelect) { if (regFromSelect != regData) { - if (i == pTab->iAutoIncPKey) { + if (i == (int) autoinc_fieldno) { /* Emit code which doesn't override * autoinc-ed value with select result * in case that result is NULL @@ -693,7 +711,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } } else { - if (i == pTab->iAutoIncPKey) { + if (i == (int) autoinc_fieldno) { if (pList->a[j].pExpr->op == TK_NULL) { sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore); continue; @@ -730,8 +748,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */ on_error, endOfLoop, 0); fkey_emit_check(pParse, pTab, 0, regIns, 0); int pk_cursor = pParse->nTab++; - struct space *space = space_by_id(pTab->def->id); - assert(space != NULL); vdbe_emit_open_cursor(pParse, pk_cursor, 0, space); vdbe_emit_insertion_completion(v, pk_cursor, regIns + 1, pTab->def->field_count, @@ -846,6 +862,9 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, /* Insertion into VIEW is prohibited. */ assert(!def->opts.is_view); bool is_update = upd_cols != NULL; + struct space *space = space_by_id(tab->def->id); + assert(space != NULL); + uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space); /* Test all NOT NULL constraints. */ for (uint32_t i = 0; i < def->field_count; i++) { /* @@ -855,7 +874,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, if (is_update && upd_cols[i] < 0) continue; /* This column is allowed to be NULL. */ - if (def->fields[i].is_nullable || tab->iAutoIncPKey == (int) i) + if (def->fields[i].is_nullable || autoinc_fieldno == i) continue; enum on_conflict_action on_conflict_nullable = on_conflict != ON_CONFLICT_ACTION_DEFAULT ? @@ -948,7 +967,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, int reg_pk = new_tuple_reg + fieldno; if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { int skip_if_null = sqlite3VdbeMakeLabel(v); - if (tab->iAutoIncPKey >= 0) { + if (autoinc_fieldno != UINT32_MAX) { sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, skip_if_null); } @@ -1000,7 +1019,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, process_index: ; int cursor = parse_context->nTab++; vdbe_emit_open_cursor(parse_context, cursor, idx->def->iid, - space_by_id(tab->def->id)); + space); /* * If there is no conflict in current index, just * jump to the start of next iteration. Label is diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 9a45acedd..d2ef85846 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1850,8 +1850,6 @@ struct Savepoint { struct Table { Index *pIndex; /* List of SQL indexes on this table. */ u32 nTabRef; /* Number of pointers to this Table */ - i16 iAutoIncPKey; /* If PK is marked INTEGER PRIMARY KEY AUTOINCREMENT, store - column number here, -1 otherwise Tarantool specifics */ /** * Estimated number of entries in table. * Used only when table represents temporary objects, @@ -2838,6 +2836,8 @@ struct Parse { */ struct rlist new_fkey; bool initiateTTrans; /* Initiate Tarantool transaction */ + /** True, if table to be created has AUTOINCREMENT PK. */ + bool is_new_table_autoinc; /** If set - do not emit byte code at all, just parse. */ bool parse_only; /** Type of parsed_ast member. */ ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server 2018-08-21 16:31 ` n.pettik @ 2018-08-24 21:03 ` Vladislav Shpilevoy 2018-08-26 19:44 ` n.pettik 0 siblings, 1 reply; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-24 21:03 UTC (permalink / raw) To: n.pettik, tarantool-patches Thanks for the patch! On 21/08/2018 19:31, n.pettik wrote: > >> On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: >> >> Are you sure it is the only way to pass autoinc fieldno? And that it can >> not be dropped and calculated from other fields without significant problems? >> >> Now this flag looks very ugly inside _space tuples. >> >> I think, autoinc is rather primary index option than the space, and that it >> can be detected via checking >> >> pk->part_count == 1 and space->sequence != NULL >> >> then pk->parts[0].fieldno is autoinc field. It is not? > > Okay, it seems to be possible. Check this out: > > sql: remove autoincrement field number from Table > > During INSERT SQL statement execution we may need to know field which > stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to get rid > of struct Table, lets remove this member from struct Table. Instead, it > can be calculated using struct space: if PK consists from one part and > sequence not NULL - table features autoincrement. > > Part of #3561 > I have done some review fixes, but the patch crashes with them, and I do not understand why. Could you please investigate? Assertion is 100% repeatable on analyze9.test: Assertion failed: (0), function vdbe_emit_constraint_checks, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/insert.c, line 916. Right after test case 4.9. The diff: =================================================== diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 56ddb10eb..4cc33e830 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -858,12 +858,12 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, 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); bool is_update = upd_cols != NULL; struct space *space = space_by_id(tab->def->id); assert(space != NULL); + struct space_def *def = space->def; + /* Insertion into VIEW is prohibited. */ + assert(!def->opts.is_view); uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space); /* Test all NOT NULL constraints. */ for (uint32_t i = 0; i < def->field_count; i++) { @@ -882,7 +882,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, /* 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); + struct Expr *dflt = def->fields[i].default_value_expr; if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE && dflt == NULL) on_conflict_nullable = ON_CONFLICT_ACTION_ABORT; @@ -923,7 +923,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, if (on_conflict == ON_CONFLICT_ACTION_DEFAULT) on_conflict = ON_CONFLICT_ACTION_ABORT; /* Test all CHECK constraints. */ - struct ExprList *checks = space_checks_expr_list(tab->def->id); + struct ExprList *checks = def->opts.checks; enum on_conflict_action on_conflict_check = on_conflict; if (on_conflict == ON_CONFLICT_ACTION_REPLACE) on_conflict_check = ON_CONFLICT_ACTION_ABORT; ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server 2018-08-24 21:03 ` Vladislav Shpilevoy @ 2018-08-26 19:44 ` n.pettik 2018-08-27 17:24 ` Vladislav Shpilevoy 0 siblings, 1 reply; 38+ messages in thread From: n.pettik @ 2018-08-26 19:44 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy >>> On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: >>> >>> Are you sure it is the only way to pass autoinc fieldno? And that it can >>> not be dropped and calculated from other fields without significant problems? >>> >>> Now this flag looks very ugly inside _space tuples. >>> >>> I think, autoinc is rather primary index option than the space, and that it >>> can be detected via checking >>> >>> pk->part_count == 1 and space->sequence != NULL >>> >>> then pk->parts[0].fieldno is autoinc field. It is not? >> Okay, it seems to be possible. Check this out: >> sql: remove autoincrement field number from Table >> During INSERT SQL statement execution we may need to know field which >> stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to get rid >> of struct Table, lets remove this member from struct Table. Instead, it >> can be calculated using struct space: if PK consists from one part and >> sequence not NULL - table features autoincrement. >> Part of #3561 > > I have done some review fixes, but the patch crashes with them, > and I do not understand why. Could you please investigate? Yep, surely. > > Assertion is 100% repeatable on analyze9.test: > Assertion failed: (0), function vdbe_emit_constraint_checks, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/insert.c, line 916. > > Right after test case 4.9. > > The diff: > > =================================================== > > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 56ddb10eb..4cc33e830 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -858,12 +858,12 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, > 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); > bool is_update = upd_cols != NULL; > struct space *space = space_by_id(tab->def->id); > assert(space != NULL); > + struct space_def *def = space->def; > + /* Insertion into VIEW is prohibited. */ > + assert(!def->opts.is_view); > uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space); > /* Test all NOT NULL constraints. */ > for (uint32_t i = 0; i < def->field_count; i++) { > @@ -882,7 +882,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, > /* 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); > + struct Expr *dflt = def->fields[i].default_value_expr; > if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE && > dflt == NULL) > on_conflict_nullable = ON_CONFLICT_ACTION_ABORT; > @@ -923,7 +923,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, > if (on_conflict == ON_CONFLICT_ACTION_DEFAULT) > on_conflict = ON_CONFLICT_ACTION_ABORT; > /* Test all CHECK constraints. */ > - struct ExprList *checks = space_checks_expr_list(tab->def->id); > + struct ExprList *checks = def->opts.checks; > enum on_conflict_action on_conflict_check = on_conflict; > if (on_conflict == ON_CONFLICT_ACTION_REPLACE) > on_conflict_check = ON_CONFLICT_ACTION_ABORT; I slightly changed your diff. Final version is (no assertions happen and all tests are passed): +++ b/src/box/sql/insert.c @@ -858,12 +858,12 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, 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); bool is_update = upd_cols != NULL; struct space *space = space_by_id(tab->def->id); assert(space != NULL); + struct space_def *def = space->def; + /* Insertion into VIEW is prohibited. */ + assert(!def->opts.is_view); uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space); /* Test all NOT NULL constraints. */ for (uint32_t i = 0; i < def->field_count; i++) { @@ -878,11 +878,11 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, continue; enum on_conflict_action on_conflict_nullable = on_conflict != ON_CONFLICT_ACTION_DEFAULT ? - on_conflict : tab->def->fields[i].nullable_action; + on_conflict : def->fields[i].nullable_action; /* 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); + struct Expr *dflt = space_column_default_expr(def->id, i); if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE && dflt == NULL) on_conflict_nullable = ON_CONFLICT_ACTION_ABORT; @@ -923,7 +923,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, if (on_conflict == ON_CONFLICT_ACTION_DEFAULT) on_conflict = ON_CONFLICT_ACTION_ABORT; /* Test all CHECK constraints. */ - struct ExprList *checks = space_checks_expr_list(tab->def->id); + struct ExprList *checks = space_checks_expr_list(def->id); enum on_conflict_action on_conflict_check = on_conflict; if (on_conflict == ON_CONFLICT_ACTION_REPLACE) on_conflict_check = ON_CONFLICT_ACTION_ABORT; @@ -965,7 +965,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, if (part_count == 1) { uint32_t fieldno = pk->def->key_def->parts[0].fieldno; int reg_pk = new_tuple_reg + fieldno; - if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { + if (def->fields[fieldno].affinity == AFFINITY_INTEGER) { int skip_if_null = sqlite3VdbeMakeLabel(v); if (autoinc_fieldno != UINT32_MAX) { sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, Your initial diff led to assertion fault since def->fields[i].nullable_action and tab->def->fields[i].nullable_action had different values - DEFAULT and NONE respectively. So I just replaced all usages of tab->def->… to simply def->… Why they had different nullable actions? _sql_stat4 is created from Lua and has no specified is_nullable property, so nullable action is set to DEFAULT: alter.cc (373 lie) if (is_action_missing) { field->nullable_action = field->is_nullable ? ON_CONFLICT_ACTION_NONE : ON_CONFLICT_ACTION_DEFAULT; } For SQL-land tables nullable action is never set to DEFAULT: it is either ABORT or NONE. In fact, DEFAULT makes no sense, so it is always substituted with ABORT (field_def_create_for_pk() in build.c). Hence, I guess setting default nullable action to DEFAULT in alter.cc seems to be messy and should be changed to ABORT. DEFAULT is really helpful for statement action (e.g. INSERT OR REPLACE): in this case it allows to tell INSERT OR ABORT (ABORT action) from simple INSERT (DEFAULT action). Do you agree with me? Btw now I see in trunk commit: https://github.com/tarantool/tarantool/commit/db37dd2d663e23d55fe2caecbf911e822e9182d4 IMHO it makes things even worse... ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server 2018-08-26 19:44 ` n.pettik @ 2018-08-27 17:24 ` Vladislav Shpilevoy 0 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-27 17:24 UTC (permalink / raw) To: n.pettik, tarantool-patches > @@ -965,7 +965,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, > if (part_count == 1) { > uint32_t fieldno = pk->def->key_def->parts[0].fieldno; > int reg_pk = new_tuple_reg + fieldno; > - if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) { > + if (def->fields[fieldno].affinity == AFFINITY_INTEGER) { > int skip_if_null = sqlite3VdbeMakeLabel(v); > if (autoinc_fieldno != UINT32_MAX) { > sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk, > > Your initial diff led to assertion fault since def->fields[i].nullable_action and > tab->def->fields[i].nullable_action had different values - DEFAULT and NONE respectively. > So I just replaced all usages of tab->def->… to simply def->… > Why they had different nullable actions? _sql_stat4 is created from Lua and has no specified > is_nullable property, so nullable action is set to DEFAULT: > > alter.cc (373 lie) > > if (is_action_missing) { > field->nullable_action = field->is_nullable ? > ON_CONFLICT_ACTION_NONE > : ON_CONFLICT_ACTION_DEFAULT; > } > > For SQL-land tables nullable action is never set to DEFAULT: > it is either ABORT or NONE. In fact, DEFAULT makes no sense, > so it is always substituted with ABORT (field_def_create_for_pk() in build.c). > Hence, I guess setting default nullable action to DEFAULT in alter.cc seems > to be messy and should be changed to ABORT. DEFAULT is really helpful > for statement action (e.g. INSERT OR REPLACE): in this case it allows to > tell INSERT OR ABORT (ABORT action) from simple INSERT (DEFAULT action). > Do you agree with me? > > Btw now I see in trunk commit: > https://github.com/tarantool/tarantool/commit/db37dd2d663e23d55fe2caecbf911e822e9182d4 > > IMHO it makes things even worse... Thanks for the debugging for me! Sure, DEFAULT from Lua is bad idea as we can see in this particular example. I believe, DEFAULT should never be set from non-SQL DDL. If a field is set to is_nullable false, it is false always, not default. So the commit db37dd2d663e23d55fe2caecbf911e822e9182d4 is wrong, I think. Unfortunately, it was pushed without a review, as I see. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 00/10] sql: cleanup in struct Index and struct Table 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (9 preceding siblings ...) 2018-08-12 14:13 ` [tarantool-patches] [PATCH 10/10] sql: move autoincrement field number to server Nikita Pettik @ 2018-08-27 17:24 ` Vladislav Shpilevoy 2018-08-29 14:11 ` Kirill Yukhin 11 siblings, 0 replies; 38+ messages in thread From: Vladislav Shpilevoy @ 2018-08-27 17:24 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches Thanks a lot for the patchset! LGTM. On 12/08/2018 11:12, Nikita Pettik wrote: > Branch: https://github.com/tarantool/tarantool/tree/np/cleanup-in-Index-and-Table > Issues: > https://github.com/tarantool/tarantool/issues/3561 > https://github.com/tarantool/tarantool/issues/3565 > https://github.com/tarantool/tarantool/issues/3566 > > Current patch-set is preliminary to finishing DD integration. > It gets rid off all functional fields from original SQLite's struct > Index and struct Table, which can be substituted with corresponding > fields from server structures or be calculated using them. > > It is worth mentioning ninth patch in series which removes support of > ON CONFLICT error actions related to index uniqueness. Firstly, alongside > with it INSERT and UPDATE code generation has been slightly simplified: > removed a lot of excess register allocations; moved cursors' allocations > closer to their usages (auxiliary cursors on secondary indexes are > required only to process REPLACE or IGNORE conflict actions); > significantly reworked function which generates code to process > constraint checks. To sump up, all these changes result in fixing > assertion fault during INSERT OR REPLACE and incorrect behavior > of UPDATE OR IGNORE statements. > > * To those who will review * > I deliberately separated all commits on removing fields since this way > seems to be easier to review and understand provided changes. > After review I can squash some of them, if you want so. > > Nikita Pettik (10): > sql: remove suport of ALTER TABLE ADD COLUMN > sql: remove string of fields collation from Table > sql: remove index hash from struct Table > sql: remove flags from struct Table > sql: remove affinity string of columns from Index > sql: completely remove support of partial indexes > sql: remove index type from struct Index > sql: use secondary indexes to process OP_Delete > sql: disable ON CONFLICT actions for indexes > sql: move autoincrement field number to server > > src/box/space_def.c | 3 + > src/box/space_def.h | 5 + > src/box/sql.c | 37 +- > src/box/sql/alter.c | 185 ------ > src/box/sql/analyze.c | 2 +- > src/box/sql/build.c | 263 +++------ > src/box/sql/delete.c | 20 +- > src/box/sql/insert.c | 968 ++++++++----------------------- > src/box/sql/main.c | 45 +- > src/box/sql/parse.y | 42 +- > src/box/sql/pragma.c | 10 +- > src/box/sql/prepare.c | 9 +- > src/box/sql/select.c | 4 +- > src/box/sql/sqliteInt.h | 198 ++++--- > src/box/sql/tarantoolInt.h | 16 +- > src/box/sql/update.c | 252 ++------ > src/box/sql/vdbe.c | 11 +- > src/box/sql/vdbeaux.c | 11 +- > src/box/sql/vdbemem.c | 3 +- > src/box/sql/where.c | 64 +- > src/box/sql/wherecode.c | 7 +- > 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 ++- > 31 files changed, 733 insertions(+), 2229 deletions(-) > delete mode 100755 test/sql-tap/conflict3.test.lua > ^ permalink raw reply [flat|nested] 38+ messages in thread
* [tarantool-patches] Re: [PATCH 00/10] sql: cleanup in struct Index and struct Table 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik ` (10 preceding siblings ...) 2018-08-27 17:24 ` [tarantool-patches] Re: [PATCH 00/10] sql: cleanup in struct Index and struct Table Vladislav Shpilevoy @ 2018-08-29 14:11 ` Kirill Yukhin 11 siblings, 0 replies; 38+ messages in thread From: Kirill Yukhin @ 2018-08-29 14:11 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Hello, On 12 авг 17:12, Nikita Pettik wrote: > Branch: https://github.com/tarantool/tarantool/tree/np/cleanup-in-Index-and-Table > Issues: > https://github.com/tarantool/tarantool/issues/3561 > https://github.com/tarantool/tarantool/issues/3565 > https://github.com/tarantool/tarantool/issues/3566 I've pushed the patchset into 2.0 branch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2018-08-29 14:11 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-12 14:12 [tarantool-patches] [PATCH 00/10] sql: cleanup in struct Index and struct Table Nikita Pettik 2018-08-12 14:12 ` [tarantool-patches] [PATCH 01/10] sql: remove suport of ALTER TABLE ADD COLUMN Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:12 ` [tarantool-patches] [PATCH 02/10] sql: remove string of fields collation from Table Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:12 ` [tarantool-patches] [PATCH 03/10] sql: remove index hash from struct Table Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 04/10] sql: remove flags " Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 05/10] sql: remove affinity string of columns from Index Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 2018-08-24 21:04 ` Vladislav Shpilevoy 2018-08-26 19:45 ` n.pettik 2018-08-12 14:13 ` [tarantool-patches] [PATCH 06/10] sql: completely remove support of partial indexes Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 2018-08-24 21:04 ` Vladislav Shpilevoy 2018-08-26 19:44 ` n.pettik 2018-08-12 14:13 ` [tarantool-patches] [PATCH 07/10] sql: remove index type from struct Index Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 2018-08-12 14:13 ` [tarantool-patches] [PATCH 08/10] sql: use secondary indexes to process OP_Delete Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 09/10] sql: disable ON CONFLICT actions for indexes Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 2018-08-24 21:04 ` Vladislav Shpilevoy 2018-08-26 19:44 ` n.pettik 2018-08-27 17:24 ` Vladislav Shpilevoy 2018-08-12 14:13 ` [tarantool-patches] [PATCH 10/10] sql: move autoincrement field number to server Nikita Pettik 2018-08-13 20:24 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-21 16:31 ` n.pettik 2018-08-24 21:03 ` Vladislav Shpilevoy 2018-08-26 19:44 ` n.pettik 2018-08-27 17:24 ` Vladislav Shpilevoy 2018-08-27 17:24 ` [tarantool-patches] Re: [PATCH 00/10] sql: cleanup in struct Index and struct Table Vladislav Shpilevoy 2018-08-29 14:11 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox