[tarantool-patches] Re: [PATCH 5/7] sql: remove lookups in Table hash

n.pettik korablev at tarantool.org
Mon Sep 3 02:52:19 MSK 2018


> On 23/08/2018 19:55, Nikita Pettik wrote:
>> Instead of looking at Table hash lets extract space from Tarantool
>> internals and create Table wrapper around it.
> 
> The major question - why do we still need struct Table? Just because
> of Table.nTabRef? This looks like an insignificant stopper since
> anyway we need refs for struct space and multiversion transactional
> DDL. We could create surrogate struct spaces for some temporary things
> instead of surrogate Tables. And do not create anything for ordinal
> spaces.

I guess it is possible to come up with workaround and avoid
using nTabRef. The real reason was to reduce size of patch and
finish DD ASAP: complete removing struct Table seems to result
in quite huge diff (struct Table still is included to struct Expr).
So, in theory nothing prevents us from removing struct Table
from source code except for time and meticulous refactoring.

>>  13 files changed, 204 insertions(+), 245 deletions(-)
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 79dc46592..840604aa3 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -217,22 +217,10 @@ sqlite3CommitInternalChanges()
>>  	current_session()->sql_flags &= ~SQLITE_InternChanges;
>>  }
>>  -/*
>> - * Return true if given column is part of primary key.
>> - * If field number is less than 63, corresponding bit
>> - * in column mask is tested. Otherwise, check whether 64-th bit
>> - * in mask is set or not. If it is set, then iterate through
>> - * key parts of primary index and check field number.
>> - * In case it isn't set, there are no key columns among
>> - * the rest of fields.
>> - */
>>  bool
>> -table_column_is_in_pk(Table *table, uint32_t column)
>> +sql_space_column_is_in_pk(struct space *space, uint32_t column)
>>  {
>> -	struct space *space = space_by_id(table->def->id);
>> -	assert(space != NULL);
>> -
>> -	struct index *primary_idx = index_find(space, 0 /* PK */);
>> +	struct index *primary_idx = space->index[0];
> 
> 1. space_index(space, 0).

Ok, I slightly refactored this part:

+++ b/src/box/sql/build.c
@@ -221,10 +221,10 @@ sqlite3CommitInternalChanges()
 bool
 sql_space_column_is_in_pk(struct space *space, uint32_t column)
 {
-       struct index *primary_idx = space->index[0];
-       /* Views don't have any indexes. */
-       if (primary_idx == NULL)
+       if (space->def->opts.is_view)
                return false;
+       struct index *primary_idx = space_index(space, 0);
+       assert(primary_idx != NULL);
        struct key_def *key_def = primary_idx->def->key_def;
        uint64_t pk_mask = key_def->column_mask;
        if (column < 63)


> Also I see below check for idx == NULL.
> But if a space has no indexes, its space->index is NULL as well
> AFAIK. How does it work? The previous version was checking if a
> space has an index.

It is not true, actually. You can check it by adding assert():

 sql_space_column_is_in_pk(struct space *space, uint32_t column)
 {
-       struct index *primary_idx = space->index[0];
-       /* Views don't have any indexes. */
-       if (primary_idx == NULL)
+       if (space->def->opts.is_view) {
+               assert(space->index == NULL);
                return false;
+       }

And launch sql-tap/trigger9.test.lua. It would fire this assert,
even despite the fact that VIEW has no any indexes.

> 
>>  	/* Views don't have any indexes. */
>>  	if (primary_idx == NULL)
>>  		return false;
>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>> index 0f285cc8b..a457a71d1 100644
>> --- a/src/box/sql/delete.c
>> +++ b/src/box/sql/delete.c
>> @@ -36,16 +36,31 @@
>>  #include "tarantoolInt.h"
>>    struct Table *
>> -sql_list_lookup_table(struct Parse *parse, SrcList *src_list)
>> +sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
>>  {
>> -	struct SrcList_item *item = src_list->a;
>> -	assert(item != NULL && src_list->nSrc == 1);
>> -	struct Table *table = sqlite3LocateTable(parse, 0, item->zName);
>> -	sqlite3DeleteTable(parse->db, item->pTab);
>> -	item->pTab = table;
>> -	if (table != NULL)
>> -		item->pTab->nTabRef++;
>> -	if (sqlite3IndexedByLookup(parse, item) != 0)
>> +	assert(tbl_name != NULL);
>> +	uint32_t space_id = box_space_id_by_name(tbl_name->zName,
>> +						 strlen(tbl_name->zName));
>> +	sqlite3DeleteTable(parse->db, tbl_name->pTab);
> 
> 2. What is happening here? If tbl_name->pTab != NULL, why do you
> recreate it?

I checked, it can’t be != NULL. Lets add simple assert:

+++ b/src/box/sql/delete.c
@@ -39,9 +39,9 @@ struct Table *
 sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
 {
        assert(tbl_name != NULL);
+       assert(tbl_name->pTab == NULL);
        uint32_t space_id = box_space_id_by_name(tbl_name->zName,
                                                 strlen(tbl_name->zName));
-       sqlite3DeleteTable(parse->db, tbl_name->pTab);
        struct space *space = space_by_id(space_id);
        if (space_id == BOX_ID_NIL || space == NULL) {
                sqlite3ErrorMsg(parse, "no such table: %s", tbl_name->zName);

+++ b/src/box/sql/select.c
@@ -4811,7 +4811,6 @@ selectExpander(Walker * pWalker, Select * p)
                         * An ordinary table or view name in the
                         * FROM clause.
                         */
-                       assert(pFrom->pTab == NULL);
                        pTab = sql_lookup_table(pParse, pFrom);
                        if (pTab == NULL)
                                return WRC_Abort;

> 
>> +	struct space *space = space_by_id(space_id);
>> +	if (space_id == BOX_ID_NIL || space == NULL) {
> 
> 3. If 'space_id' == BOX_ID_NIL then space == NULL as well,
> always.

Not quite. Check out this issue https://github.com/tarantool/tarantool/issues/3611
and patch resolving it 16c3989c7432c27a0e06ff1fc4bf571d63d394ff

Here we may face with the same problem. If I removed this additional check,
sql/checks.test.lua would fail.

> You should not even try to lookup a space when
> box_space_id_by_name returned BOX_ID_NIL.
> 
>> +		sqlite3ErrorMsg(parse, "no such table: %s", tbl_name->zName);
> 
> 4. ER_NO_SUCH_SPACE.

I left this error message on purpose: firstly replacing it to ’no such space’
would lead to massive refactoring in test suite; secondly, I am not sure
that we should use ’space’ terminology in SQL. For instance, Peter is
against even using ‘field’ instead of ‘column’: https://github.com/tarantool/tarantool/issues/3621

>Minor problem: the error message contains the word 'field' rather than 'column’.

If we finally decide to use ’space’ term in SQL, I will fix it.
*In before* I know that in some SQL parts we already use errors messages
containing Tarantool’s terminology, but anyway…
As a compromise I can use ER_SQL and “no such table”.

> 
>> +		return NULL;
>> +	}
>> +	assert(space != NULL);
>> +	if (space->def->field_count == 0) {
>> +		sqlite3ErrorMsg(parse, "no format for space: %s",
>> +				tbl_name->zName);
> 
> 5. ER_UNSUPPORTED or a new error code.

Ok:

+++ b/src/box/sql/delete.c
@@ -49,8 +49,10 @@ sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
        }
        assert(space != NULL);
        if (space->def->field_count == 0) {
-               sqlite3ErrorMsg(parse, "no format for space: %s",
-                               tbl_name->zName);
+               diag_set(ClientError, ER_UNSUPPORTED, "SQL",
+                        "space without format");
+               parse->rc = SQL_TARANTOOL_ERROR;
+               parse->nErr++;
                return NULL;
        }
        struct Table *table = sqlite3DbMallocZero(parse->db, sizeof(*table));
diff --git a/test/sql-tap/lua-tables.test.lua b/test/sql-tap/lua-tables.test.lua
index 0e79c61a8..19d80b611 100755
--- a/test/sql-tap/lua-tables.test.lua
+++ b/test/sql-tap/lua-tables.test.lua
@@ -46,7 +46,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "lua-tables-5",
     [[SELECT * from "t1"]],
-    {1, "no format for space: t1"}
+    {1, "SQL does not support space without format"}

> 
>> +		return NULL;
>> +	}
>> +	struct Table *table = sqlite3DbMallocZero(parse->db, sizeof(*table));
>> +	if (table == NULL)
>> +		return NULL;
>> +	table->def = space_def_dup(space->def);
>> +	table->space = space;
>> +	table->nTabRef = 1;
>> +	tbl_name->pTab = table;
>> +	if (sqlite3IndexedByLookup(parse, tbl_name) != 0)
>>  		table = NULL;
> 
> 6. And here table leaks.

No, it doesn’t:

 tbl_name->pTab = table;

sql_lookup_table will return NULL and launch cleanup mechanism in
caller function which in turn calls sqlite3SrcListDelete().

> 
>>  	return table;
>>  }
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index cb4f89e35..7b6300c75 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -278,7 +278,7 @@ comparisonAffinity(Expr * pExpr)
>>  		aff =
>>  		    sqlite3CompareAffinity(pExpr->x.pSelect->pEList->a[0].pExpr,
>>  					   aff);
>> -	} else if (NEVER(aff == 0)) {
>> +	} else {
>>  		aff = AFFINITY_BLOB;
>>  	}
>>  	return aff;
> 
> 7. Why? How is affinity linked with the patch?

Look, spaces created outside SQL have fields with affinity == AFFINITY_UNDEFINED,
unless otherwise indicated:

const struct field_def field_def_default = {
       .type = FIELD_TYPE_ANY,
       .affinity = AFFINITY_UNDEFINED,
       .name = NULL,
       .is_nullable = false,
       .nullable_action = ON_CONFLICT_ACTION_DEFAULT,
       .coll_id = COLL_NONE,
       .default_value = NULL,
       .default_value_expr = NULL
};

Thus, in some cases it may result in reaching that ‘else’ branch.
For example, SELECT count(*) FROM "_space" WHERE "name" IN ('T', 'S’);
I’m not going to dive into details, it is quite boring (but if you want to I can
attach call stack and explain what is going on).

> 
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index 9220d34e1..d3950254e 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -1234,40 +1233,40 @@ xferOptimization(Parse * pParse,	/* Parser context */
>>  	 * we have to check the semantics.
>>  	 */
>>  	pItem = pSelect->pSrc->a;
>> -	pSrc = sqlite3LocateTable(pParse, 0, pItem->zName);
>> +	uint32_t src_id = box_space_id_by_name(pItem->zName,
>> +						strlen(pItem->zName));
>>  	/* FROM clause does not contain a real table. */
>> -	if (pSrc == NULL)
>> +	if (src_id == BOX_ID_NIL)
>>  		return 0;
>> +	struct space *src = space_by_id(src_id);
>> +	assert(src != NULL);
> 
> 8. In this function you do not need pDest as Table. It is
> enough to pass a space here. Only sqlite3OpenTable still
> uses struct Table in this function, but anyway in the first
> lines it does space lookup. I fixed this remark partially and
> dirty. Please, finish my fixes (refactor or remove
> sqlite3OpenTable etc).

Ok, fixed:

+++ b/src/box/sql/insert.c
@@ -152,7 +152,7 @@ vdbe_has_table_read(struct Parse *parser, const struct Table *table)
 /* Forward declaration */
 static int
 xferOptimization(Parse * pParse,       /* Parser context */
-                Table * pDest, /* The table we are inserting into */
+                struct space *dest,
                 Select * pSelect,      /* A SELECT statement to use as the data source */
                 int onError);  /* How to handle constraint errors */
 
@@ -358,7 +358,8 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
         *
         * This is the 2nd template.
         */
-       if (pColumn == 0 && xferOptimization(pParse, pTab, pSelect, on_error)) {
+       if (pColumn == 0 &&
+           xferOptimization(pParse, pTab->space, pSelect, on_error)) {
                assert(trigger == NULL);
                assert(pList == 0);
                goto insert_end;
@@ -1131,7 +1132,7 @@ sql_index_is_xfer_compatible(const struct index_def *dest,
  */
 static int
 xferOptimization(Parse * pParse,       /* Parser context */
-                Table * pDest,         /* The table we are inserting into */
+                struct space *dest,
                 Select * pSelect,      /* A SELECT statement to use as the data source */
                 int onError)           /* How to handle constraint errors */
 {
@@ -1145,8 +1146,6 @@ xferOptimization(Parse * pParse,  /* Parser context */
        int regData, regTupleid;        /* Registers holding data and tupleid */
        struct session *user_session = current_session();
        bool is_err_action_default = false;
-       struct space *dest = space_by_id(pDest->def->id);
-       assert(dest != NULL);
 
        if (pSelect == NULL)
                return 0;       /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -1298,7 +1297,7 @@ xferOptimization(Parse * pParse,  /* Parser context */
        regTupleid = sqlite3GetTempReg(pParse);
 
        vdbe_emit_open_cursor(pParse, iDest, 0, dest);
-       VdbeComment((v, "%s", pDest->def->name));
+       VdbeComment((v, "%s", dest->def->name));

> 
>> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
>> index 6173462ff..063836989 100644
>> --- a/src/box/sql/pragma.c
>> +++ b/src/box/sql/pragma.c
>> @@ -245,6 +245,95 @@ sql_default_engine_set(const char *engine_name)
>>  	return 0;
>>  }
>>  +/**
>> + * This function handles PRAGMA TABLE_INFO(<table>).
>> + *
>> + * Return a single row for each column of the named table.
>> + * The columns of the returned data set are:
>> + *
>> + * - cid: Column id (numbered from left to right, starting at 0);
>> + * - name: Column name;
>> + * - type: Column declaration type;
>> + * - notnull: True if 'NOT NULL' is part of column declaration;
>> + * - dflt_value: The default value for the column, if any.
>> + *
>> + * @param parse Current parsing context.
>> + * @param tbl_name Name of table to be examined.
>> + */
>> +static void
>> +sql_pragma_table_info(struct Parse *parse, const char *tbl_name)
>> +{
>> +	if (tbl_name == NULL)
>> +		return;
>> +	uint32_t space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
>> +	if (space_id == BOX_ID_NIL)
>> +		return;
>> +	struct space *space = space_by_id(space_id);
>> +	assert(space != NULL);
>> +	struct space_def *def = space->def;
>> +	struct index *pk = space->index[0];
> 
> 9. space_index(). What is strange, you use space_index sometimes.

I use (and used) space_index() when it comes for real space from cache.
When we are operating on ‘fake’ space from struct Table (like in previous patch)
we can’t use map (such spaces simply lack it). Starting from this patch, we can
use map everywhere except for building (i.e. creating table/index) routine.

Also, I see a lot of usages of index[0] through the sever code, so
what is wrong with using it?

+++ b/src/box/sql/pragma.c
@@ -270,7 +270,7 @@ sql_pragma_table_info(struct Parse *parse, const char *tbl_name)
                return;
        struct space *space = space_by_id(space_id);
        assert(space != NULL);
-       struct index *pk = space->index[0];
+       struct index *pk = space_index(space, 0);

> 
>> +	parse->nMem = 6;
>> +	if (def->opts.is_view) {
>> +		const char *sql = def->opts.sql;
>> +		sql_view_assign_cursors(parse, sql);
>> +	}
>> +	struct Vdbe *v = sqlite3GetVdbe(parse);
>> +	for (uint32_t i = 0, k; i < def->field_count; ++i) {
>> +		if (!sql_space_column_is_in_pk(space, i)) {
>> +			k = 0;
>> +		} else if (pk == NULL) {
>> +			k = 1;
>> +		} else {
>> +			struct key_def *kdef = pk->def->key_def;
>> +			k = key_def_find(kdef, i) - kdef->parts + 1;
>> +		}
>> +		bool is_nullable = def->fields[i].is_nullable;
>> +		char *expr_str = def->fields[i].default_value;
>> +		const char *name = def->fields[i].name;
>> +		enum field_type type = def->fields[i].type;
>> +		sqlite3VdbeMultiLoad(v, 1, "issisi", i, name,
>> +				     field_type_strs[type], !is_nullable,
>> +				     expr_str, k);
>> +		sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 6);
>> +	}
>> +}
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 3ae8db1bc..08893c473 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -3333,6 +3333,11 @@ void sqlite3ExprListSetSpan(Parse *, ExprList *, ExprSpan *);
>>  u32 sqlite3ExprListFlags(const ExprList *);
>>  int sqlite3Init(sqlite3 *);
>>  +void sqlite3Pragma(Parse *, Token *, Token *, Token *, int);
>> +void sqlite3ResetAllSchemasOfConnection(sqlite3 *);
>> +void sqlite3CommitInternalChanges();
>> +void sqlite3DeleteColumnNames(sqlite3 *, Table *);
> 
> 10. Why do you need these movements?

Idk, some refactoring artefacts I guess. Moved it back:

+++ b/src/box/sql/sqliteInt.h
@@ -3331,11 +3331,6 @@ void sqlite3ExprListSetSpan(Parse *, ExprList *, ExprSpan *);
 u32 sqlite3ExprListFlags(const ExprList *);
 int sqlite3Init(sqlite3 *);
 
-void sqlite3Pragma(Parse *, Token *, Token *, Token *, int);
-void sqlite3ResetAllSchemasOfConnection(sqlite3 *);
-void sqlite3CommitInternalChanges();
-void sqlite3DeleteColumnNames(sqlite3 *, Table *);
-
 /**
  * This is the callback routine for the code that initializes the
  * database.  See sqlite3Init() below for additional information.
@@ -3354,6 +3349,11 @@ int
 sql_init_callback(struct init_data *init, const char *name,
                  uint32_t space_id, uint32_t index_id, const char *sql);
 
+void sqlite3Pragma(Parse *, Token *, Token *, Token *, int);
+void sqlite3ResetAllSchemasOfConnection(sqlite3 *);
+void sqlite3CommitInternalChanges();
+void sqlite3DeleteColumnNames(sqlite3 *, Table *);
+
 /**
  * Return true if given column is part of primary key.
  * If field number is less than 63, corresponding bit

> 
>> +
>>  /**
>>   * This is the callback routine for the code that initializes the
>>   * database.  See sqlite3Init() below for additional information.
>> diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
>> index 2b37e3ad5..05ed50192 100755
>> --- a/test/sql-tap/analyze9.test.lua
>> +++ b/test/sql-tap/analyze9.test.lua
>> @@ -1021,7 +1021,7 @@ test:do_execsql_test(
>>          INSERT INTO x1 VALUES(3, 4);
>>          INSERT INTO x1 VALUES(5, 6);
>>          ANALYZE;
>> -        INSERT INTO "_sql_stat4" VALUES('x1', 'abc', 0, 0, 0, '');
>> +        INSERT INTO "_sql_stat4" VALUES('x1', 'abc', '', '', '', '');
> 
> 11. Why are tests changed? As I understand, you just did refactoring.

According to format of _sql_stat4, we should pass empty string
instead of integers 0. After fetching space from Tarantool, this
test started to fail:
"Tuple field 3 type does not match one required by operation: expected string #"  
(and it seems to be fair, IDK why it passed before).

> 
>>      ]])
>>    test:do_execsql_test(
>> diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua
>> index 04b7674ed..ac05a98b2 100755
>> --- a/test/sql-tap/join.test.lua
>> +++ b/test/sql-tap/join.test.lua
>> @@ -1080,9 +1080,9 @@ jointest("join-12.9", 1000, {1, 'at most 64 tables in a join'})
>>  --    if X(703, "X!cmd", [=[["expr","[lsearch [db eval {PRAGMA compile_options}] MEMDEBUG]<0"]]=])
>>  -- then
>>  jointest("join-12.10", 65534, {1, 'at most 64 tables in a join'})
>> -jointest("join-12.11", 65535, {1, 'too many references to "T14": max 65535'})
>> -jointest("join-12.12", 65536, {1, 'too many references to "T14": max 65535'})
>> -jointest("join-12.13", 65537, {1, 'too many references to "T14": max 65535'})
>> +jointest("join-12.11", 65535, {1, 'at most 64 tables in a join'})
>> +jointest("join-12.12", 65536, {1, 'at most 64 tables in a join'})
>> +jointest("join-12.13", 65537, {1, 'at most 64 tables in a join’})

And these tests are changed since we don’t use tables from hash:
previously we fetch table from hash increasing its ref counter, i.e.
one table instance was shared. Now, each invocation to table results
in creating of new table instance and reseting ref counter.
However, it have become even better IMHO.

>>  --    end
>>  --end
>>  diff --git a/test/sql/delete.result b/test/sql/delete.result
>> index 52f9969c1..993e9e04d 100644
>> --- a/test/sql/delete.result
>> +++ b/test/sql/delete.result
>> @@ -44,7 +44,7 @@ box.sql.execute("DROP TABLE t1;");
>>  --
>>  box.sql.execute("DELETE FROM t1;")
>>  ---
>> -- error: Space 'T1' does not exist
>> +- error: 'no such table: T1'
>>  ...
>>  box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);")
>>  ---
>> @@ -54,7 +54,7 @@ box.sql.execute("CREATE TRIGGER t2 BEFORE INSERT ON t2 BEGIN DELETE FROM t1; END
>>  ...
>>  box.sql.execute("INSERT INTO t2 VALUES (0);")
>>  ---
>> -- error: Space 'T1' does not exist
>> +- error: 'no such table: T1'
>>  ...
>>  box.sql.execute("DROP TABLE t2;")
>>  ---
> 
> My diff is on the branch and below:

Seems OK, so simply applied.

Also, I have removed index_is_unordered() function as I promised:

+++ b/src/box/sql/where.c
@@ -2690,30 +2690,6 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder,      /* The WhereLoop factory */
        return rc;
 }
 
-/**
- * Check if is_unordered flag is set for given index.
- * Since now there may not be corresponding Tarantool's index
- * (e.g. for temporary objects such as ephemeral tables),
- * a few preliminary checks are made.
- *
- * @param idx Index to be tested.
- * @retval True, if statistics exist and unordered flag is set.
- */
-static bool
-index_is_unordered(const struct index_def *idx)
-{
-       assert(idx != NULL);
-       struct space *space = space_by_id(idx->space_id);
-       if (space == NULL)
-               return false;
-       struct index *tnt_idx = space_index(space, idx->iid);
-       if (tnt_idx == NULL)
-               return false;
-       if (tnt_idx->def->opts.stat != NULL)
-               return tnt_idx->def->opts.stat->is_unordered;
-       return false;
-}
-
 /*
  * Return True if it is possible that pIndex might be useful in
  * implementing the ORDER BY clause in pBuilder.
@@ -2729,7 +2705,7 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
        ExprList *pOB;
        int ii, jj;
        int part_count = idx_def->key_def->part_count;
-       if (index_is_unordered(idx_def))
+       if (idx_def->opts.stat != NULL && idx_def->opts.stat->is_unordered)
                return 0;
        if ((pOB = pBuilder->pWInfo->pOrderBy) == 0)
                return 0;
@@ -3310,7 +3286,8 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo,     /* The WHERE clause */
                                idx_def = NULL;
                                nColumn = 1;
                        } else if ((idx_def = pLoop->index_def) == NULL ||
-                                  index_is_unordered(idx_def)) {
+                                  (idx_def->opts.stat != NULL &&
+                                   idx_def->opts.stat->is_unordered)) {
                                return 0;
                        } else {
                                nColumn = idx_def->key_def->part_count;

Full diff:

+++ b/src/box/sql/build.c
@@ -221,10 +221,10 @@ sqlite3CommitInternalChanges()
 bool
 sql_space_column_is_in_pk(struct space *space, uint32_t column)
 {
-       struct index *primary_idx = space->index[0];
-       /* Views don't have any indexes. */
-       if (primary_idx == NULL)
+       if (space->def->opts.is_view)
                return false;
+       struct index *primary_idx = space_index(space, 0);
+       assert(primary_idx != NULL);
        struct key_def *key_def = primary_idx->def->key_def;
        uint64_t pk_mask = key_def->column_mask;
        if (column < 63)
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index a457a71d1..702e5d676 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -39,9 +39,9 @@ struct Table *
 sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
 {
        assert(tbl_name != NULL);
+       assert(tbl_name->pTab == NULL);
        uint32_t space_id = box_space_id_by_name(tbl_name->zName,
                                                 strlen(tbl_name->zName));
-       sqlite3DeleteTable(parse->db, tbl_name->pTab);
        struct space *space = space_by_id(space_id);
        if (space_id == BOX_ID_NIL || space == NULL) {
                sqlite3ErrorMsg(parse, "no such table: %s", tbl_name->zName);
@@ -49,8 +49,10 @@ sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
        }
        assert(space != NULL);
        if (space->def->field_count == 0) {
-               sqlite3ErrorMsg(parse, "no format for space: %s",
-                               tbl_name->zName);
+               diag_set(ClientError, ER_UNSUPPORTED, "SQL",
+                        "space without format");
+               parse->rc = SQL_TARANTOOL_ERROR;
+               parse->nErr++;
                return NULL;
        }
        struct Table *table = sqlite3DbMallocZero(parse->db, sizeof(*table));

+++ b/src/box/sql/insert.c
@@ -152,7 +152,7 @@ vdbe_has_table_read(struct Parse *parser, const struct Table *table)
 /* Forward declaration */
 static int
 xferOptimization(Parse * pParse,       /* Parser context */
-                Table * pDest, /* The table we are inserting into */
+                struct space *dest,
                 Select * pSelect,      /* A SELECT statement to use as the data source */
                 int onError);  /* How to handle constraint errors */
 
@@ -358,7 +358,8 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
         *
         * This is the 2nd template.
         */
-       if (pColumn == 0 && xferOptimization(pParse, pTab, pSelect, on_error)) {
+       if (pColumn == 0 &&
+           xferOptimization(pParse, pTab->space, pSelect, on_error)) {
                assert(trigger == NULL);
                assert(pList == 0);
                goto insert_end;
@@ -1131,7 +1132,7 @@ sql_index_is_xfer_compatible(const struct index_def *dest,
  */
 static int
 xferOptimization(Parse * pParse,       /* Parser context */
-                Table * pDest,         /* The table we are inserting into */
+                struct space *dest,
                 Select * pSelect,      /* A SELECT statement to use as the data source */
                 int onError)           /* How to handle constraint errors */
 {
@@ -1145,8 +1146,6 @@ xferOptimization(Parse * pParse,  /* Parser context */
        int regData, regTupleid;        /* Registers holding data and tupleid */
        struct session *user_session = current_session();
        bool is_err_action_default = false;
-       struct space *dest = space_by_id(pDest->def->id);
-       assert(dest != NULL);
 
        if (pSelect == NULL)
                return 0;       /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -1298,7 +1297,7 @@ xferOptimization(Parse * pParse,  /* Parser context */
        regTupleid = sqlite3GetTempReg(pParse);
 
        vdbe_emit_open_cursor(pParse, iDest, 0, dest);
-       VdbeComment((v, "%s", pDest->def->name));
+       VdbeComment((v, "%s", dest->def->name));

diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 3451d7c79..90fe0b7da 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -270,7 +270,7 @@ sql_pragma_table_info(struct Parse *parse, const char *tbl_name)
                return;
        struct space *space = space_by_id(space_id);
        assert(space != NULL);
-       struct index *pk = space->index[0];
+       struct index *pk = space_index(space, 0);
        parse->nMem = 6;
        if (space->def->opts.is_view)
                sql_view_assign_cursors(parse, space->def->opts.sql);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index a20d0bb3f..dd8d89b99 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -4811,7 +4811,6 @@ selectExpander(Walker * pWalker, Select * p)
                         * An ordinary table or view name in the
                         * FROM clause.
                         */
-                       assert(pFrom->pTab == NULL);
                        pTab = sql_lookup_table(pParse, pFrom);
                        if (pTab == NULL)
                                return WRC_Abort;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 04716cb5c..8d8abbffc 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3331,11 +3331,6 @@ void sqlite3ExprListSetSpan(Parse *, ExprList *, ExprSpan *);
 u32 sqlite3ExprListFlags(const ExprList *);
 int sqlite3Init(sqlite3 *);
 
-void sqlite3Pragma(Parse *, Token *, Token *, Token *, int);
-void sqlite3ResetAllSchemasOfConnection(sqlite3 *);
-void sqlite3CommitInternalChanges();
-void sqlite3DeleteColumnNames(sqlite3 *, Table *);
-
 /**
  * This is the callback routine for the code that initializes the
  * database.  See sqlite3Init() below for additional information.
@@ -3354,6 +3349,11 @@ int
 sql_init_callback(struct init_data *init, const char *name,
                  uint32_t space_id, uint32_t index_id, const char *sql);
 
+void sqlite3Pragma(Parse *, Token *, Token *, Token *, int);
+void sqlite3ResetAllSchemasOfConnection(sqlite3 *);
+void sqlite3CommitInternalChanges();
+void sqlite3DeleteColumnNames(sqlite3 *, Table *);
+
 /**
  * Return true if given column is part of primary key.
  * If field number is less than 63, corresponding bit

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/where.c
@@ -2690,30 +2690,6 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder,      /* The WhereLoop factory */
        return rc;
 }
 
-/**
- * Check if is_unordered flag is set for given index.
- * Since now there may not be corresponding Tarantool's index
- * (e.g. for temporary objects such as ephemeral tables),
- * a few preliminary checks are made.
- *
- * @param idx Index to be tested.
- * @retval True, if statistics exist and unordered flag is set.
- */
-static bool
-index_is_unordered(const struct index_def *idx)
-{
-       assert(idx != NULL);
-       struct space *space = space_by_id(idx->space_id);
-       if (space == NULL)
-               return false;
-       struct index *tnt_idx = space_index(space, idx->iid);
-       if (tnt_idx == NULL)
-               return false;
-       if (tnt_idx->def->opts.stat != NULL)
-               return tnt_idx->def->opts.stat->is_unordered;
-       return false;
-}
-
 /*
  * Return True if it is possible that pIndex might be useful in
  * implementing the ORDER BY clause in pBuilder.
@@ -2729,7 +2705,7 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
        ExprList *pOB;
        int ii, jj;
        int part_count = idx_def->key_def->part_count;
-       if (index_is_unordered(idx_def))
+       if (idx_def->opts.stat != NULL && idx_def->opts.stat->is_unordered)
                return 0;
        if ((pOB = pBuilder->pWInfo->pOrderBy) == 0)
                return 0;
@@ -3310,7 +3286,8 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo,     /* The WHERE clause */
                                idx_def = NULL;
                                nColumn = 1;
                        } else if ((idx_def = pLoop->index_def) == NULL ||
-                                  index_is_unordered(idx_def)) {
+                                  (idx_def->opts.stat != NULL &&
+                                   idx_def->opts.stat->is_unordered)) {
                                return 0;
                        } else {
                                nColumn = idx_def->key_def->part_count;
 
 test:do_execsql_test(
diff --git a/test/sql-tap/lua-tables.test.lua b/test/sql-tap/lua-tables.test.lua
index 0e79c61a8..19d80b611 100755
--- a/test/sql-tap/lua-tables.test.lua
+++ b/test/sql-tap/lua-tables.test.lua
@@ -46,7 +46,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "lua-tables-5",
     [[SELECT * from "t1"]],
-    {1, "no format for space: t1"}
+    {1, "SQL does not support space without format"}
 )
 -- Extract from tkt3527.test.lua
 test:do_test(





More information about the Tarantool-patches mailing list