[tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
n.pettik
korablev at tarantool.org
Mon Jun 18 15:29:53 MSK 2018
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'.
>
> Now we have too many different prefixes of parser methods:
> sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser’.
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 = 0; aTable[i]; i++) {
struct space *space =
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)
}
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 != NULL);
struct Vdbe *vdbe = 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 = 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 =
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 != 0);
assert(pPk->tnum == pTab->tnum);
struct space *space = 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 =
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 =
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);
>>>
>>>> +}
>>>> 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 = parse;
>>>> if (tab_list->a[0].pTab == NULL) {
>>>> - struct Table *t = malloc(sizeof(struct Table));
>>>> + struct Table *t = calloc(sizeof(struct Table), 1);
>>>
>>> 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 == 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 = space_by_id(space_id);
>> + tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> + if (tab_list->a[0].pTab == NULL)
>> + goto delete_from_cleanup;
>
> 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 = space_by_id(space_id);
- tab_list->a[0].pTab = sql_table_construct_from_space(space);
- if (tab_list->a[0].pTab == NULL)
- goto delete_from_cleanup;
+ struct Table tmp_tab;
+ memset(&tmp_tab, 0, sizeof(tmp_tab));
+ tmp_tab.def = space->def;
+ /* Prevent from freeing memory in DeleteTable. */
+ tmp_tab.nTabRef = 2;
+ tab_list->a[0].pTab = &tmp_tab;
-struct Table *
-sql_table_construct_from_space(const struct space *space)
-{
- assert(space != NULL);
- struct Table *table = calloc(1, sizeof(*table));
- if (table == NULL) {
- diag_set(OutOfMemory, sizeof(table), "calloc", "table");
- return NULL;
- }
- table->def = space_def_dup(space->def);
- if (table->def == 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);
-
>
>
>> @@ -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 = def->key_def->part_count;
>> - aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
>> + uint32_t column_count = def->key_def->part_count;
>> + char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
>> if (aff == NULL) {
>> sqlite3OomFault(db);
>> - return 0;
>> + return NULL;
>> }
>
> 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 = def->key_def->part_count;
- char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
- if (aff == NULL) {
- sqlite3OomFault(db);
+ char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1);
+ if (aff == 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 <stdlib.h>
>> #include <string.h>
>> +#include "box/box.h"
>> +#include "box/schema.h"
>
> 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)
>
> 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’.
-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 = 0; i < column_count; ++i) {
struct WhereTerm *term =
- sql_where_find_term(where, cursor, i, 0, WO_EQ,
- space_def, idx_def != NULL ?
- idx_def->key_def : NULL);
+ where_clause_find_term(where, cursor, i, 0, WO_EQ,
+ space_def, idx_def != 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 = 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) == 0)
break;
@@ -4222,7 +4222,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
} else {
/* Space is ephemeral. */
assert(space_def->id == 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
- if (nTabList != 1 || sql_where_shortcut(&sWLB) == 0) {
+ if (nTabList != 1 || where_loop_builder_shortcut(&sWLB) == 0) {
>
>> +/**
>> + * Most queries use only a single table (they are not joins) and
>> + * have simple == 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)
>
> 6. Same. For example, 'where_loop_builder_shortcut’.
Fixed. See code above.
More information about the Tarantool-patches
mailing list