From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C8A182534B for ; Mon, 18 Jun 2018 08:29:56 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4zBhgqRLYrF9 for ; Mon, 18 Jun 2018 08:29:56 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5D2E925347 for ; Mon, 18 Jun 2018 08:29:56 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts From: "n.pettik" In-Reply-To: <5b82e05e-3977-5189-7291-c302712dda58@tarantool.org> Date: Mon, 18 Jun 2018 15:29:53 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8E3011EE-AF57-4055-AE08-58DABDB1CC40@tarantool.org> References: <16d25107-35e6-d6db-d0d5-01a47f1d3909@tarantool.org> <90D2A28D-188B-45EB-89A9-43462A3ECF36@tarantool.org> <5b82e05e-3977-5189-7291-c302712dda58@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy Thanks for fixes, I have squashed them. > 1. How about rename this function to parser_emit_open_cursor? > According to Tarantool naming convention a method name should > be called as 'object_action_subject'. >=20 > Now we have too many different prefixes of parser methods: > sqlite3, sql, parse, parser, . Lets use 'parser=E2=80=99. Ok, but I decided to call it vdbe_emit_open_cursor() since all staff = connected with code generation rather belongs to vdbe: +++ b/src/box/sql/analyze.c @@ -178,7 +178,7 @@ openStatTable(Parse * pParse, /* Parsing = context */ for (i =3D 0; aTable[i]; i++) { struct space *space =3D space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i])); - sql_emit_open_cursor(pParse, iStatCur + i, aRoot[i], = space); + vdbe_emit_open_cursor(pParse, iStatCur + i, aRoot[i], = space); +++ b/src/box/sql/build.c @@ -1181,8 +1181,8 @@ space_checks_expr_list(uint32_t space_id) } =20 int -sql_emit_open_cursor(struct Parse *parse_context, int cursor, int = index_id, - struct space *space) +vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int = index_id, + struct space *space) { assert(space !=3D NULL); struct Vdbe *vdbe =3D parse_context->pVdbe; @@ -2651,7 +2651,7 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, = int memRootPage) sqlite3VdbeAddOp2(v, OP_Clear, = SQLITE_PAGENO_TO_SPACEID(tnum), 0); struct space *space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); - sql_emit_open_cursor(pParse, iIdx, tnum, space); + vdbe_emit_open_cursor(pParse, iIdx, tnum, space); +++ b/src/box/sql/expr.c @@ -2489,8 +2489,8 @@ sqlite3FindInIndex(Parse * pParse, /* = Parsing context */ P4_DYNAMIC); struct space *space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); - sql_emit_open_cursor(pParse, = iTab, - pIdx->tnum, = space); + vdbe_emit_open_cursor(pParse, = iTab, + = pIdx->tnum, space); +++ b/src/box/sql/insert.c @@ -55,7 +55,7 @@ sqlite3OpenTable(Parse * pParse, /* Generate code = into this VDBE */ assert(pPk !=3D 0); assert(pPk->tnum =3D=3D pTab->tnum); struct space *space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum)); - sql_emit_open_cursor(pParse, iCur, pPk->tnum, space); + vdbe_emit_open_cursor(pParse, iCur, pPk->tnum, space); @@ -1943,11 +1941,11 @@ xferOptimization(Parse * pParse, /* = Parser context */ assert(pSrcIdx); struct space *src_space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum)); - sql_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, = src_space); + vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, = src_space); VdbeComment((v, "%s", pSrcIdx->zName)); struct space *dest_space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum)); - sql_emit_open_cursor(pParse, iDest, pDestIdx->tnum, = dest_space); + vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, = dest_space); +++ b/src/box/sql/select.c @@ -6107,9 +6107,9 @@ sqlite3Select(Parse * pParse, /* The = parser context */ * Open the cursor, execute the = OP_Count, * close the cursor. */ - sql_emit_open_cursor(pParse, cursor, - space->def->id << = 10, - space); + vdbe_emit_open_cursor(pParse, cursor, + space->def->id << = 10, + space); +++ b/src/box/sql/sqliteInt.h @@ -3564,8 +3564,8 @@ void sqlite3EndTable(Parse *, Token *, Token *, = Select *); * @retval address of last opcode. */ int -sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, - struct space *space); +vdbe_emit_open_cursor(struct Parse *parse, int cursor, int index_id, + struct space *space); >>>=20 >>>> +} >>>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c >>>> index 2b59130..de4e0c1 100644 >>>> --- a/src/box/sql/delete.c >>>> +++ b/src/box/sql/delete.c >>>> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, = struct SrcList *tab_list, >>>> memset(&nc, 0, sizeof(nc)); >>>> nc.pParse =3D parse; >>>> if (tab_list->a[0].pTab =3D=3D NULL) { >>>> - struct Table *t =3D malloc(sizeof(struct = Table)); >>>> + struct Table *t =3D calloc(sizeof(struct Table), = 1); >>>=20 >>> 4. It must the part of the previous patch. >> Refactored this way: >> +++ b/src/box/sql/delete.c >> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, = struct SrcList *tab_list, >> strlen(tab_name)); >> if (space_id =3D=3D BOX_ID_NIL) >> goto delete_from_cleanup; >> + /* >> + * In this case space has been created via Lua >> + * facilities, so there is no corresponding entry >> + * in table hash. Thus, lets create simple >> + * wrapper around space_def to support interface. >> + */ >> + space =3D space_by_id(space_id); >> + tab_list->a[0].pTab =3D = sql_table_construct_from_space(space); >> + if (tab_list->a[0].pTab =3D=3D NULL) >> + goto delete_from_cleanup; >=20 > 2. As I proposed in the review, why can not we use stack struct Table = here? > Just declare it at the beginning of the function and here save its = pointer. Well, OK it is possible: +++ b/src/box/sql/delete.c @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct = SrcList *tab_list, * wrapper around space_def to support interface. */ space =3D space_by_id(space_id); - tab_list->a[0].pTab =3D = sql_table_construct_from_space(space); - if (tab_list->a[0].pTab =3D=3D NULL) - goto delete_from_cleanup; + struct Table tmp_tab; + memset(&tmp_tab, 0, sizeof(tmp_tab)); + tmp_tab.def =3D space->def; + /* Prevent from freeing memory in DeleteTable. */ + tmp_tab.nTabRef =3D 2; + tab_list->a[0].pTab =3D &tmp_tab; -struct Table * -sql_table_construct_from_space(const struct space *space) -{ - assert(space !=3D NULL); - struct Table *table =3D calloc(1, sizeof(*table)); - if (table =3D=3D NULL) { - diag_set(OutOfMemory, sizeof(table), "calloc", "table"); - return NULL; - } - table->def =3D space_def_dup(space->def); - if (table->def =3D=3D NULL) - return NULL; - return table; -} - - -/** - * Create wrapper around space_def of the given space. - * This routine is required to support original SQLite interface, - * which accepts struct Table and struct Index DD arguments. - * Both table and space def are allocated on heap. - * - * @param space Space to be prototype. - * @retval New instance of struct Table allocated on malloc, - * or NULL in case of OOM. - */ -struct Table * -sql_table_construct_from_space(const struct space *space); - >=20 >=20 >> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index = *index) >> char * >> sql_index_affinity_str(struct sqlite3 * db, struct index_def *def) >> { >> - char *aff; >> - /* The first time a column affinity string for a particular = index is >> - * required, it is allocated and populated here. It is then = stored as >> - * a member of the Index structure for subsequent use. >> - * >> - * The column affinity string will eventually be deleted by >> - * sqliteDeleteIndex() when the Index structure itself is = cleaned >> - * up. >> - */ >> - int nColumn =3D def->key_def->part_count; >> - aff =3D (char *)sqlite3DbMallocRaw(0, nColumn + 1); >> + uint32_t column_count =3D def->key_def->part_count; >> + char *aff =3D (char *)sqlite3DbMallocRaw(0, column_count + = 1); >> if (aff =3D=3D NULL) { >> sqlite3OomFault(db); >> - return 0; >> + return NULL; >> } >=20 > 3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for = us. Ok: @@ -108,11 +108,9 @@ char * sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) { uint32_t column_count =3D def->key_def->part_count; - char *aff =3D (char *)sqlite3DbMallocRaw(0, column_count + 1); - if (aff =3D=3D NULL) { - sqlite3OomFault(db); + char *aff =3D (char *)sqlite3DbMallocRaw(db, column_count + 1); + if (aff =3D=3D NULL) return NULL; - } >> 11 files changed, 256 insertions(+), 253 deletions(-) >>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c >> index 23e16189a..e7d9723fa 100644 >> --- a/src/box/sql/resolve.c >> +++ b/src/box/sql/resolve.c >> @@ -39,6 +39,9 @@ >> #include >> #include >> +#include "box/box.h" >> +#include "box/schema.h" >=20 > 4. Why do you need these headers? Looks like I forgot to remove them. Anyway, in your fixes you removed = them. Thx. >> */ >> static int >> -sql_where_shortcut(struct WhereLoopBuilder *builder) >> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause = *where, >> + int cursor, struct space_def *space_def, >> + struct index_def *idx_def) >=20 > 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop = method has prefix > 'where_'? Lets make it 'where_clause_find_term' and = 'where_loop_assign_terms=E2=80=99. -sql_where_find_term(struct WhereClause *where_clause, int cursor, int = column, - Bitmask is_ready, u32 op, struct space_def = *space_def, - struct key_def *key_def) +where_clase_find_term(struct WhereClause *where_clause, int cursor, int = column, + Bitmask is_ready, u32 op, struct space_def *space_def, + struct key_def *key_def) @@ -4133,7 +4133,7 @@ wherePathSolver(WhereInfo * pWInfo, LogEst = nRowEst) * @retval 0 on success, -1 otherwise. */ static int -where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause = *where, +where_loop_assign_terms(struct WhereLoop *loop, struct WhereClause = *where, int cursor, struct space_def *space_def, struct index_def *idx_def) @@ -4144,9 +4144,9 @@ where_assign_loop_terms(struct WhereLoop *loop, = struct WhereClause *where, uint32_t i; for (i =3D 0; i < column_count; ++i) { struct WhereTerm *term =3D - sql_where_find_term(where, cursor, i, 0, WO_EQ, - space_def, idx_def !=3D NULL = ? - idx_def->key_def : NULL); + where_clause_find_term(where, cursor, i, 0, = WO_EQ, + space_def, idx_def !=3D = NULL ? + idx_def->key_def : NULL); @@ -4178,7 +4178,7 @@ where_assign_loop_terms(struct WhereLoop *loop, = struct WhereClause *where, * if this query needs the general-purpose query planner. */ static int -sql_where_shortcut(struct WhereLoopBuilder *builder) +where_loop_builder_shortcut(struct WhereLoopBuilder *builder) { struct WhereInfo *where_info =3D builder->pWInfo; if (where_info->wctrlFlags & WHERE_OR_SUBCLAUSE) @@ -4214,7 +4214,7 @@ sql_where_shortcut(struct WhereLoopBuilder = *builder) space->index[i]->def; if (!idx_def->opts.is_unique) continue; - if (where_assign_loop_terms(loop, = clause, + if (where_loop_assign_terms(loop, = clause, cursor, = space_def, idx_def) =3D=3D= 0) break; @@ -4222,7 +4222,7 @@ sql_where_shortcut(struct WhereLoopBuilder = *builder) } else { /* Space is ephemeral. */ assert(space_def->id =3D=3D 0); - where_assign_loop_terms(loop, clause, cursor, + where_loop_assign_terms(loop, clause, cursor, space_def, NULL); @@ -4537,7 +4537,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser = context */ } #endif =20 - if (nTabList !=3D 1 || sql_where_shortcut(&sWLB) =3D=3D 0) { + if (nTabList !=3D 1 || where_loop_builder_shortcut(&sWLB) =3D=3D = 0) { >=20 >> +/** >> + * Most queries use only a single table (they are not joins) and >> + * have simple =3D=3D constraints against indexed fields. This >> + * routine attempts to plan those simple cases using much less >> + * ceremony than the general-purpose query planner, and thereby >> + * yield faster sqlite3_prepare() times for the common case. >> + * >> + * @param builder Where-Loop Builder. >> + * @retval Return non-zero on success, i.e. if this query can be >> + * handled by this no-frills query planner. Return zero >> + * if this query needs the general-purpose query planner. >> + */ >> +static int >> +sql_where_shortcut(struct WhereLoopBuilder *builder) >=20 > 6. Same. For example, 'where_loop_builder_shortcut=E2=80=99. Fixed. See code above.