* [tarantool-patches] [PATCH] sql: refactor SQL delete to allow Lua spaces @ 2018-05-18 13:36 Kirill Yukhin 2018-05-18 21:07 ` [tarantool-patches] " Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Yukhin @ 2018-05-18 13:36 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-3235-sql-truncate-on-lua-spaces Issue: https://github.com/tarantool/tarantool/issues/3235 This is a first step toward fully-featured deletion of spaces created in Lua by means of SQL language. This change to handle most simple case: DELETE * FROM <space_name> and will be improved in nearest future. Part of #3235 --- src/box/sql/analyze.c | 3 +- src/box/sql/build.c | 3 +- src/box/sql/delete.c | 69 ++++++++++++++++++++++++++----------------- src/box/sql/vdbe.c | 2 +- test/sql-tap/delete1.test.lua | 23 ++++++++++++++- 5 files changed, 69 insertions(+), 31 deletions(-) diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 998dd5b..cc594a7 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -169,7 +169,8 @@ openStatTable(Parse * pParse, /* Parsing context */ * The sql_stat[134] table already exists. * Delete all rows. */ - sqlite3VdbeAddOp2(v, OP_Clear, aRoot[i], 0); + sqlite3VdbeAddOp2(v, OP_Clear, + SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0); } } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index df75186..49e59f4 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2672,7 +2672,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) VdbeCoverage(v); sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) - sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); + sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum), + 0); emit_open_cursor(pParse, iIdx, tnum); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR | ((memRootPage >= 0) ? diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index f4d248e..dfc91cb 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -29,6 +29,7 @@ * SUCH DAMAGE. */ +#include "box/box.h" #include "box/session.h" #include "box/schema.h" #include "sqliteInt.h" @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * with multiple tables and expect an SrcList* parameter * instead of just a Table* parameter. */ - struct Table *table = sql_list_lookup_table(parse, tab_list); - if (table == NULL) - goto delete_from_cleanup; - - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(table->tnum)); - assert(space != NULL); + struct Table *table = NULL; + struct space *space; + uint32_t space_id; /* Figure out if we have any triggers and if the table * being deleted from is a view. */ - struct Trigger *trigger_list = sqlite3TriggersExist(table, TK_DELETE, - NULL, NULL); - - bool is_view = space->def->opts.is_view; + struct Trigger *trigger_list = NULL; /* True if there are triggers or FKs or subqueries in the * WHERE clause. */ - bool is_complex = trigger_list != NULL || - sqlite3FkRequired(table, NULL); + bool is_complex = false; + const char *tab_name = tab_list->a->zName; + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { + space_id = box_space_id_by_name(tab_name, + strlen(tab_name)); + if (space_id == BOX_ID_NIL) + goto delete_from_cleanup; + } else { + table = sql_list_lookup_table(parse, tab_list); + if (table == NULL) + goto delete_from_cleanup; + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); + trigger_list =sqlite3TriggersExist(table, TK_DELETE, + NULL, NULL); + is_complex = trigger_list != NULL || + sqlite3FkRequired(table, NULL); + } + space = space_by_id(space_id); + assert(space != NULL); + + bool is_view = space->def->opts.is_view; /* If table is really a view, make sure it has been * initialized. */ - if (sqlite3ViewGetColumnNames(parse, table)) - goto delete_from_cleanup; + if (is_view) { + if (sqlite3ViewGetColumnNames(parse, table)) + goto delete_from_cleanup; - if (is_view && trigger_list == NULL) { - sqlite3ErrorMsg(parse, "cannot modify %s because it is a view", - space->def->name); - goto delete_from_cleanup; + if (trigger_list == NULL) { + sqlite3ErrorMsg(parse, "cannot modify %s because it is a" + " view", space->def->name); + goto delete_from_cleanup; + } } /* Assign cursor numbers to the table and all its indices. @@ -139,14 +155,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, tab_cursor); } - /* Resolve the column names in the WHERE clause. */ - struct NameContext nc; - memset(&nc, 0, sizeof(nc)); - nc.pParse = parse; - nc.pSrcList = tab_list; - if (sqlite3ResolveExprNames(&nc, where)) - goto delete_from_cleanup; - /* Initialize the counter of the number of rows deleted, * if we are counting rows. */ @@ -162,7 +170,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, if (where == NULL && !is_complex) { assert(!is_view); - sqlite3VdbeAddOp1(v, OP_Clear, table->tnum); + sqlite3VdbeAddOp1(v, OP_Clear, space_id); /* Do not start Tarantool's transaction in case of * truncate optimization. This is workaround until @@ -171,6 +179,13 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, */ parse->initiateTTrans = false; } else { + /* Resolve the column names in the WHERE clause. */ + struct NameContext nc; + memset(&nc, 0, sizeof(nc)); + nc.pParse = parse; + nc.pSrcList = tab_list; + if (sqlite3ResolveExprNames(&nc, where)) + goto delete_from_cleanup; uint16_t wcf = WHERE_ONEPASS_DESIRED | WHERE_DUPLICATES_OK | WHERE_SEEK_TABLE; if (nc.ncFlags & NC_VarSelect) diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 455198d..9acb9b6 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4614,7 +4614,7 @@ case OP_IdxGE: { /* jump */ */ case OP_Clear: { assert(pOp->p1 > 0); - uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1); + uint32_t space_id = pOp->p1; struct space *space = space_by_id(space_id); assert(space != NULL); rc = tarantoolSqlite3ClearTable(space); diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua index 61d7756..810ca8a 100755 --- a/test/sql-tap/delete1.test.lua +++ b/test/sql-tap/delete1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(7) +test:plan(9) --!./tcltestrunner.lua -- ["set","testdir",[["file","dirname",["argv0"]]]] @@ -131,5 +131,26 @@ test:do_test( -- </delete1-5.0> }) +-- Tests for data dictionary integration. +s = box.schema.create_space('t') +i = s:create_index('i', {parts={1, 'unsigned'}}) +test:do_test( + "delete1-6.0", + function() + s:replace({1}) + s:replace({2}) + s:replace({3}) + return s:count() + end, + 3) + +test:do_test( + "delete1-6.1", + function() + box.sql.execute([[delete from "t"]]) + return s:count() + end, + 0) + test:finish_test() -- 2.16.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-18 13:36 [tarantool-patches] [PATCH] sql: refactor SQL delete to allow Lua spaces Kirill Yukhin @ 2018-05-18 21:07 ` Vladislav Shpilevoy 2018-05-21 8:20 ` Kirill Yukhin 0 siblings, 1 reply; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-05-18 21:07 UTC (permalink / raw) To: tarantool-patches, Kirill Yukhin Hello. Thanks for the patch! See 1 comment below. On 18/05/2018 16:36, Kirill Yukhin wrote: > Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-3235-sql-truncate-on-lua-spaces > Issue: https://github.com/tarantool/tarantool/issues/3235 > > This is a first step toward fully-featured deletion > of spaces created in Lua by means of SQL language. > This change to handle most simple case: > DELETE * FROM <space_name> and will be improved in > nearest future. > > Part of #3235 > --- > src/box/sql/analyze.c | 3 +- > src/box/sql/build.c | 3 +- > src/box/sql/delete.c | 69 ++++++++++++++++++++++++++----------------- > src/box/sql/vdbe.c | 2 +- > test/sql-tap/delete1.test.lua | 23 ++++++++++++++- > 5 files changed, 69 insertions(+), 31 deletions(-) > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index f4d248e..dfc91cb 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -29,6 +29,7 @@ > * SUCH DAMAGE. > */ > > +#include "box/box.h" > #include "box/session.h" > #include "box/schema.h" > #include "sqliteInt.h" > @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > + bool is_complex = false; > + const char *tab_name = tab_list->a->zName; > + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { > + space_id = box_space_id_by_name(tab_name, > + strlen(tab_name)); > + if (space_id == BOX_ID_NIL) > + goto delete_from_cleanup; > + } else { > + table = sql_list_lookup_table(parse, tab_list); > + if (table == NULL) > + goto delete_from_cleanup; > + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); > + trigger_list =sqlite3TriggersExist(table, TK_DELETE, > + NULL, NULL); > + is_complex = trigger_list != NULL || > + sqlite3FkRequired(table, NULL); > + } > + space = space_by_id(space_id); > + assert(space != NULL); > + > + bool is_view = space->def->opts.is_view; > > /* If table is really a view, make sure it has been > * initialized. > */ > - if (sqlite3ViewGetColumnNames(parse, table)) > - goto delete_from_cleanup; > + if (is_view) { 1. sqlite3ViewGetColumnNames makes the check too. Lets turn it into assertion inside sqlite3ViewGetColumnNames, and add the 'if (is_view)' in other places, where it is called and the check is necessary. For example, in selectExpander it is checked twice too. > + if (sqlite3ViewGetColumnNames(parse, table)) > + goto delete_from_cleanup; > > > /* Assign cursor numbers to the table and all its indices. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-18 21:07 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-05-21 8:20 ` Kirill Yukhin 2018-05-21 9:47 ` Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Yukhin @ 2018-05-21 8:20 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi Vlad, Thanks a lot for review. My comment is inlined. Updated patch in the bottom. On 19 мая 00:07, Vladislav Shpilevoy wrote: > Hello. Thanks for the patch! See 1 comment below. > > On 18/05/2018 16:36, Kirill Yukhin wrote: > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > > @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > > + bool is_complex = false; > > + const char *tab_name = tab_list->a->zName; > > + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { > > + space_id = box_space_id_by_name(tab_name, > > + strlen(tab_name)); > > + if (space_id == BOX_ID_NIL) > > + goto delete_from_cleanup; > > + } else { > > + table = sql_list_lookup_table(parse, tab_list); > > + if (table == NULL) > > + goto delete_from_cleanup; > > + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); > > + trigger_list =sqlite3TriggersExist(table, TK_DELETE, > > + NULL, NULL); > > + is_complex = trigger_list != NULL || > > + sqlite3FkRequired(table, NULL); > > + } > > + space = space_by_id(space_id); > > + assert(space != NULL); > > + > > + bool is_view = space->def->opts.is_view; > > /* If table is really a view, make sure it has been > > * initialized. > > */ > > - if (sqlite3ViewGetColumnNames(parse, table)) > > - goto delete_from_cleanup; > > + if (is_view) { > > 1. sqlite3ViewGetColumnNames makes the check too. Lets turn it into assertion > inside sqlite3ViewGetColumnNames, and add the 'if (is_view)' in other places, > where it is called and the check is necessary. For example, in selectExpander > it is checked twice too. I've put explicit assert and refactored sqlite3ViewGetColumnNames() to obey Tarantool's coding style. I've also put explicit check at other call sites of the function. -- Regards, Kirill Yukhin commit ea717170e89a01881526bf3b546af6dffa273de1 Author: Kirill Yukhin <kyukhin@tarantool.org> Date: Fri May 18 16:31:24 2018 +0300 sql: refactor SQL delete to allow Lua spaces This is a first step toward fully-featured deletion of spaces created in Lua by means of SQL language. This change to handle most simple case: DELETE * FROM <space_name> and will be improved in nearest future. Also, refactor (sqlite3ViewGetColumnNames()) to use Tarantool's code style and naming convention. Part of #3235 diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 998dd5b..cc594a7 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -169,7 +169,8 @@ openStatTable(Parse * pParse, /* Parsing context */ * The sql_stat[134] table already exists. * Delete all rows. */ - sqlite3VdbeAddOp2(v, OP_Clear, aRoot[i], 0); + sqlite3VdbeAddOp2(v, OP_Clear, + SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0); } } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index df75186..68f81e8 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ } #endif /* SQLITE_OMIT_VIEW */ -#if !defined(SQLITE_OMIT_VIEW) -/* - * The Table structure pTable is really a VIEW. Fill in the names of - * the columns of the view in the pTable structure. Return the number - * of errors. If an error is seen leave an error message in pParse->zErrMsg. - */ -int -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) +bool +sql_view_column_names(struct Parse *parse, struct Table *table) { - Table *pSelTab; /* A fake table from which we get the result set */ - Select *pSel; /* Copy of the SELECT that implements the view */ - int nErr = 0; /* Number of errors encountered */ - int n; /* Temporarily holds the number of cursors assigned */ - sqlite3 *db = pParse->db; /* Database connection for malloc errors */ - - assert(pTable); - -#ifndef SQLITE_OMIT_VIEW - /* A positive nCol means the columns names for this view are - * already known. + assert(table != NULL); + assert(space_is_view(table)); + /* A positive nCol means the columns names for this view + * are already known. */ - if (pTable->nCol > 0) - return 0; + if (table->nCol > 0) + return true; - /* A negative nCol is a special marker meaning that we are currently - * trying to compute the column names. If we enter this routine with - * a negative nCol, it means two or more views form a loop, like this: + /* A negative nCol is a special marker meaning that we are + * currently trying to compute the column names. If we + * enter this routine with a negative nCol, it means two + * or more views form a loop, like this: * * CREATE VIEW one AS SELECT * FROM two; * CREATE VIEW two AS SELECT * FROM one; * - * Actually, the error above is now caught prior to reaching this point. - * But the following test is still important as it does come up - * in the following: + * Actually, the error above is now caught prior to + * reaching this point. But the following test is still + * important as it does come up in the following: * * CREATE TABLE main.ex1(a); * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1; * SELECT * FROM temp.ex1; */ - if (pTable->nCol < 0) { - sqlite3ErrorMsg(pParse, "view %s is circularly defined", - pTable->zName); - return 1; - } - assert(pTable->nCol >= 0); - - /* If we get this far, it means we need to compute the table names. - * Note that the call to sqlite3ResultSetOfSelect() will expand any - * "*" elements in the results set of the view and will assign cursors - * to the elements of the FROM clause. But we do not want these changes - * to be permanent. So the computation is done on a copy of the SELECT - * statement that defines the view. + if (table->nCol < 0) { + sqlite3ErrorMsg(parse, "view %s is circularly defined", + table->zName); + return false; + } + assert(table->nCol >= 0); + + /* If we get this far, it means we need to compute the + * table names. Note that the call to + * sqlite3ResultSetOfSelect() will expand any "*" elements + * in the results set of the view and will assign cursors + * to the elements of the FROM clause. But we do not want + * these changes to be permanent. So the computation is + * done on a copy of the SELECT statement that defines the + * view. */ - assert(pTable->pSelect); - pSel = sqlite3SelectDup(db, pTable->pSelect, 0); - if (pSel) { - n = pParse->nTab; - sqlite3SrcListAssignCursors(pParse, pSel->pSrc); - pTable->nCol = -1; - db->lookaside.bDisable++; - pSelTab = sqlite3ResultSetOfSelect(pParse, pSel); - pParse->nTab = n; - if (pTable->pCheck) { - /* CREATE VIEW name(arglist) AS ... - * The names of the columns in the table are taken from - * arglist which is stored in pTable->pCheck. The pCheck field - * normally holds CHECK constraints on an ordinary table, but for - * a VIEW it holds the list of column names. - */ - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, - &pTable->nCol, - &pTable->aCol); - if (db->mallocFailed == 0 && pParse->nErr == 0 - && pTable->nCol == pSel->pEList->nExpr) { - sqlite3SelectAddColumnTypeAndCollation(pParse, - pTable, - pSel); - } - } else if (pSelTab) { - /* CREATE VIEW name AS... without an argument list. Construct - * the column names from the SELECT statement that defines the view. - */ - assert(pTable->aCol == 0); - pTable->nCol = pSelTab->nCol; - pTable->aCol = pSelTab->aCol; - pSelTab->nCol = 0; - pSelTab->aCol = 0; - } else { - pTable->nCol = 0; - nErr++; + assert(table->pSelect != NULL); + sqlite3 *db = parse->db; + struct Select *select = sqlite3SelectDup(db, table->pSelect, 0); + if (select == NULL) + return false; + int n = parse->nTab; + sqlite3SrcListAssignCursors(parse, select->pSrc); + table->nCol = -1; + db->lookaside.bDisable++; + struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select); + parse->nTab = n; + bool is_ok = true; + if (table->pCheck != NULL) { + /* CREATE VIEW name(arglist) AS ... + * The names of the columns in the table are taken + * from arglist which is stored in table->pCheck. + * The pCheck field normally holds CHECK + * constraints on an ordinary table, but for a + * VIEW it holds the list of column names. + */ + sqlite3ColumnsFromExprList(parse, table->pCheck, + &table->nCol, + &table->aCol); + if (db->mallocFailed == 0 && parse->nErr == 0 + && table->nCol == select->pEList->nExpr) { + sqlite3SelectAddColumnTypeAndCollation(parse, table, + select); } - sqlite3DeleteTable(db, pSelTab); - sqlite3SelectDelete(db, pSel); - db->lookaside.bDisable--; + } else if (sel_tab != NULL) { + /* CREATE VIEW name AS... without an argument + * list. Construct the column names from the + * SELECT statement that defines the view. + */ + assert(table->aCol == NULL); + table->nCol = sel_tab->nCol; + table->aCol = sel_tab->aCol; + sel_tab->nCol = 0; + sel_tab->aCol = NULL; } else { - nErr++; + table->nCol = 0; + is_ok = false; } -#endif /* SQLITE_OMIT_VIEW */ - return nErr; + sqlite3DeleteTable(db, sel_tab); + sqlite3SelectDelete(db, select); + db->lookaside.bDisable--; + + return is_ok; } -#endif /* !defined(SQLITE_OMIT_VIEW) */ #ifndef SQLITE_OMIT_VIEW /* @@ -2672,7 +2662,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) VdbeCoverage(v); sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) - sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); + sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum), + 0); emit_open_cursor(pParse, iIdx, tnum); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR | ((memRootPage >= 0) ? diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index f4d248e..e073f0c 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -29,6 +29,7 @@ * SUCH DAMAGE. */ +#include "box/box.h" #include "box/session.h" #include "box/schema.h" #include "sqliteInt.h" @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * with multiple tables and expect an SrcList* parameter * instead of just a Table* parameter. */ - struct Table *table = sql_list_lookup_table(parse, tab_list); - if (table == NULL) - goto delete_from_cleanup; - - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(table->tnum)); - assert(space != NULL); + struct Table *table = NULL; + struct space *space; + uint32_t space_id; /* Figure out if we have any triggers and if the table * being deleted from is a view. */ - struct Trigger *trigger_list = sqlite3TriggersExist(table, TK_DELETE, - NULL, NULL); - - bool is_view = space->def->opts.is_view; + struct Trigger *trigger_list = NULL; /* True if there are triggers or FKs or subqueries in the * WHERE clause. */ - bool is_complex = trigger_list != NULL || - sqlite3FkRequired(table, NULL); + bool is_complex = false; + const char *tab_name = tab_list->a->zName; + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { + space_id = box_space_id_by_name(tab_name, + strlen(tab_name)); + if (space_id == BOX_ID_NIL) + goto delete_from_cleanup; + } else { + table = sql_list_lookup_table(parse, tab_list); + if (table == NULL) + goto delete_from_cleanup; + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); + trigger_list =sqlite3TriggersExist(table, TK_DELETE, + NULL, NULL); + is_complex = trigger_list != NULL || + sqlite3FkRequired(table, NULL); + } + space = space_by_id(space_id); + assert(space != NULL); + + bool is_view = space->def->opts.is_view; /* If table is really a view, make sure it has been * initialized. */ - if (sqlite3ViewGetColumnNames(parse, table)) - goto delete_from_cleanup; + if (is_view) { + if (!sql_view_column_names(parse, table)) + goto delete_from_cleanup; - if (is_view && trigger_list == NULL) { - sqlite3ErrorMsg(parse, "cannot modify %s because it is a view", - space->def->name); - goto delete_from_cleanup; + if (trigger_list == NULL) { + sqlite3ErrorMsg(parse, "cannot modify %s because it is a" + " view", space->def->name); + goto delete_from_cleanup; + } } /* Assign cursor numbers to the table and all its indices. @@ -139,14 +155,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, tab_cursor); } - /* Resolve the column names in the WHERE clause. */ - struct NameContext nc; - memset(&nc, 0, sizeof(nc)); - nc.pParse = parse; - nc.pSrcList = tab_list; - if (sqlite3ResolveExprNames(&nc, where)) - goto delete_from_cleanup; - /* Initialize the counter of the number of rows deleted, * if we are counting rows. */ @@ -162,7 +170,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, if (where == NULL && !is_complex) { assert(!is_view); - sqlite3VdbeAddOp1(v, OP_Clear, table->tnum); + sqlite3VdbeAddOp1(v, OP_Clear, space_id); /* Do not start Tarantool's transaction in case of * truncate optimization. This is workaround until @@ -171,6 +179,13 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, */ parse->initiateTTrans = false; } else { + /* Resolve the column names in the WHERE clause. */ + struct NameContext nc; + memset(&nc, 0, sizeof(nc)); + nc.pParse = parse; + nc.pSrcList = tab_list; + if (sqlite3ResolveExprNames(&nc, where)) + goto delete_from_cleanup; uint16_t wcf = WHERE_ONEPASS_DESIRED | WHERE_DUPLICATES_OK | WHERE_SEEK_TABLE; if (nc.ncFlags & NC_VarSelect) diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index e23cffd..9e37def 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -349,11 +349,9 @@ sqlite3Insert(Parse * pParse, /* Parser context */ int *aRegIdx = 0; /* One register allocated to each index */ uint32_t space_id = 0; -#ifndef SQLITE_OMIT_TRIGGER - int isView; /* True if attempting to insert into a view */ + bool is_view; /* True if attempting to insert into a view */ Trigger *pTrigger; /* List of triggers on pTab, if required */ int tmask; /* Mask of trigger times */ -#endif db = pParse->db; memset(&dest, 0, sizeof(dest)); @@ -389,28 +387,18 @@ sqlite3Insert(Parse * pParse, /* Parser context */ /* Figure out if we have any triggers and if the table being * inserted into is a view */ -#ifndef SQLITE_OMIT_TRIGGER pTrigger = sqlite3TriggersExist(pTab, TK_INSERT, 0, &tmask); - isView = space_is_view(pTab); -#else -#define pTrigger 0 -#define tmask 0 -#define isView 0 -#endif -#ifdef SQLITE_OMIT_VIEW -#undef isView -#define isView 0 -#endif + is_view = space_is_view(pTab); assert((pTrigger && tmask) || (pTrigger == 0 && tmask == 0)); /* If pTab is really a view, make sure it has been initialized. * ViewGetColumnNames() is a no-op if pTab is not a view. */ - if (sqlite3ViewGetColumnNames(pParse, pTab)) + if (is_view && !sql_view_column_names(pParse, pTab)) goto insert_cleanup; /* Cannot insert into a read-only table. */ - if (isView && tmask == 0) { + if (is_view && tmask == 0) { sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", pTab->zName); goto insert_cleanup; @@ -481,7 +469,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ bIdListInOrder = 0; if (j == pTab->iPKey) { ipkColumn = i; - assert(isView); + assert(is_view); } break; } @@ -637,7 +625,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } /* If this is not a view, open the table and and all indices */ - if (!isView) { + if (!is_view) { int nIdx; nIdx = sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0, @@ -731,7 +719,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ * If this is a real table, attempt conversions as required by the * table column affinities. */ - if (!isView) { + if (!is_view) { sqlite3TableAffinity(v, pTab, regCols + 1); } @@ -747,7 +735,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ /* Compute the content of the next row to insert into a range of * registers beginning at regIns. */ - if (!isView) { + if (!is_view) { if (ipkColumn >= 0) { if (useTempTable) { sqlite3VdbeAddOp3(v, OP_Column, srcTab, @@ -930,20 +918,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */ sqlite3DbFree(db, aRegIdx); } -/* Make sure "isView" and other macros defined above are undefined. Otherwise - * they may interfere with compilation of other functions in this file - * (or in another file, if this file becomes part of the amalgamation). - */ -#ifdef isView -#undef isView -#endif -#ifdef pTrigger -#undef pTrigger -#endif -#ifdef tmask -#undef tmask -#endif - /* * Meanings of bits in of pWalker->eCode for checkConstraintUnchanged() */ diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index e4ceec2..634da87 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -357,7 +357,8 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ Column *pCol; Index *pPk = sqlite3PrimaryKeyIndex(pTab); pParse->nMem = 6; - sqlite3ViewGetColumnNames(pParse, pTab); + if (space_is_view(pTab)) + sql_view_column_names(pParse, pTab); for (i = 0, pCol = pTab->aCol; i < pTab->nCol; i++, pCol++) { if (!table_column_is_in_pk(pTab, i)) { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index de111d3..3ae40f1 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -4680,7 +4680,7 @@ selectExpander(Walker * pWalker, Select * p) #if !defined(SQLITE_OMIT_VIEW) if (space_is_view(pTab)) { i16 nCol; - if (sqlite3ViewGetColumnNames(pParse, pTab)) + if (!sql_view_column_names(pParse, pTab)) return WRC_Abort; assert(pFrom->pSelect == 0); pFrom->pSelect = diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index ab50c3e..1c17350 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3592,11 +3592,18 @@ int sqlite3FaultSim(int); void sqlite3CreateView(Parse *, Token *, Token *, ExprList *, Select *, int); -#if !defined(SQLITE_OMIT_VIEW) -int sqlite3ViewGetColumnNames(Parse *, Table *); -#else -#define sqlite3ViewGetColumnNames(A,B) 0 -#endif +/** + * The Table structure pTable is really a VIEW. Fill in the names + * of the columns of the view in the table structure. Return the + * number of errors. If an error is seen leave an error message + * in parse->zErrMsg. + * + * @param parse Parsing context. + * @param table Tables to process. + * @retval True if success. + */ +bool +sql_view_column_names(struct Parse *parse, struct Table *table); void sql_drop_table(struct Parse *, struct SrcList *, bool, bool); diff --git a/src/box/sql/update.c b/src/box/sql/update.c index c521a3b..ec240d3 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -137,11 +137,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ int labelContinue; /* Jump here to continue next step of UPDATE loop */ struct session *user_session = current_session(); -#ifndef SQLITE_OMIT_TRIGGER - int isView; /* True when updating a view (INSTEAD OF trigger) */ + bool is_view; /* True when updating a view (INSTEAD OF trigger) */ Trigger *pTrigger; /* List of triggers on pTab, if required */ int tmask; /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */ -#endif int newmask; /* Mask of NEW.* columns accessed by BEFORE triggers */ int iEph = 0; /* Ephemeral table holding all primary key values */ int nKey = 0; /* Number of elements in regKey */ @@ -170,24 +168,14 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* Figure out if we have any triggers and if the table being * updated is a view. */ -#ifndef SQLITE_OMIT_TRIGGER pTrigger = sqlite3TriggersExist(pTab, TK_UPDATE, pChanges, &tmask); - isView = space_is_view(pTab); + is_view = space_is_view(pTab); assert(pTrigger || tmask == 0); -#else -#define pTrigger 0 -#define isView 0 -#define tmask 0 -#endif -#ifdef SQLITE_OMIT_VIEW -#undef isView -#define isView 0 -#endif - if (sqlite3ViewGetColumnNames(pParse, pTab)) { + if (is_view && !sql_view_column_names(pParse, pTab)) { goto update_cleanup; } - if (isView && tmask == 0) { + if (is_view && tmask == 0) { sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", pTab->zName); goto update_cleanup; @@ -200,7 +188,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ */ pTabList->a[0].iCursor = iBaseCur = iDataCur = pParse->nTab++; iIdxCur = iDataCur + 1; - pPk = isView ? 0 : sqlite3PrimaryKeyIndex(pTab); + pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab); for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) { if (IsPrimaryKeyIndex(pIdx) && pPk != 0) { iDataCur = pParse->nTab; @@ -324,13 +312,11 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* If we are trying to update a view, realize that view into * an ephemeral table. */ -#if !defined(SQLITE_OMIT_VIEW) && !defined(SQLITE_OMIT_TRIGGER) - if (isView) { + if (is_view) { sql_materialize_view(pParse, pTab->zName, pWhere, iDataCur); /* Number of columns from SELECT plus ID.*/ nKey = pTab->nCol + 1; } -#endif /* Resolve the column names in all the expressions in the * WHERE clause. @@ -347,7 +333,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ i16 nPk; /* Number of components of the PRIMARY KEY */ int addrOpen; /* Address of the OpenEphemeral instruction */ - if (isView) { + if (is_view) { nPk = nKey; } else { assert(pPk != 0); @@ -359,7 +345,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ iEph = pParse->nTab++; sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); - if (isView) { + if (is_view) { addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, nKey); } else { @@ -372,7 +358,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ if (pWInfo == 0) goto update_cleanup; okOnePass = sqlite3WhereOkOnePass(pWInfo, aiCurOnePass); - if (isView){ + if (is_view) { for (i = 0; i < nPk; i++) { sqlite3VdbeAddOp3(v, OP_Column, iDataCur, i, iPk + i); } @@ -390,7 +376,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ nKey = nPk; regKey = iPk; } else { - const char *zAff = isView ? 0 : + const char *zAff = is_view ? 0 : sqlite3IndexAffinityStr(pParse->db, pPk); sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey, zAff, nPk); @@ -412,7 +398,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } labelBreak = sqlite3VdbeMakeLabel(v); - if (!isView) { + if (!is_view) { /* * Open every index that needs updating. Note that if any * index could potentially invoke a REPLACE conflict resolution @@ -445,7 +431,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ labelContinue = labelBreak; sqlite3VdbeAddOp2(v, OP_IsNull, regKey, labelBreak); - if (aToOpen[iDataCur - iBaseCur] && !isView) { + if (aToOpen[iDataCur - iBaseCur] && !is_view) { assert(pPk); sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelBreak, regKey, nKey); @@ -552,7 +538,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ * is deleted or renamed by a BEFORE trigger - is left undefined in the * documentation. */ - if (!isView) { + if (!is_view) { sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelContinue, regKey, nKey); VdbeCoverage(v); @@ -576,7 +562,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } } - if (!isView) { + if (!is_view) { int addr1 = 0; /* Address of jump instruction */ int bReplace = 0; /* True if REPLACE conflict resolution might happen */ @@ -691,15 +677,3 @@ sqlite3Update(Parse * pParse, /* The parser context */ sql_expr_free(db, pWhere, false); return; } - -/* Make sure "isView" and other macros defined above are undefined. Otherwise - * they may interfere with compilation of other functions in this file - * (or in another file, if this file becomes part of the amalgamation). - */ -#ifdef isView -#undef isView -#endif -#ifdef pTrigger -#undef pTrigger -#endif - diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 455198d..9acb9b6 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4614,7 +4614,7 @@ case OP_IdxGE: { /* jump */ */ case OP_Clear: { assert(pOp->p1 > 0); - uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1); + uint32_t space_id = pOp->p1; struct space *space = space_by_id(space_id); assert(space != NULL); rc = tarantoolSqlite3ClearTable(space); diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua index 61d7756..810ca8a 100755 --- a/test/sql-tap/delete1.test.lua +++ b/test/sql-tap/delete1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(7) +test:plan(9) --!./tcltestrunner.lua -- ["set","testdir",[["file","dirname",["argv0"]]]] @@ -131,5 +131,26 @@ test:do_test( -- </delete1-5.0> }) +-- Tests for data dictionary integration. +s = box.schema.create_space('t') +i = s:create_index('i', {parts={1, 'unsigned'}}) +test:do_test( + "delete1-6.0", + function() + s:replace({1}) + s:replace({2}) + s:replace({3}) + return s:count() + end, + 3) + +test:do_test( + "delete1-6.1", + function() + box.sql.execute([[delete from "t"]]) + return s:count() + end, + 0) + test:finish_test() ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-21 8:20 ` Kirill Yukhin @ 2018-05-21 9:47 ` Vladislav Shpilevoy 2018-05-21 10:25 ` Kirill Yukhin 0 siblings, 1 reply; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-05-21 9:47 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches Hello. Thanks for the fixes! > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index df75186..68f81e8 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ > } > #endif /* SQLITE_OMIT_VIEW */ > > -#if !defined(SQLITE_OMIT_VIEW) > -/* > - * The Table structure pTable is really a VIEW. Fill in the names of > - * the columns of the view in the pTable structure. Return the number > - * of errors. If an error is seen leave an error message in pParse->zErrMsg. > - */ > -int > -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > +bool In Tarantool code style we do not return boolean on non-check functions. 0 or -1, please. The rest of patch is ok. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-21 9:47 ` Vladislav Shpilevoy @ 2018-05-21 10:25 ` Kirill Yukhin 2018-05-21 10:49 ` Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Yukhin @ 2018-05-21 10:25 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches On 21 мая 12:47, Vladislav Shpilevoy wrote: > Hello. Thanks for the fixes! > > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > > index df75186..68f81e8 100644 > > --- a/src/box/sql/build.c > > +++ b/src/box/sql/build.c > > @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ > > -int > > -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > > +bool > > In Tarantool code style we do not return boolean on non-check functions. > 0 or -1, please. Done. -- Regards, Kirill Yukhin commit ab5fabef4625312bfe5534c28487aca139d3d658 Author: Kirill Yukhin <kyukhin@tarantool.org> Date: Fri May 18 16:31:24 2018 +0300 sql: refactor SQL delete to allow Lua spaces This is a first step toward fully-featured deletion of spaces created in Lua by means of SQL language. This change to handle most simple case: DELETE * FROM <space_name> and will be improved in nearest future. Also, refactor (sqlite3ViewGetColumnNames()) to use Tarantool's code style and naming convention. Part of #3235 diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 998dd5b..cc594a7 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -169,7 +169,8 @@ openStatTable(Parse * pParse, /* Parsing context */ * The sql_stat[134] table already exists. * Delete all rows. */ - sqlite3VdbeAddOp2(v, OP_Clear, aRoot[i], 0); + sqlite3VdbeAddOp2(v, OP_Clear, + SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0); } } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index df75186..f59087b 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ } #endif /* SQLITE_OMIT_VIEW */ -#if !defined(SQLITE_OMIT_VIEW) -/* - * The Table structure pTable is really a VIEW. Fill in the names of - * the columns of the view in the pTable structure. Return the number - * of errors. If an error is seen leave an error message in pParse->zErrMsg. - */ int -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) +sql_view_column_names(struct Parse *parse, struct Table *table) { - Table *pSelTab; /* A fake table from which we get the result set */ - Select *pSel; /* Copy of the SELECT that implements the view */ - int nErr = 0; /* Number of errors encountered */ - int n; /* Temporarily holds the number of cursors assigned */ - sqlite3 *db = pParse->db; /* Database connection for malloc errors */ - - assert(pTable); - -#ifndef SQLITE_OMIT_VIEW - /* A positive nCol means the columns names for this view are - * already known. + assert(table != NULL); + assert(space_is_view(table)); + /* A positive nCol means the columns names for this view + * are already known. */ - if (pTable->nCol > 0) - return 0; + if (table->nCol > 0) + return true; - /* A negative nCol is a special marker meaning that we are currently - * trying to compute the column names. If we enter this routine with - * a negative nCol, it means two or more views form a loop, like this: + /* A negative nCol is a special marker meaning that we are + * currently trying to compute the column names. If we + * enter this routine with a negative nCol, it means two + * or more views form a loop, like this: * * CREATE VIEW one AS SELECT * FROM two; * CREATE VIEW two AS SELECT * FROM one; * - * Actually, the error above is now caught prior to reaching this point. - * But the following test is still important as it does come up - * in the following: + * Actually, the error above is now caught prior to + * reaching this point. But the following test is still + * important as it does come up in the following: * * CREATE TABLE main.ex1(a); * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1; * SELECT * FROM temp.ex1; */ - if (pTable->nCol < 0) { - sqlite3ErrorMsg(pParse, "view %s is circularly defined", - pTable->zName); - return 1; - } - assert(pTable->nCol >= 0); - - /* If we get this far, it means we need to compute the table names. - * Note that the call to sqlite3ResultSetOfSelect() will expand any - * "*" elements in the results set of the view and will assign cursors - * to the elements of the FROM clause. But we do not want these changes - * to be permanent. So the computation is done on a copy of the SELECT - * statement that defines the view. + if (table->nCol < 0) { + sqlite3ErrorMsg(parse, "view %s is circularly defined", + table->zName); + return false; + } + assert(table->nCol >= 0); + + /* If we get this far, it means we need to compute the + * table names. Note that the call to + * sqlite3ResultSetOfSelect() will expand any "*" elements + * in the results set of the view and will assign cursors + * to the elements of the FROM clause. But we do not want + * these changes to be permanent. So the computation is + * done on a copy of the SELECT statement that defines the + * view. */ - assert(pTable->pSelect); - pSel = sqlite3SelectDup(db, pTable->pSelect, 0); - if (pSel) { - n = pParse->nTab; - sqlite3SrcListAssignCursors(pParse, pSel->pSrc); - pTable->nCol = -1; - db->lookaside.bDisable++; - pSelTab = sqlite3ResultSetOfSelect(pParse, pSel); - pParse->nTab = n; - if (pTable->pCheck) { - /* CREATE VIEW name(arglist) AS ... - * The names of the columns in the table are taken from - * arglist which is stored in pTable->pCheck. The pCheck field - * normally holds CHECK constraints on an ordinary table, but for - * a VIEW it holds the list of column names. - */ - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, - &pTable->nCol, - &pTable->aCol); - if (db->mallocFailed == 0 && pParse->nErr == 0 - && pTable->nCol == pSel->pEList->nExpr) { - sqlite3SelectAddColumnTypeAndCollation(pParse, - pTable, - pSel); - } - } else if (pSelTab) { - /* CREATE VIEW name AS... without an argument list. Construct - * the column names from the SELECT statement that defines the view. - */ - assert(pTable->aCol == 0); - pTable->nCol = pSelTab->nCol; - pTable->aCol = pSelTab->aCol; - pSelTab->nCol = 0; - pSelTab->aCol = 0; - } else { - pTable->nCol = 0; - nErr++; + assert(table->pSelect != NULL); + sqlite3 *db = parse->db; + struct Select *select = sqlite3SelectDup(db, table->pSelect, 0); + if (select == NULL) + return false; + int n = parse->nTab; + sqlite3SrcListAssignCursors(parse, select->pSrc); + table->nCol = -1; + db->lookaside.bDisable++; + struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select); + parse->nTab = n; + bool is_ok = true; + if (table->pCheck != NULL) { + /* CREATE VIEW name(arglist) AS ... + * The names of the columns in the table are taken + * from arglist which is stored in table->pCheck. + * The pCheck field normally holds CHECK + * constraints on an ordinary table, but for a + * VIEW it holds the list of column names. + */ + sqlite3ColumnsFromExprList(parse, table->pCheck, + &table->nCol, + &table->aCol); + if (db->mallocFailed == 0 && parse->nErr == 0 + && table->nCol == select->pEList->nExpr) { + sqlite3SelectAddColumnTypeAndCollation(parse, table, + select); } - sqlite3DeleteTable(db, pSelTab); - sqlite3SelectDelete(db, pSel); - db->lookaside.bDisable--; + } else if (sel_tab != NULL) { + /* CREATE VIEW name AS... without an argument + * list. Construct the column names from the + * SELECT statement that defines the view. + */ + assert(table->aCol == NULL); + table->nCol = sel_tab->nCol; + table->aCol = sel_tab->aCol; + sel_tab->nCol = 0; + sel_tab->aCol = NULL; } else { - nErr++; + table->nCol = 0; + is_ok = false; } -#endif /* SQLITE_OMIT_VIEW */ - return nErr; + sqlite3DeleteTable(db, sel_tab); + sqlite3SelectDelete(db, select); + db->lookaside.bDisable--; + + return is_ok; } -#endif /* !defined(SQLITE_OMIT_VIEW) */ #ifndef SQLITE_OMIT_VIEW /* @@ -2672,7 +2662,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) VdbeCoverage(v); sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) - sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); + sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum), + 0); emit_open_cursor(pParse, iIdx, tnum); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR | ((memRootPage >= 0) ? diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index f4d248e..8c7cd7d 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -29,6 +29,7 @@ * SUCH DAMAGE. */ +#include "box/box.h" #include "box/session.h" #include "box/schema.h" #include "sqliteInt.h" @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * with multiple tables and expect an SrcList* parameter * instead of just a Table* parameter. */ - struct Table *table = sql_list_lookup_table(parse, tab_list); - if (table == NULL) - goto delete_from_cleanup; - - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(table->tnum)); - assert(space != NULL); + struct Table *table = NULL; + struct space *space; + uint32_t space_id; /* Figure out if we have any triggers and if the table * being deleted from is a view. */ - struct Trigger *trigger_list = sqlite3TriggersExist(table, TK_DELETE, - NULL, NULL); - - bool is_view = space->def->opts.is_view; + struct Trigger *trigger_list = NULL; /* True if there are triggers or FKs or subqueries in the * WHERE clause. */ - bool is_complex = trigger_list != NULL || - sqlite3FkRequired(table, NULL); + bool is_complex = false; + const char *tab_name = tab_list->a->zName; + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { + space_id = box_space_id_by_name(tab_name, + strlen(tab_name)); + if (space_id == BOX_ID_NIL) + goto delete_from_cleanup; + } else { + table = sql_list_lookup_table(parse, tab_list); + if (table == NULL) + goto delete_from_cleanup; + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); + trigger_list =sqlite3TriggersExist(table, TK_DELETE, + NULL, NULL); + is_complex = trigger_list != NULL || + sqlite3FkRequired(table, NULL); + } + space = space_by_id(space_id); + assert(space != NULL); + + bool is_view = space->def->opts.is_view; /* If table is really a view, make sure it has been * initialized. */ - if (sqlite3ViewGetColumnNames(parse, table)) - goto delete_from_cleanup; + if (is_view) { + if (sql_view_column_names(parse, table) != 0) + goto delete_from_cleanup; - if (is_view && trigger_list == NULL) { - sqlite3ErrorMsg(parse, "cannot modify %s because it is a view", - space->def->name); - goto delete_from_cleanup; + if (trigger_list == NULL) { + sqlite3ErrorMsg(parse, "cannot modify %s because it is a" + " view", space->def->name); + goto delete_from_cleanup; + } } /* Assign cursor numbers to the table and all its indices. @@ -139,14 +155,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, tab_cursor); } - /* Resolve the column names in the WHERE clause. */ - struct NameContext nc; - memset(&nc, 0, sizeof(nc)); - nc.pParse = parse; - nc.pSrcList = tab_list; - if (sqlite3ResolveExprNames(&nc, where)) - goto delete_from_cleanup; - /* Initialize the counter of the number of rows deleted, * if we are counting rows. */ @@ -162,7 +170,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, if (where == NULL && !is_complex) { assert(!is_view); - sqlite3VdbeAddOp1(v, OP_Clear, table->tnum); + sqlite3VdbeAddOp1(v, OP_Clear, space_id); /* Do not start Tarantool's transaction in case of * truncate optimization. This is workaround until @@ -171,6 +179,13 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, */ parse->initiateTTrans = false; } else { + /* Resolve the column names in the WHERE clause. */ + struct NameContext nc; + memset(&nc, 0, sizeof(nc)); + nc.pParse = parse; + nc.pSrcList = tab_list; + if (sqlite3ResolveExprNames(&nc, where)) + goto delete_from_cleanup; uint16_t wcf = WHERE_ONEPASS_DESIRED | WHERE_DUPLICATES_OK | WHERE_SEEK_TABLE; if (nc.ncFlags & NC_VarSelect) diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index e23cffd..5d62e58 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -349,11 +349,9 @@ sqlite3Insert(Parse * pParse, /* Parser context */ int *aRegIdx = 0; /* One register allocated to each index */ uint32_t space_id = 0; -#ifndef SQLITE_OMIT_TRIGGER - int isView; /* True if attempting to insert into a view */ + bool is_view; /* True if attempting to insert into a view */ Trigger *pTrigger; /* List of triggers on pTab, if required */ int tmask; /* Mask of trigger times */ -#endif db = pParse->db; memset(&dest, 0, sizeof(dest)); @@ -389,28 +387,18 @@ sqlite3Insert(Parse * pParse, /* Parser context */ /* Figure out if we have any triggers and if the table being * inserted into is a view */ -#ifndef SQLITE_OMIT_TRIGGER pTrigger = sqlite3TriggersExist(pTab, TK_INSERT, 0, &tmask); - isView = space_is_view(pTab); -#else -#define pTrigger 0 -#define tmask 0 -#define isView 0 -#endif -#ifdef SQLITE_OMIT_VIEW -#undef isView -#define isView 0 -#endif + is_view = space_is_view(pTab); assert((pTrigger && tmask) || (pTrigger == 0 && tmask == 0)); /* If pTab is really a view, make sure it has been initialized. * ViewGetColumnNames() is a no-op if pTab is not a view. */ - if (sqlite3ViewGetColumnNames(pParse, pTab)) + if (is_view && sql_view_column_names(pParse, pTab) != 0) goto insert_cleanup; /* Cannot insert into a read-only table. */ - if (isView && tmask == 0) { + if (is_view && tmask == 0) { sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", pTab->zName); goto insert_cleanup; @@ -481,7 +469,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ bIdListInOrder = 0; if (j == pTab->iPKey) { ipkColumn = i; - assert(isView); + assert(is_view); } break; } @@ -637,7 +625,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } /* If this is not a view, open the table and and all indices */ - if (!isView) { + if (!is_view) { int nIdx; nIdx = sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0, @@ -731,7 +719,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ * If this is a real table, attempt conversions as required by the * table column affinities. */ - if (!isView) { + if (!is_view) { sqlite3TableAffinity(v, pTab, regCols + 1); } @@ -747,7 +735,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ /* Compute the content of the next row to insert into a range of * registers beginning at regIns. */ - if (!isView) { + if (!is_view) { if (ipkColumn >= 0) { if (useTempTable) { sqlite3VdbeAddOp3(v, OP_Column, srcTab, @@ -930,20 +918,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */ sqlite3DbFree(db, aRegIdx); } -/* Make sure "isView" and other macros defined above are undefined. Otherwise - * they may interfere with compilation of other functions in this file - * (or in another file, if this file becomes part of the amalgamation). - */ -#ifdef isView -#undef isView -#endif -#ifdef pTrigger -#undef pTrigger -#endif -#ifdef tmask -#undef tmask -#endif - /* * Meanings of bits in of pWalker->eCode for checkConstraintUnchanged() */ diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index e4ceec2..634da87 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -357,7 +357,8 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ Column *pCol; Index *pPk = sqlite3PrimaryKeyIndex(pTab); pParse->nMem = 6; - sqlite3ViewGetColumnNames(pParse, pTab); + if (space_is_view(pTab)) + sql_view_column_names(pParse, pTab); for (i = 0, pCol = pTab->aCol; i < pTab->nCol; i++, pCol++) { if (!table_column_is_in_pk(pTab, i)) { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index de111d3..e4d4013 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -4680,7 +4680,7 @@ selectExpander(Walker * pWalker, Select * p) #if !defined(SQLITE_OMIT_VIEW) if (space_is_view(pTab)) { i16 nCol; - if (sqlite3ViewGetColumnNames(pParse, pTab)) + if (sql_view_column_names(pParse, pTab) != 0) return WRC_Abort; assert(pFrom->pSelect == 0); pFrom->pSelect = diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index ab50c3e..26db466 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3592,11 +3592,18 @@ int sqlite3FaultSim(int); void sqlite3CreateView(Parse *, Token *, Token *, ExprList *, Select *, int); -#if !defined(SQLITE_OMIT_VIEW) -int sqlite3ViewGetColumnNames(Parse *, Table *); -#else -#define sqlite3ViewGetColumnNames(A,B) 0 -#endif +/** + * The Table structure pTable is really a VIEW. Fill in the names + * of the columns of the view in the table structure. Return the + * number of errors. If an error is seen leave an error message + * in parse->zErrMsg. + * + * @param parse Parsing context. + * @param table Tables to process. + * @retval 0 if success, -1 in case of error. + */ +int +sql_view_column_names(struct Parse *parse, struct Table *table); void sql_drop_table(struct Parse *, struct SrcList *, bool, bool); diff --git a/src/box/sql/update.c b/src/box/sql/update.c index c521a3b..4d6d0e1 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -137,11 +137,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ int labelContinue; /* Jump here to continue next step of UPDATE loop */ struct session *user_session = current_session(); -#ifndef SQLITE_OMIT_TRIGGER - int isView; /* True when updating a view (INSTEAD OF trigger) */ + bool is_view; /* True when updating a view (INSTEAD OF trigger) */ Trigger *pTrigger; /* List of triggers on pTab, if required */ int tmask; /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */ -#endif int newmask; /* Mask of NEW.* columns accessed by BEFORE triggers */ int iEph = 0; /* Ephemeral table holding all primary key values */ int nKey = 0; /* Number of elements in regKey */ @@ -170,24 +168,14 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* Figure out if we have any triggers and if the table being * updated is a view. */ -#ifndef SQLITE_OMIT_TRIGGER pTrigger = sqlite3TriggersExist(pTab, TK_UPDATE, pChanges, &tmask); - isView = space_is_view(pTab); + is_view = space_is_view(pTab); assert(pTrigger || tmask == 0); -#else -#define pTrigger 0 -#define isView 0 -#define tmask 0 -#endif -#ifdef SQLITE_OMIT_VIEW -#undef isView -#define isView 0 -#endif - if (sqlite3ViewGetColumnNames(pParse, pTab)) { + if (is_view && sql_view_column_names(pParse, pTab) != 0) { goto update_cleanup; } - if (isView && tmask == 0) { + if (is_view && tmask == 0) { sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", pTab->zName); goto update_cleanup; @@ -200,7 +188,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ */ pTabList->a[0].iCursor = iBaseCur = iDataCur = pParse->nTab++; iIdxCur = iDataCur + 1; - pPk = isView ? 0 : sqlite3PrimaryKeyIndex(pTab); + pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab); for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) { if (IsPrimaryKeyIndex(pIdx) && pPk != 0) { iDataCur = pParse->nTab; @@ -324,13 +312,11 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* If we are trying to update a view, realize that view into * an ephemeral table. */ -#if !defined(SQLITE_OMIT_VIEW) && !defined(SQLITE_OMIT_TRIGGER) - if (isView) { + if (is_view) { sql_materialize_view(pParse, pTab->zName, pWhere, iDataCur); /* Number of columns from SELECT plus ID.*/ nKey = pTab->nCol + 1; } -#endif /* Resolve the column names in all the expressions in the * WHERE clause. @@ -347,7 +333,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ i16 nPk; /* Number of components of the PRIMARY KEY */ int addrOpen; /* Address of the OpenEphemeral instruction */ - if (isView) { + if (is_view) { nPk = nKey; } else { assert(pPk != 0); @@ -359,7 +345,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ iEph = pParse->nTab++; sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); - if (isView) { + if (is_view) { addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, nKey); } else { @@ -372,7 +358,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ if (pWInfo == 0) goto update_cleanup; okOnePass = sqlite3WhereOkOnePass(pWInfo, aiCurOnePass); - if (isView){ + if (is_view) { for (i = 0; i < nPk; i++) { sqlite3VdbeAddOp3(v, OP_Column, iDataCur, i, iPk + i); } @@ -390,7 +376,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ nKey = nPk; regKey = iPk; } else { - const char *zAff = isView ? 0 : + const char *zAff = is_view ? 0 : sqlite3IndexAffinityStr(pParse->db, pPk); sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey, zAff, nPk); @@ -412,7 +398,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } labelBreak = sqlite3VdbeMakeLabel(v); - if (!isView) { + if (!is_view) { /* * Open every index that needs updating. Note that if any * index could potentially invoke a REPLACE conflict resolution @@ -445,7 +431,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ labelContinue = labelBreak; sqlite3VdbeAddOp2(v, OP_IsNull, regKey, labelBreak); - if (aToOpen[iDataCur - iBaseCur] && !isView) { + if (aToOpen[iDataCur - iBaseCur] && !is_view) { assert(pPk); sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelBreak, regKey, nKey); @@ -552,7 +538,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ * is deleted or renamed by a BEFORE trigger - is left undefined in the * documentation. */ - if (!isView) { + if (!is_view) { sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelContinue, regKey, nKey); VdbeCoverage(v); @@ -576,7 +562,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } } - if (!isView) { + if (!is_view) { int addr1 = 0; /* Address of jump instruction */ int bReplace = 0; /* True if REPLACE conflict resolution might happen */ @@ -691,15 +677,3 @@ sqlite3Update(Parse * pParse, /* The parser context */ sql_expr_free(db, pWhere, false); return; } - -/* Make sure "isView" and other macros defined above are undefined. Otherwise - * they may interfere with compilation of other functions in this file - * (or in another file, if this file becomes part of the amalgamation). - */ -#ifdef isView -#undef isView -#endif -#ifdef pTrigger -#undef pTrigger -#endif - diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 455198d..9acb9b6 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4614,7 +4614,7 @@ case OP_IdxGE: { /* jump */ */ case OP_Clear: { assert(pOp->p1 > 0); - uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1); + uint32_t space_id = pOp->p1; struct space *space = space_by_id(space_id); assert(space != NULL); rc = tarantoolSqlite3ClearTable(space); diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua index 61d7756..810ca8a 100755 --- a/test/sql-tap/delete1.test.lua +++ b/test/sql-tap/delete1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(7) +test:plan(9) --!./tcltestrunner.lua -- ["set","testdir",[["file","dirname",["argv0"]]]] @@ -131,5 +131,26 @@ test:do_test( -- </delete1-5.0> }) +-- Tests for data dictionary integration. +s = box.schema.create_space('t') +i = s:create_index('i', {parts={1, 'unsigned'}}) +test:do_test( + "delete1-6.0", + function() + s:replace({1}) + s:replace({2}) + s:replace({3}) + return s:count() + end, + 3) + +test:do_test( + "delete1-6.1", + function() + box.sql.execute([[delete from "t"]]) + return s:count() + end, + 0) + test:finish_test() ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-21 10:25 ` Kirill Yukhin @ 2018-05-21 10:49 ` Vladislav Shpilevoy 2018-05-21 11:24 ` Kirill Yukhin 0 siblings, 1 reply; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-05-21 10:49 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches On 21/05/2018 13:25, Kirill Yukhin wrote: > On 21 мая 12:47, Vladislav Shpilevoy wrote: >> Hello. Thanks for the fixes! >> >>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >>> index df75186..68f81e8 100644 >>> --- a/src/box/sql/build.c >>> +++ b/src/box/sql/build.c >>> @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ >>> -int >>> -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) >>> +bool >> >> In Tarantool code style we do not return boolean on non-check functions. >> 0 or -1, please. > Done. No(( Tests are failing. See 5 comments below, why. > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index df75186..f59087b 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > int > -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > +sql_view_column_names(struct Parse *parse, struct Table *table) > { > - Table *pSelTab; /* A fake table from which we get the result set */ > - Select *pSel; /* Copy of the SELECT that implements the view */ > - int nErr = 0; /* Number of errors encountered */ > - int n; /* Temporarily holds the number of cursors assigned */ > - sqlite3 *db = pParse->db; /* Database connection for malloc errors */ > - > - assert(pTable); > - > -#ifndef SQLITE_OMIT_VIEW > - /* A positive nCol means the columns names for this view are > - * already known. > + assert(table != NULL); > + assert(space_is_view(table)); > + /* A positive nCol means the columns names for this view > + * are already known. > */ > - if (pTable->nCol > 0) > - return 0; > + if (table->nCol > 0) > + return true; 1. Here you return 'true', but returned type is int. It is converted to 1, and this function is treated as failed. > > - /* A negative nCol is a special marker meaning that we are currently > - * trying to compute the column names. If we enter this routine with > - * a negative nCol, it means two or more views form a loop, like this: > + /* A negative nCol is a special marker meaning that we are > + * currently trying to compute the column names. If we > + * enter this routine with a negative nCol, it means two > + * or more views form a loop, like this: > * > * CREATE VIEW one AS SELECT * FROM two; > * CREATE VIEW two AS SELECT * FROM one; > * > - * Actually, the error above is now caught prior to reaching this point. > - * But the following test is still important as it does come up > - * in the following: > + * Actually, the error above is now caught prior to > + * reaching this point. But the following test is still > + * important as it does come up in the following: > * > * CREATE TABLE main.ex1(a); > * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1; > * SELECT * FROM temp.ex1; > */ > - if (pTable->nCol < 0) { > - sqlite3ErrorMsg(pParse, "view %s is circularly defined", > - pTable->zName); > - return 1; > - } > - assert(pTable->nCol >= 0); > - > - /* If we get this far, it means we need to compute the table names. > - * Note that the call to sqlite3ResultSetOfSelect() will expand any > - * "*" elements in the results set of the view and will assign cursors > - * to the elements of the FROM clause. But we do not want these changes > - * to be permanent. So the computation is done on a copy of the SELECT > - * statement that defines the view. > + if (table->nCol < 0) { > + sqlite3ErrorMsg(parse, "view %s is circularly defined", > + table->zName); > + return false; 2. Same. > + } > + assert(table->nCol >= 0); > + > + /* If we get this far, it means we need to compute the > + * table names. Note that the call to > + * sqlite3ResultSetOfSelect() will expand any "*" elements > + * in the results set of the view and will assign cursors > + * to the elements of the FROM clause. But we do not want > + * these changes to be permanent. So the computation is > + * done on a copy of the SELECT statement that defines the > + * view. > */ > - assert(pTable->pSelect); > - pSel = sqlite3SelectDup(db, pTable->pSelect, 0); > - if (pSel) { > - n = pParse->nTab; > - sqlite3SrcListAssignCursors(pParse, pSel->pSrc); > - pTable->nCol = -1; > - db->lookaside.bDisable++; > - pSelTab = sqlite3ResultSetOfSelect(pParse, pSel); > - pParse->nTab = n; > - if (pTable->pCheck) { > - /* CREATE VIEW name(arglist) AS ... > - * The names of the columns in the table are taken from > - * arglist which is stored in pTable->pCheck. The pCheck field > - * normally holds CHECK constraints on an ordinary table, but for > - * a VIEW it holds the list of column names. > - */ > - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, > - &pTable->nCol, > - &pTable->aCol); > - if (db->mallocFailed == 0 && pParse->nErr == 0 > - && pTable->nCol == pSel->pEList->nExpr) { > - sqlite3SelectAddColumnTypeAndCollation(pParse, > - pTable, > - pSel); > - } > - } else if (pSelTab) { > - /* CREATE VIEW name AS... without an argument list. Construct > - * the column names from the SELECT statement that defines the view. > - */ > - assert(pTable->aCol == 0); > - pTable->nCol = pSelTab->nCol; > - pTable->aCol = pSelTab->aCol; > - pSelTab->nCol = 0; > - pSelTab->aCol = 0; > - } else { > - pTable->nCol = 0; > - nErr++; > + assert(table->pSelect != NULL); > + sqlite3 *db = parse->db; > + struct Select *select = sqlite3SelectDup(db, table->pSelect, 0); > + if (select == NULL) > + return false; 3. Same. > + int n = parse->nTab; > + sqlite3SrcListAssignCursors(parse, select->pSrc); > + table->nCol = -1; > + db->lookaside.bDisable++; > + struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select); > + parse->nTab = n; > + bool is_ok = true; 4. is_ok -> rc. > + if (table->pCheck != NULL) { > + /* CREATE VIEW name(arglist) AS ... > + * The names of the columns in the table are taken > + * from arglist which is stored in table->pCheck. > + * The pCheck field normally holds CHECK > + * constraints on an ordinary table, but for a > + * VIEW it holds the list of column names. > + */ > + sqlite3ColumnsFromExprList(parse, table->pCheck, > + &table->nCol, > + &table->aCol); > + if (db->mallocFailed == 0 && parse->nErr == 0 > + && table->nCol == select->pEList->nExpr) { > + sqlite3SelectAddColumnTypeAndCollation(parse, table, > + select); > } > - sqlite3DeleteTable(db, pSelTab); > - sqlite3SelectDelete(db, pSel); > - db->lookaside.bDisable--; > + } else if (sel_tab != NULL) { > + /* CREATE VIEW name AS... without an argument > + * list. Construct the column names from the > + * SELECT statement that defines the view. > + */ > + assert(table->aCol == NULL); > + table->nCol = sel_tab->nCol; > + table->aCol = sel_tab->aCol; > + sel_tab->nCol = 0; > + sel_tab->aCol = NULL; > } else { > - nErr++; > + table->nCol = 0; > + is_ok = false; > } > -#endif /* SQLITE_OMIT_VIEW */ > - return nErr; > + sqlite3DeleteTable(db, sel_tab); > + sqlite3SelectDelete(db, select); > + db->lookaside.bDisable--; > + > + return is_ok; 5. Same as 1 - 3. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-21 10:49 ` Vladislav Shpilevoy @ 2018-05-21 11:24 ` Kirill Yukhin 2018-05-21 11:39 ` Vladislav Shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Yukhin @ 2018-05-21 11:24 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches On 21 мая 13:49, Vladislav Shpilevoy wrote: > > > On 21/05/2018 13:25, Kirill Yukhin wrote: > > On 21 мая 12:47, Vladislav Shpilevoy wrote: > > > Hello. Thanks for the fixes! > > > > > > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > > > > index df75186..68f81e8 100644 > > > > --- a/src/box/sql/build.c > > > > +++ b/src/box/sql/build.c > > > > @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ > > > > -int > > > > -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > > > > +bool > > > > > > In Tarantool code style we do not return boolean on non-check functions. > > > 0 or -1, please. > > Done. > > No(( Tests are failing. See 5 comments below, why. I've fixed that. > > - if (pTable->nCol > 0) > > - return 0; > > + if (table->nCol > 0) > > + return true; > > 1. Here you return 'true', but returned type is int. It is converted to 1, > and this function is treated as failed. Fixed. > > + sqlite3ErrorMsg(parse, "view %s is circularly defined", > > + table->zName); > > + return false; > > 2. Same. Ditto. > > + if (select == NULL) > > + return false; > > 3. Same. Ditto. > > + parse->nTab = n; > > + bool is_ok = true; > > 4. is_ok -> rc. Renamed. > > + db->lookaside.bDisable--; > > + > > + return is_ok; > 5. Same as 1 - 3. Fixed. -- Regards, Kirill Yukhin commit 3bb55c57ade2eed862d43f03947dde7ee39357e3 Author: Kirill Yukhin <kyukhin@tarantool.org> Date: Fri May 18 16:31:24 2018 +0300 sql: refactor SQL delete to allow Lua spaces This is a first step toward fully-featured deletion of spaces created in Lua by means of SQL language. This change to handle most simple case: DELETE * FROM <space_name> and will be improved in nearest future. Also, refactor (sqlite3ViewGetColumnNames()) to use Tarantool's code style and naming convention. Part of #3235 diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 998dd5b..cc594a7 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -169,7 +169,8 @@ openStatTable(Parse * pParse, /* Parsing context */ * The sql_stat[134] table already exists. * Delete all rows. */ - sqlite3VdbeAddOp2(v, OP_Clear, aRoot[i], 0); + sqlite3VdbeAddOp2(v, OP_Clear, + SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0); } } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index df75186..f81cb52 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ } #endif /* SQLITE_OMIT_VIEW */ -#if !defined(SQLITE_OMIT_VIEW) -/* - * The Table structure pTable is really a VIEW. Fill in the names of - * the columns of the view in the pTable structure. Return the number - * of errors. If an error is seen leave an error message in pParse->zErrMsg. - */ int -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) +sql_view_column_names(struct Parse *parse, struct Table *table) { - Table *pSelTab; /* A fake table from which we get the result set */ - Select *pSel; /* Copy of the SELECT that implements the view */ - int nErr = 0; /* Number of errors encountered */ - int n; /* Temporarily holds the number of cursors assigned */ - sqlite3 *db = pParse->db; /* Database connection for malloc errors */ - - assert(pTable); - -#ifndef SQLITE_OMIT_VIEW - /* A positive nCol means the columns names for this view are - * already known. + assert(table != NULL); + assert(space_is_view(table)); + /* A positive nCol means the columns names for this view + * are already known. */ - if (pTable->nCol > 0) + if (table->nCol > 0) return 0; - /* A negative nCol is a special marker meaning that we are currently - * trying to compute the column names. If we enter this routine with - * a negative nCol, it means two or more views form a loop, like this: + /* A negative nCol is a special marker meaning that we are + * currently trying to compute the column names. If we + * enter this routine with a negative nCol, it means two + * or more views form a loop, like this: * * CREATE VIEW one AS SELECT * FROM two; * CREATE VIEW two AS SELECT * FROM one; * - * Actually, the error above is now caught prior to reaching this point. - * But the following test is still important as it does come up - * in the following: + * Actually, the error above is now caught prior to + * reaching this point. But the following test is still + * important as it does come up in the following: * * CREATE TABLE main.ex1(a); * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1; * SELECT * FROM temp.ex1; */ - if (pTable->nCol < 0) { - sqlite3ErrorMsg(pParse, "view %s is circularly defined", - pTable->zName); - return 1; - } - assert(pTable->nCol >= 0); - - /* If we get this far, it means we need to compute the table names. - * Note that the call to sqlite3ResultSetOfSelect() will expand any - * "*" elements in the results set of the view and will assign cursors - * to the elements of the FROM clause. But we do not want these changes - * to be permanent. So the computation is done on a copy of the SELECT - * statement that defines the view. + if (table->nCol < 0) { + sqlite3ErrorMsg(parse, "view %s is circularly defined", + table->zName); + return -1; + } + assert(table->nCol >= 0); + + /* If we get this far, it means we need to compute the + * table names. Note that the call to + * sqlite3ResultSetOfSelect() will expand any "*" elements + * in the results set of the view and will assign cursors + * to the elements of the FROM clause. But we do not want + * these changes to be permanent. So the computation is + * done on a copy of the SELECT statement that defines the + * view. */ - assert(pTable->pSelect); - pSel = sqlite3SelectDup(db, pTable->pSelect, 0); - if (pSel) { - n = pParse->nTab; - sqlite3SrcListAssignCursors(pParse, pSel->pSrc); - pTable->nCol = -1; - db->lookaside.bDisable++; - pSelTab = sqlite3ResultSetOfSelect(pParse, pSel); - pParse->nTab = n; - if (pTable->pCheck) { - /* CREATE VIEW name(arglist) AS ... - * The names of the columns in the table are taken from - * arglist which is stored in pTable->pCheck. The pCheck field - * normally holds CHECK constraints on an ordinary table, but for - * a VIEW it holds the list of column names. - */ - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, - &pTable->nCol, - &pTable->aCol); - if (db->mallocFailed == 0 && pParse->nErr == 0 - && pTable->nCol == pSel->pEList->nExpr) { - sqlite3SelectAddColumnTypeAndCollation(pParse, - pTable, - pSel); - } - } else if (pSelTab) { - /* CREATE VIEW name AS... without an argument list. Construct - * the column names from the SELECT statement that defines the view. - */ - assert(pTable->aCol == 0); - pTable->nCol = pSelTab->nCol; - pTable->aCol = pSelTab->aCol; - pSelTab->nCol = 0; - pSelTab->aCol = 0; - } else { - pTable->nCol = 0; - nErr++; + assert(table->pSelect != NULL); + sqlite3 *db = parse->db; + struct Select *select = sqlite3SelectDup(db, table->pSelect, 0); + if (select == NULL) + return -1; + int n = parse->nTab; + sqlite3SrcListAssignCursors(parse, select->pSrc); + table->nCol = -1; + db->lookaside.bDisable++; + struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select); + parse->nTab = n; + int rc = 0; + if (table->pCheck != NULL) { + /* CREATE VIEW name(arglist) AS ... + * The names of the columns in the table are taken + * from arglist which is stored in table->pCheck. + * The pCheck field normally holds CHECK + * constraints on an ordinary table, but for a + * VIEW it holds the list of column names. + */ + sqlite3ColumnsFromExprList(parse, table->pCheck, + &table->nCol, + &table->aCol); + if (db->mallocFailed == 0 && parse->nErr == 0 + && table->nCol == select->pEList->nExpr) { + sqlite3SelectAddColumnTypeAndCollation(parse, table, + select); } - sqlite3DeleteTable(db, pSelTab); - sqlite3SelectDelete(db, pSel); - db->lookaside.bDisable--; + } else if (sel_tab != NULL) { + /* CREATE VIEW name AS... without an argument + * list. Construct the column names from the + * SELECT statement that defines the view. + */ + assert(table->aCol == NULL); + table->nCol = sel_tab->nCol; + table->aCol = sel_tab->aCol; + sel_tab->nCol = 0; + sel_tab->aCol = NULL; } else { - nErr++; + table->nCol = 0; + rc = -1; } -#endif /* SQLITE_OMIT_VIEW */ - return nErr; + sqlite3DeleteTable(db, sel_tab); + sqlite3SelectDelete(db, select); + db->lookaside.bDisable--; + + return rc; } -#endif /* !defined(SQLITE_OMIT_VIEW) */ #ifndef SQLITE_OMIT_VIEW /* @@ -2672,7 +2662,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) VdbeCoverage(v); sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) - sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); + sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum), + 0); emit_open_cursor(pParse, iIdx, tnum); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR | ((memRootPage >= 0) ? diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index f4d248e..8c7cd7d 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -29,6 +29,7 @@ * SUCH DAMAGE. */ +#include "box/box.h" #include "box/session.h" #include "box/schema.h" #include "sqliteInt.h" @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * with multiple tables and expect an SrcList* parameter * instead of just a Table* parameter. */ - struct Table *table = sql_list_lookup_table(parse, tab_list); - if (table == NULL) - goto delete_from_cleanup; - - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(table->tnum)); - assert(space != NULL); + struct Table *table = NULL; + struct space *space; + uint32_t space_id; /* Figure out if we have any triggers and if the table * being deleted from is a view. */ - struct Trigger *trigger_list = sqlite3TriggersExist(table, TK_DELETE, - NULL, NULL); - - bool is_view = space->def->opts.is_view; + struct Trigger *trigger_list = NULL; /* True if there are triggers or FKs or subqueries in the * WHERE clause. */ - bool is_complex = trigger_list != NULL || - sqlite3FkRequired(table, NULL); + bool is_complex = false; + const char *tab_name = tab_list->a->zName; + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { + space_id = box_space_id_by_name(tab_name, + strlen(tab_name)); + if (space_id == BOX_ID_NIL) + goto delete_from_cleanup; + } else { + table = sql_list_lookup_table(parse, tab_list); + if (table == NULL) + goto delete_from_cleanup; + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); + trigger_list =sqlite3TriggersExist(table, TK_DELETE, + NULL, NULL); + is_complex = trigger_list != NULL || + sqlite3FkRequired(table, NULL); + } + space = space_by_id(space_id); + assert(space != NULL); + + bool is_view = space->def->opts.is_view; /* If table is really a view, make sure it has been * initialized. */ - if (sqlite3ViewGetColumnNames(parse, table)) - goto delete_from_cleanup; + if (is_view) { + if (sql_view_column_names(parse, table) != 0) + goto delete_from_cleanup; - if (is_view && trigger_list == NULL) { - sqlite3ErrorMsg(parse, "cannot modify %s because it is a view", - space->def->name); - goto delete_from_cleanup; + if (trigger_list == NULL) { + sqlite3ErrorMsg(parse, "cannot modify %s because it is a" + " view", space->def->name); + goto delete_from_cleanup; + } } /* Assign cursor numbers to the table and all its indices. @@ -139,14 +155,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, tab_cursor); } - /* Resolve the column names in the WHERE clause. */ - struct NameContext nc; - memset(&nc, 0, sizeof(nc)); - nc.pParse = parse; - nc.pSrcList = tab_list; - if (sqlite3ResolveExprNames(&nc, where)) - goto delete_from_cleanup; - /* Initialize the counter of the number of rows deleted, * if we are counting rows. */ @@ -162,7 +170,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, if (where == NULL && !is_complex) { assert(!is_view); - sqlite3VdbeAddOp1(v, OP_Clear, table->tnum); + sqlite3VdbeAddOp1(v, OP_Clear, space_id); /* Do not start Tarantool's transaction in case of * truncate optimization. This is workaround until @@ -171,6 +179,13 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, */ parse->initiateTTrans = false; } else { + /* Resolve the column names in the WHERE clause. */ + struct NameContext nc; + memset(&nc, 0, sizeof(nc)); + nc.pParse = parse; + nc.pSrcList = tab_list; + if (sqlite3ResolveExprNames(&nc, where)) + goto delete_from_cleanup; uint16_t wcf = WHERE_ONEPASS_DESIRED | WHERE_DUPLICATES_OK | WHERE_SEEK_TABLE; if (nc.ncFlags & NC_VarSelect) diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index e23cffd..5d62e58 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -349,11 +349,9 @@ sqlite3Insert(Parse * pParse, /* Parser context */ int *aRegIdx = 0; /* One register allocated to each index */ uint32_t space_id = 0; -#ifndef SQLITE_OMIT_TRIGGER - int isView; /* True if attempting to insert into a view */ + bool is_view; /* True if attempting to insert into a view */ Trigger *pTrigger; /* List of triggers on pTab, if required */ int tmask; /* Mask of trigger times */ -#endif db = pParse->db; memset(&dest, 0, sizeof(dest)); @@ -389,28 +387,18 @@ sqlite3Insert(Parse * pParse, /* Parser context */ /* Figure out if we have any triggers and if the table being * inserted into is a view */ -#ifndef SQLITE_OMIT_TRIGGER pTrigger = sqlite3TriggersExist(pTab, TK_INSERT, 0, &tmask); - isView = space_is_view(pTab); -#else -#define pTrigger 0 -#define tmask 0 -#define isView 0 -#endif -#ifdef SQLITE_OMIT_VIEW -#undef isView -#define isView 0 -#endif + is_view = space_is_view(pTab); assert((pTrigger && tmask) || (pTrigger == 0 && tmask == 0)); /* If pTab is really a view, make sure it has been initialized. * ViewGetColumnNames() is a no-op if pTab is not a view. */ - if (sqlite3ViewGetColumnNames(pParse, pTab)) + if (is_view && sql_view_column_names(pParse, pTab) != 0) goto insert_cleanup; /* Cannot insert into a read-only table. */ - if (isView && tmask == 0) { + if (is_view && tmask == 0) { sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", pTab->zName); goto insert_cleanup; @@ -481,7 +469,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ bIdListInOrder = 0; if (j == pTab->iPKey) { ipkColumn = i; - assert(isView); + assert(is_view); } break; } @@ -637,7 +625,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ } /* If this is not a view, open the table and and all indices */ - if (!isView) { + if (!is_view) { int nIdx; nIdx = sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0, @@ -731,7 +719,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ * If this is a real table, attempt conversions as required by the * table column affinities. */ - if (!isView) { + if (!is_view) { sqlite3TableAffinity(v, pTab, regCols + 1); } @@ -747,7 +735,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ /* Compute the content of the next row to insert into a range of * registers beginning at regIns. */ - if (!isView) { + if (!is_view) { if (ipkColumn >= 0) { if (useTempTable) { sqlite3VdbeAddOp3(v, OP_Column, srcTab, @@ -930,20 +918,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */ sqlite3DbFree(db, aRegIdx); } -/* Make sure "isView" and other macros defined above are undefined. Otherwise - * they may interfere with compilation of other functions in this file - * (or in another file, if this file becomes part of the amalgamation). - */ -#ifdef isView -#undef isView -#endif -#ifdef pTrigger -#undef pTrigger -#endif -#ifdef tmask -#undef tmask -#endif - /* * Meanings of bits in of pWalker->eCode for checkConstraintUnchanged() */ diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index e4ceec2..634da87 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -357,7 +357,8 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ Column *pCol; Index *pPk = sqlite3PrimaryKeyIndex(pTab); pParse->nMem = 6; - sqlite3ViewGetColumnNames(pParse, pTab); + if (space_is_view(pTab)) + sql_view_column_names(pParse, pTab); for (i = 0, pCol = pTab->aCol; i < pTab->nCol; i++, pCol++) { if (!table_column_is_in_pk(pTab, i)) { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index de111d3..e4d4013 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -4680,7 +4680,7 @@ selectExpander(Walker * pWalker, Select * p) #if !defined(SQLITE_OMIT_VIEW) if (space_is_view(pTab)) { i16 nCol; - if (sqlite3ViewGetColumnNames(pParse, pTab)) + if (sql_view_column_names(pParse, pTab) != 0) return WRC_Abort; assert(pFrom->pSelect == 0); pFrom->pSelect = diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index ab50c3e..26db466 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3592,11 +3592,18 @@ int sqlite3FaultSim(int); void sqlite3CreateView(Parse *, Token *, Token *, ExprList *, Select *, int); -#if !defined(SQLITE_OMIT_VIEW) -int sqlite3ViewGetColumnNames(Parse *, Table *); -#else -#define sqlite3ViewGetColumnNames(A,B) 0 -#endif +/** + * The Table structure pTable is really a VIEW. Fill in the names + * of the columns of the view in the table structure. Return the + * number of errors. If an error is seen leave an error message + * in parse->zErrMsg. + * + * @param parse Parsing context. + * @param table Tables to process. + * @retval 0 if success, -1 in case of error. + */ +int +sql_view_column_names(struct Parse *parse, struct Table *table); void sql_drop_table(struct Parse *, struct SrcList *, bool, bool); diff --git a/src/box/sql/update.c b/src/box/sql/update.c index c521a3b..4d6d0e1 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -137,11 +137,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ int labelContinue; /* Jump here to continue next step of UPDATE loop */ struct session *user_session = current_session(); -#ifndef SQLITE_OMIT_TRIGGER - int isView; /* True when updating a view (INSTEAD OF trigger) */ + bool is_view; /* True when updating a view (INSTEAD OF trigger) */ Trigger *pTrigger; /* List of triggers on pTab, if required */ int tmask; /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */ -#endif int newmask; /* Mask of NEW.* columns accessed by BEFORE triggers */ int iEph = 0; /* Ephemeral table holding all primary key values */ int nKey = 0; /* Number of elements in regKey */ @@ -170,24 +168,14 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* Figure out if we have any triggers and if the table being * updated is a view. */ -#ifndef SQLITE_OMIT_TRIGGER pTrigger = sqlite3TriggersExist(pTab, TK_UPDATE, pChanges, &tmask); - isView = space_is_view(pTab); + is_view = space_is_view(pTab); assert(pTrigger || tmask == 0); -#else -#define pTrigger 0 -#define isView 0 -#define tmask 0 -#endif -#ifdef SQLITE_OMIT_VIEW -#undef isView -#define isView 0 -#endif - if (sqlite3ViewGetColumnNames(pParse, pTab)) { + if (is_view && sql_view_column_names(pParse, pTab) != 0) { goto update_cleanup; } - if (isView && tmask == 0) { + if (is_view && tmask == 0) { sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", pTab->zName); goto update_cleanup; @@ -200,7 +188,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ */ pTabList->a[0].iCursor = iBaseCur = iDataCur = pParse->nTab++; iIdxCur = iDataCur + 1; - pPk = isView ? 0 : sqlite3PrimaryKeyIndex(pTab); + pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab); for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) { if (IsPrimaryKeyIndex(pIdx) && pPk != 0) { iDataCur = pParse->nTab; @@ -324,13 +312,11 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* If we are trying to update a view, realize that view into * an ephemeral table. */ -#if !defined(SQLITE_OMIT_VIEW) && !defined(SQLITE_OMIT_TRIGGER) - if (isView) { + if (is_view) { sql_materialize_view(pParse, pTab->zName, pWhere, iDataCur); /* Number of columns from SELECT plus ID.*/ nKey = pTab->nCol + 1; } -#endif /* Resolve the column names in all the expressions in the * WHERE clause. @@ -347,7 +333,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ i16 nPk; /* Number of components of the PRIMARY KEY */ int addrOpen; /* Address of the OpenEphemeral instruction */ - if (isView) { + if (is_view) { nPk = nKey; } else { assert(pPk != 0); @@ -359,7 +345,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ iEph = pParse->nTab++; sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); - if (isView) { + if (is_view) { addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, nKey); } else { @@ -372,7 +358,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ if (pWInfo == 0) goto update_cleanup; okOnePass = sqlite3WhereOkOnePass(pWInfo, aiCurOnePass); - if (isView){ + if (is_view) { for (i = 0; i < nPk; i++) { sqlite3VdbeAddOp3(v, OP_Column, iDataCur, i, iPk + i); } @@ -390,7 +376,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ nKey = nPk; regKey = iPk; } else { - const char *zAff = isView ? 0 : + const char *zAff = is_view ? 0 : sqlite3IndexAffinityStr(pParse->db, pPk); sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey, zAff, nPk); @@ -412,7 +398,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } labelBreak = sqlite3VdbeMakeLabel(v); - if (!isView) { + if (!is_view) { /* * Open every index that needs updating. Note that if any * index could potentially invoke a REPLACE conflict resolution @@ -445,7 +431,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ labelContinue = labelBreak; sqlite3VdbeAddOp2(v, OP_IsNull, regKey, labelBreak); - if (aToOpen[iDataCur - iBaseCur] && !isView) { + if (aToOpen[iDataCur - iBaseCur] && !is_view) { assert(pPk); sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelBreak, regKey, nKey); @@ -552,7 +538,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ * is deleted or renamed by a BEFORE trigger - is left undefined in the * documentation. */ - if (!isView) { + if (!is_view) { sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, labelContinue, regKey, nKey); VdbeCoverage(v); @@ -576,7 +562,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ } } - if (!isView) { + if (!is_view) { int addr1 = 0; /* Address of jump instruction */ int bReplace = 0; /* True if REPLACE conflict resolution might happen */ @@ -691,15 +677,3 @@ sqlite3Update(Parse * pParse, /* The parser context */ sql_expr_free(db, pWhere, false); return; } - -/* Make sure "isView" and other macros defined above are undefined. Otherwise - * they may interfere with compilation of other functions in this file - * (or in another file, if this file becomes part of the amalgamation). - */ -#ifdef isView -#undef isView -#endif -#ifdef pTrigger -#undef pTrigger -#endif - diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 455198d..9acb9b6 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4614,7 +4614,7 @@ case OP_IdxGE: { /* jump */ */ case OP_Clear: { assert(pOp->p1 > 0); - uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1); + uint32_t space_id = pOp->p1; struct space *space = space_by_id(space_id); assert(space != NULL); rc = tarantoolSqlite3ClearTable(space); diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua index 61d7756..810ca8a 100755 --- a/test/sql-tap/delete1.test.lua +++ b/test/sql-tap/delete1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(7) +test:plan(9) --!./tcltestrunner.lua -- ["set","testdir",[["file","dirname",["argv0"]]]] @@ -131,5 +131,26 @@ test:do_test( -- </delete1-5.0> }) +-- Tests for data dictionary integration. +s = box.schema.create_space('t') +i = s:create_index('i', {parts={1, 'unsigned'}}) +test:do_test( + "delete1-6.0", + function() + s:replace({1}) + s:replace({2}) + s:replace({3}) + return s:count() + end, + 3) + +test:do_test( + "delete1-6.1", + function() + box.sql.execute([[delete from "t"]]) + return s:count() + end, + 0) + test:finish_test() ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces 2018-05-21 11:24 ` Kirill Yukhin @ 2018-05-21 11:39 ` Vladislav Shpilevoy 0 siblings, 0 replies; 8+ messages in thread From: Vladislav Shpilevoy @ 2018-05-21 11:39 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches Thanks! LGTM. On 21/05/2018 14:24, Kirill Yukhin wrote: > On 21 мая 13:49, Vladislav Shpilevoy wrote: >> >> >> On 21/05/2018 13:25, Kirill Yukhin wrote: >>> On 21 мая 12:47, Vladislav Shpilevoy wrote: >>>> Hello. Thanks for the fixes! >>>> >>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >>>>> index df75186..68f81e8 100644 >>>>> --- a/src/box/sql/build.c >>>>> +++ b/src/box/sql/build.c >>>>> @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ >>>>> -int >>>>> -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) >>>>> +bool >>>> >>>> In Tarantool code style we do not return boolean on non-check functions. >>>> 0 or -1, please. >>> Done. >> >> No(( Tests are failing. See 5 comments below, why. > I've fixed that. > >>> - if (pTable->nCol > 0) >>> - return 0; >>> + if (table->nCol > 0) >>> + return true; >> >> 1. Here you return 'true', but returned type is int. It is converted to 1, >> and this function is treated as failed. > Fixed. > >>> + sqlite3ErrorMsg(parse, "view %s is circularly defined", >>> + table->zName); >>> + return false; >> >> 2. Same. > Ditto. > >>> + if (select == NULL) >>> + return false; >> >> 3. Same. > Ditto. > >>> + parse->nTab = n; >>> + bool is_ok = true; >> >> 4. is_ok -> rc. > Renamed. > >>> + db->lookaside.bDisable--; >>> + >>> + return is_ok; >> 5. Same as 1 - 3. > Fixed. > > -- > Regards, Kirill Yukhin > > commit 3bb55c57ade2eed862d43f03947dde7ee39357e3 > Author: Kirill Yukhin <kyukhin@tarantool.org> > Date: Fri May 18 16:31:24 2018 +0300 > > sql: refactor SQL delete to allow Lua spaces > > This is a first step toward fully-featured deletion > of spaces created in Lua by means of SQL language. > This change to handle most simple case: > DELETE * FROM <space_name> and will be improved in > nearest future. > Also, refactor (sqlite3ViewGetColumnNames()) to > use Tarantool's code style and naming convention. > > Part of #3235 > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 998dd5b..cc594a7 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -169,7 +169,8 @@ openStatTable(Parse * pParse, /* Parsing context */ > * The sql_stat[134] table already exists. > * Delete all rows. > */ > - sqlite3VdbeAddOp2(v, OP_Clear, aRoot[i], 0); > + sqlite3VdbeAddOp2(v, OP_Clear, > + SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0); > } > } > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index df75186..f81cb52 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */ > } > #endif /* SQLITE_OMIT_VIEW */ > > -#if !defined(SQLITE_OMIT_VIEW) > -/* > - * The Table structure pTable is really a VIEW. Fill in the names of > - * the columns of the view in the pTable structure. Return the number > - * of errors. If an error is seen leave an error message in pParse->zErrMsg. > - */ > int > -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > +sql_view_column_names(struct Parse *parse, struct Table *table) > { > - Table *pSelTab; /* A fake table from which we get the result set */ > - Select *pSel; /* Copy of the SELECT that implements the view */ > - int nErr = 0; /* Number of errors encountered */ > - int n; /* Temporarily holds the number of cursors assigned */ > - sqlite3 *db = pParse->db; /* Database connection for malloc errors */ > - > - assert(pTable); > - > -#ifndef SQLITE_OMIT_VIEW > - /* A positive nCol means the columns names for this view are > - * already known. > + assert(table != NULL); > + assert(space_is_view(table)); > + /* A positive nCol means the columns names for this view > + * are already known. > */ > - if (pTable->nCol > 0) > + if (table->nCol > 0) > return 0; > > - /* A negative nCol is a special marker meaning that we are currently > - * trying to compute the column names. If we enter this routine with > - * a negative nCol, it means two or more views form a loop, like this: > + /* A negative nCol is a special marker meaning that we are > + * currently trying to compute the column names. If we > + * enter this routine with a negative nCol, it means two > + * or more views form a loop, like this: > * > * CREATE VIEW one AS SELECT * FROM two; > * CREATE VIEW two AS SELECT * FROM one; > * > - * Actually, the error above is now caught prior to reaching this point. > - * But the following test is still important as it does come up > - * in the following: > + * Actually, the error above is now caught prior to > + * reaching this point. But the following test is still > + * important as it does come up in the following: > * > * CREATE TABLE main.ex1(a); > * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1; > * SELECT * FROM temp.ex1; > */ > - if (pTable->nCol < 0) { > - sqlite3ErrorMsg(pParse, "view %s is circularly defined", > - pTable->zName); > - return 1; > - } > - assert(pTable->nCol >= 0); > - > - /* If we get this far, it means we need to compute the table names. > - * Note that the call to sqlite3ResultSetOfSelect() will expand any > - * "*" elements in the results set of the view and will assign cursors > - * to the elements of the FROM clause. But we do not want these changes > - * to be permanent. So the computation is done on a copy of the SELECT > - * statement that defines the view. > + if (table->nCol < 0) { > + sqlite3ErrorMsg(parse, "view %s is circularly defined", > + table->zName); > + return -1; > + } > + assert(table->nCol >= 0); > + > + /* If we get this far, it means we need to compute the > + * table names. Note that the call to > + * sqlite3ResultSetOfSelect() will expand any "*" elements > + * in the results set of the view and will assign cursors > + * to the elements of the FROM clause. But we do not want > + * these changes to be permanent. So the computation is > + * done on a copy of the SELECT statement that defines the > + * view. > */ > - assert(pTable->pSelect); > - pSel = sqlite3SelectDup(db, pTable->pSelect, 0); > - if (pSel) { > - n = pParse->nTab; > - sqlite3SrcListAssignCursors(pParse, pSel->pSrc); > - pTable->nCol = -1; > - db->lookaside.bDisable++; > - pSelTab = sqlite3ResultSetOfSelect(pParse, pSel); > - pParse->nTab = n; > - if (pTable->pCheck) { > - /* CREATE VIEW name(arglist) AS ... > - * The names of the columns in the table are taken from > - * arglist which is stored in pTable->pCheck. The pCheck field > - * normally holds CHECK constraints on an ordinary table, but for > - * a VIEW it holds the list of column names. > - */ > - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, > - &pTable->nCol, > - &pTable->aCol); > - if (db->mallocFailed == 0 && pParse->nErr == 0 > - && pTable->nCol == pSel->pEList->nExpr) { > - sqlite3SelectAddColumnTypeAndCollation(pParse, > - pTable, > - pSel); > - } > - } else if (pSelTab) { > - /* CREATE VIEW name AS... without an argument list. Construct > - * the column names from the SELECT statement that defines the view. > - */ > - assert(pTable->aCol == 0); > - pTable->nCol = pSelTab->nCol; > - pTable->aCol = pSelTab->aCol; > - pSelTab->nCol = 0; > - pSelTab->aCol = 0; > - } else { > - pTable->nCol = 0; > - nErr++; > + assert(table->pSelect != NULL); > + sqlite3 *db = parse->db; > + struct Select *select = sqlite3SelectDup(db, table->pSelect, 0); > + if (select == NULL) > + return -1; > + int n = parse->nTab; > + sqlite3SrcListAssignCursors(parse, select->pSrc); > + table->nCol = -1; > + db->lookaside.bDisable++; > + struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select); > + parse->nTab = n; > + int rc = 0; > + if (table->pCheck != NULL) { > + /* CREATE VIEW name(arglist) AS ... > + * The names of the columns in the table are taken > + * from arglist which is stored in table->pCheck. > + * The pCheck field normally holds CHECK > + * constraints on an ordinary table, but for a > + * VIEW it holds the list of column names. > + */ > + sqlite3ColumnsFromExprList(parse, table->pCheck, > + &table->nCol, > + &table->aCol); > + if (db->mallocFailed == 0 && parse->nErr == 0 > + && table->nCol == select->pEList->nExpr) { > + sqlite3SelectAddColumnTypeAndCollation(parse, table, > + select); > } > - sqlite3DeleteTable(db, pSelTab); > - sqlite3SelectDelete(db, pSel); > - db->lookaside.bDisable--; > + } else if (sel_tab != NULL) { > + /* CREATE VIEW name AS... without an argument > + * list. Construct the column names from the > + * SELECT statement that defines the view. > + */ > + assert(table->aCol == NULL); > + table->nCol = sel_tab->nCol; > + table->aCol = sel_tab->aCol; > + sel_tab->nCol = 0; > + sel_tab->aCol = NULL; > } else { > - nErr++; > + table->nCol = 0; > + rc = -1; > } > -#endif /* SQLITE_OMIT_VIEW */ > - return nErr; > + sqlite3DeleteTable(db, sel_tab); > + sqlite3SelectDelete(db, select); > + db->lookaside.bDisable--; > + > + return rc; > } > -#endif /* !defined(SQLITE_OMIT_VIEW) */ > > #ifndef SQLITE_OMIT_VIEW > /* > @@ -2672,7 +2662,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) > VdbeCoverage(v); > sqlite3VdbeJumpHere(v, addr1); > if (memRootPage < 0) > - sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); > + sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum), > + 0); > emit_open_cursor(pParse, iIdx, tnum); > sqlite3VdbeChangeP5(v, > OPFLAG_BULKCSR | ((memRootPage >= 0) ? > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index f4d248e..8c7cd7d 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -29,6 +29,7 @@ > * SUCH DAMAGE. > */ > > +#include "box/box.h" > #include "box/session.h" > #include "box/schema.h" > #include "sqliteInt.h" > @@ -86,35 +87,50 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > * with multiple tables and expect an SrcList* parameter > * instead of just a Table* parameter. > */ > - struct Table *table = sql_list_lookup_table(parse, tab_list); > - if (table == NULL) > - goto delete_from_cleanup; > - > - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(table->tnum)); > - assert(space != NULL); > + struct Table *table = NULL; > + struct space *space; > + uint32_t space_id; > /* Figure out if we have any triggers and if the table > * being deleted from is a view. > */ > - struct Trigger *trigger_list = sqlite3TriggersExist(table, TK_DELETE, > - NULL, NULL); > - > - bool is_view = space->def->opts.is_view; > + struct Trigger *trigger_list = NULL; > /* True if there are triggers or FKs or subqueries in the > * WHERE clause. > */ > - bool is_complex = trigger_list != NULL || > - sqlite3FkRequired(table, NULL); > + bool is_complex = false; > + const char *tab_name = tab_list->a->zName; > + if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) { > + space_id = box_space_id_by_name(tab_name, > + strlen(tab_name)); > + if (space_id == BOX_ID_NIL) > + goto delete_from_cleanup; > + } else { > + table = sql_list_lookup_table(parse, tab_list); > + if (table == NULL) > + goto delete_from_cleanup; > + space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum); > + trigger_list =sqlite3TriggersExist(table, TK_DELETE, > + NULL, NULL); > + is_complex = trigger_list != NULL || > + sqlite3FkRequired(table, NULL); > + } > + space = space_by_id(space_id); > + assert(space != NULL); > + > + bool is_view = space->def->opts.is_view; > > /* If table is really a view, make sure it has been > * initialized. > */ > - if (sqlite3ViewGetColumnNames(parse, table)) > - goto delete_from_cleanup; > + if (is_view) { > + if (sql_view_column_names(parse, table) != 0) > + goto delete_from_cleanup; > > - if (is_view && trigger_list == NULL) { > - sqlite3ErrorMsg(parse, "cannot modify %s because it is a view", > - space->def->name); > - goto delete_from_cleanup; > + if (trigger_list == NULL) { > + sqlite3ErrorMsg(parse, "cannot modify %s because it is a" > + " view", space->def->name); > + goto delete_from_cleanup; > + } > } > > /* Assign cursor numbers to the table and all its indices. > @@ -139,14 +155,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > tab_cursor); > } > > - /* Resolve the column names in the WHERE clause. */ > - struct NameContext nc; > - memset(&nc, 0, sizeof(nc)); > - nc.pParse = parse; > - nc.pSrcList = tab_list; > - if (sqlite3ResolveExprNames(&nc, where)) > - goto delete_from_cleanup; > - > /* Initialize the counter of the number of rows deleted, > * if we are counting rows. > */ > @@ -162,7 +170,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > if (where == NULL && !is_complex) { > assert(!is_view); > > - sqlite3VdbeAddOp1(v, OP_Clear, table->tnum); > + sqlite3VdbeAddOp1(v, OP_Clear, space_id); > > /* Do not start Tarantool's transaction in case of > * truncate optimization. This is workaround until > @@ -171,6 +179,13 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > */ > parse->initiateTTrans = false; > } else { > + /* Resolve the column names in the WHERE clause. */ > + struct NameContext nc; > + memset(&nc, 0, sizeof(nc)); > + nc.pParse = parse; > + nc.pSrcList = tab_list; > + if (sqlite3ResolveExprNames(&nc, where)) > + goto delete_from_cleanup; > uint16_t wcf = WHERE_ONEPASS_DESIRED | WHERE_DUPLICATES_OK | > WHERE_SEEK_TABLE; > if (nc.ncFlags & NC_VarSelect) > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index e23cffd..5d62e58 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -349,11 +349,9 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > int *aRegIdx = 0; /* One register allocated to each index */ > uint32_t space_id = 0; > > -#ifndef SQLITE_OMIT_TRIGGER > - int isView; /* True if attempting to insert into a view */ > + bool is_view; /* True if attempting to insert into a view */ > Trigger *pTrigger; /* List of triggers on pTab, if required */ > int tmask; /* Mask of trigger times */ > -#endif > > db = pParse->db; > memset(&dest, 0, sizeof(dest)); > @@ -389,28 +387,18 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > /* Figure out if we have any triggers and if the table being > * inserted into is a view > */ > -#ifndef SQLITE_OMIT_TRIGGER > pTrigger = sqlite3TriggersExist(pTab, TK_INSERT, 0, &tmask); > - isView = space_is_view(pTab); > -#else > -#define pTrigger 0 > -#define tmask 0 > -#define isView 0 > -#endif > -#ifdef SQLITE_OMIT_VIEW > -#undef isView > -#define isView 0 > -#endif > + is_view = space_is_view(pTab); > assert((pTrigger && tmask) || (pTrigger == 0 && tmask == 0)); > > /* If pTab is really a view, make sure it has been initialized. > * ViewGetColumnNames() is a no-op if pTab is not a view. > */ > - if (sqlite3ViewGetColumnNames(pParse, pTab)) > + if (is_view && sql_view_column_names(pParse, pTab) != 0) > goto insert_cleanup; > > /* Cannot insert into a read-only table. */ > - if (isView && tmask == 0) { > + if (is_view && tmask == 0) { > sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", > pTab->zName); > goto insert_cleanup; > @@ -481,7 +469,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > bIdListInOrder = 0; > if (j == pTab->iPKey) { > ipkColumn = i; > - assert(isView); > + assert(is_view); > } > break; > } > @@ -637,7 +625,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > } > > /* If this is not a view, open the table and and all indices */ > - if (!isView) { > + if (!is_view) { > int nIdx; > nIdx = > sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0, > @@ -731,7 +719,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > * If this is a real table, attempt conversions as required by the > * table column affinities. > */ > - if (!isView) { > + if (!is_view) { > sqlite3TableAffinity(v, pTab, regCols + 1); > } > > @@ -747,7 +735,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > /* Compute the content of the next row to insert into a range of > * registers beginning at regIns. > */ > - if (!isView) { > + if (!is_view) { > if (ipkColumn >= 0) { > if (useTempTable) { > sqlite3VdbeAddOp3(v, OP_Column, srcTab, > @@ -930,20 +918,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > sqlite3DbFree(db, aRegIdx); > } > > -/* Make sure "isView" and other macros defined above are undefined. Otherwise > - * they may interfere with compilation of other functions in this file > - * (or in another file, if this file becomes part of the amalgamation). > - */ > -#ifdef isView > -#undef isView > -#endif > -#ifdef pTrigger > -#undef pTrigger > -#endif > -#ifdef tmask > -#undef tmask > -#endif > - > /* > * Meanings of bits in of pWalker->eCode for checkConstraintUnchanged() > */ > diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c > index e4ceec2..634da87 100644 > --- a/src/box/sql/pragma.c > +++ b/src/box/sql/pragma.c > @@ -357,7 +357,8 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ > Column *pCol; > Index *pPk = sqlite3PrimaryKeyIndex(pTab); > pParse->nMem = 6; > - sqlite3ViewGetColumnNames(pParse, pTab); > + if (space_is_view(pTab)) > + sql_view_column_names(pParse, pTab); > for (i = 0, pCol = pTab->aCol; i < pTab->nCol; > i++, pCol++) { > if (!table_column_is_in_pk(pTab, i)) { > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index de111d3..e4d4013 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -4680,7 +4680,7 @@ selectExpander(Walker * pWalker, Select * p) > #if !defined(SQLITE_OMIT_VIEW) > if (space_is_view(pTab)) { > i16 nCol; > - if (sqlite3ViewGetColumnNames(pParse, pTab)) > + if (sql_view_column_names(pParse, pTab) != 0) > return WRC_Abort; > assert(pFrom->pSelect == 0); > pFrom->pSelect = > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index ab50c3e..26db466 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -3592,11 +3592,18 @@ int sqlite3FaultSim(int); > > void sqlite3CreateView(Parse *, Token *, Token *, ExprList *, Select *, int); > > -#if !defined(SQLITE_OMIT_VIEW) > -int sqlite3ViewGetColumnNames(Parse *, Table *); > -#else > -#define sqlite3ViewGetColumnNames(A,B) 0 > -#endif > +/** > + * The Table structure pTable is really a VIEW. Fill in the names > + * of the columns of the view in the table structure. Return the > + * number of errors. If an error is seen leave an error message > + * in parse->zErrMsg. > + * > + * @param parse Parsing context. > + * @param table Tables to process. > + * @retval 0 if success, -1 in case of error. > + */ > +int > +sql_view_column_names(struct Parse *parse, struct Table *table); > > void > sql_drop_table(struct Parse *, struct SrcList *, bool, bool); > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index c521a3b..4d6d0e1 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -137,11 +137,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ > int labelContinue; /* Jump here to continue next step of UPDATE loop */ > struct session *user_session = current_session(); > > -#ifndef SQLITE_OMIT_TRIGGER > - int isView; /* True when updating a view (INSTEAD OF trigger) */ > + bool is_view; /* True when updating a view (INSTEAD OF trigger) */ > Trigger *pTrigger; /* List of triggers on pTab, if required */ > int tmask; /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */ > -#endif > int newmask; /* Mask of NEW.* columns accessed by BEFORE triggers */ > int iEph = 0; /* Ephemeral table holding all primary key values */ > int nKey = 0; /* Number of elements in regKey */ > @@ -170,24 +168,14 @@ sqlite3Update(Parse * pParse, /* The parser context */ > /* Figure out if we have any triggers and if the table being > * updated is a view. > */ > -#ifndef SQLITE_OMIT_TRIGGER > pTrigger = sqlite3TriggersExist(pTab, TK_UPDATE, pChanges, &tmask); > - isView = space_is_view(pTab); > + is_view = space_is_view(pTab); > assert(pTrigger || tmask == 0); > -#else > -#define pTrigger 0 > -#define isView 0 > -#define tmask 0 > -#endif > -#ifdef SQLITE_OMIT_VIEW > -#undef isView > -#define isView 0 > -#endif > > - if (sqlite3ViewGetColumnNames(pParse, pTab)) { > + if (is_view && sql_view_column_names(pParse, pTab) != 0) { > goto update_cleanup; > } > - if (isView && tmask == 0) { > + if (is_view && tmask == 0) { > sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", > pTab->zName); > goto update_cleanup; > @@ -200,7 +188,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > */ > pTabList->a[0].iCursor = iBaseCur = iDataCur = pParse->nTab++; > iIdxCur = iDataCur + 1; > - pPk = isView ? 0 : sqlite3PrimaryKeyIndex(pTab); > + pPk = is_view ? 0 : sqlite3PrimaryKeyIndex(pTab); > for (nIdx = 0, pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext, nIdx++) { > if (IsPrimaryKeyIndex(pIdx) && pPk != 0) { > iDataCur = pParse->nTab; > @@ -324,13 +312,11 @@ sqlite3Update(Parse * pParse, /* The parser context */ > /* If we are trying to update a view, realize that view into > * an ephemeral table. > */ > -#if !defined(SQLITE_OMIT_VIEW) && !defined(SQLITE_OMIT_TRIGGER) > - if (isView) { > + if (is_view) { > sql_materialize_view(pParse, pTab->zName, pWhere, iDataCur); > /* Number of columns from SELECT plus ID.*/ > nKey = pTab->nCol + 1; > } > -#endif > > /* Resolve the column names in all the expressions in the > * WHERE clause. > @@ -347,7 +333,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > i16 nPk; /* Number of components of the PRIMARY KEY */ > int addrOpen; /* Address of the OpenEphemeral instruction */ > > - if (isView) { > + if (is_view) { > nPk = nKey; > } else { > assert(pPk != 0); > @@ -359,7 +345,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > iEph = pParse->nTab++; > sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); > > - if (isView) { > + if (is_view) { > addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, > nKey); > } else { > @@ -372,7 +358,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > if (pWInfo == 0) > goto update_cleanup; > okOnePass = sqlite3WhereOkOnePass(pWInfo, aiCurOnePass); > - if (isView){ > + if (is_view) { > for (i = 0; i < nPk; i++) { > sqlite3VdbeAddOp3(v, OP_Column, iDataCur, i, iPk + i); > } > @@ -390,7 +376,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > nKey = nPk; > regKey = iPk; > } else { > - const char *zAff = isView ? 0 : > + const char *zAff = is_view ? 0 : > sqlite3IndexAffinityStr(pParse->db, pPk); > sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey, > zAff, nPk); > @@ -412,7 +398,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > } > > labelBreak = sqlite3VdbeMakeLabel(v); > - if (!isView) { > + if (!is_view) { > /* > * Open every index that needs updating. Note that if any > * index could potentially invoke a REPLACE conflict resolution > @@ -445,7 +431,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > labelContinue = labelBreak; > sqlite3VdbeAddOp2(v, OP_IsNull, regKey, > labelBreak); > - if (aToOpen[iDataCur - iBaseCur] && !isView) { > + if (aToOpen[iDataCur - iBaseCur] && !is_view) { > assert(pPk); > sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, > labelBreak, regKey, nKey); > @@ -552,7 +538,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > * is deleted or renamed by a BEFORE trigger - is left undefined in the > * documentation. > */ > - if (!isView) { > + if (!is_view) { > sqlite3VdbeAddOp4Int(v, OP_NotFound, iDataCur, > labelContinue, regKey, nKey); > VdbeCoverage(v); > @@ -576,7 +562,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > } > } > > - if (!isView) { > + if (!is_view) { > int addr1 = 0; /* Address of jump instruction */ > int bReplace = 0; /* True if REPLACE conflict resolution might happen */ > > @@ -691,15 +677,3 @@ sqlite3Update(Parse * pParse, /* The parser context */ > sql_expr_free(db, pWhere, false); > return; > } > - > -/* Make sure "isView" and other macros defined above are undefined. Otherwise > - * they may interfere with compilation of other functions in this file > - * (or in another file, if this file becomes part of the amalgamation). > - */ > -#ifdef isView > -#undef isView > -#endif > -#ifdef pTrigger > -#undef pTrigger > -#endif > - > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 455198d..9acb9b6 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4614,7 +4614,7 @@ case OP_IdxGE: { /* jump */ > */ > case OP_Clear: { > assert(pOp->p1 > 0); > - uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1); > + uint32_t space_id = pOp->p1; > struct space *space = space_by_id(space_id); > assert(space != NULL); > rc = tarantoolSqlite3ClearTable(space); > diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua > index 61d7756..810ca8a 100755 > --- a/test/sql-tap/delete1.test.lua > +++ b/test/sql-tap/delete1.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test = require("sqltester") > -test:plan(7) > +test:plan(9) > > --!./tcltestrunner.lua > -- ["set","testdir",[["file","dirname",["argv0"]]]] > @@ -131,5 +131,26 @@ test:do_test( > -- </delete1-5.0> > }) > > +-- Tests for data dictionary integration. > +s = box.schema.create_space('t') > +i = s:create_index('i', {parts={1, 'unsigned'}}) > +test:do_test( > + "delete1-6.0", > + function() > + s:replace({1}) > + s:replace({2}) > + s:replace({3}) > + return s:count() > + end, > + 3) > + > +test:do_test( > + "delete1-6.1", > + function() > + box.sql.execute([[delete from "t"]]) > + return s:count() > + end, > + 0) > + > test:finish_test() > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-21 11:39 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-18 13:36 [tarantool-patches] [PATCH] sql: refactor SQL delete to allow Lua spaces Kirill Yukhin 2018-05-18 21:07 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-21 8:20 ` Kirill Yukhin 2018-05-21 9:47 ` Vladislav Shpilevoy 2018-05-21 10:25 ` Kirill Yukhin 2018-05-21 10:49 ` Vladislav Shpilevoy 2018-05-21 11:24 ` Kirill Yukhin 2018-05-21 11:39 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox