>> +ssize_t >> +sql_index_tuple_size(struct space *space, struct index *idx) >> +{ >> + assert(space != NULL); >> + assert(idx != NULL); >> + assert(idx->def->space_id == space->def->id); >> + ssize_t tuple_count = idx->vtab->size(idx); >> + ssize_t space_size = space->vtab->bsize(space); > > 1. Lets use wrappers: index_size() and space_bsize() - they are defined > already. Done: +++ b/src/box/sql/analyze.c @@ -1197,9 +1197,10 @@ sql_index_tuple_size(struct space *space, struct index *idx) assert(space != NULL); assert(idx != NULL); assert(idx->def->space_id == space->def->id); - ssize_t tuple_count = idx->vtab->size(idx); - ssize_t space_size = space->vtab->bsize(space); - ssize_t avg_tuple_size = DIV_OR_ZERO(space_size, tuple_count); + ssize_t tuple_count = index_size(idx); + ssize_t space_size = space_bsize(space); + ssize_t avg_tuple_size = tuple_count != 0 ? + (space_size / tuple_count) : 0; >> diff --git a/src/box/sql/select.c b/src/box/sql/select.c >> index 0df8a71d4..391b7e0a2 100644 >> --- a/src/box/sql/select.c >> +++ b/src/box/sql/select.c >> @@ -1588,20 +1588,19 @@ generateSortTail(Parse * pParse, /* Parsing context */ >> * the SQLITE_ENABLE_COLUMN_METADATA compile-time option is used. >> */ >> #ifdef SQLITE_ENABLE_COLUMN_METADATA >> -#define columnType(A,B,C,D,E,F) columnTypeImpl(A,B,D,E,F) >> +#define columnType(A,B,C,D,E) columnTypeImpl(A,B,D,E) >> #else /* if !defined(SQLITE_ENABLE_COLUMN_METADATA) */ >> -#define columnType(A,B,C,D,E,F) columnTypeImpl(A,B,F) >> +#define columnType(A,B,C,D,E) columnTypeImpl(A,B) >> #endif >> static enum field_type >> -columnTypeImpl(NameContext * pNC, Expr * pExpr, >> +columnTypeImpl(NameContext * pNC, Expr * pExpr >> #ifdef SQLITE_ENABLE_COLUMN_METADATA >> - const char **pzOrigTab, const char **pzOrigCol, >> + , const char **pzOrigTab, const char **pzOrigCol, > > 2. As I can see, the third argument is always NULL. Lets remove it > too. Done: @@ -1655,8 +1655,8 @@ columnTypeImpl(NameContext * pNC, Expr * pExpr sNC.pNext = pNC; sNC.pParse = pNC->pParse; column_type = - columnType(&sNC, p, 0, - &zOrigTab, &zOrigCol); + columnType(&sNC, p, &zOrigTab, + &zOrigCol); @@ -1685,7 +1685,7 @@ columnTypeImpl(NameContext * pNC, Expr * pExpr sNC.pNext = pNC; sNC.pParse = pNC->pParse; column_type = - columnType(&sNC, p, 0, &zOrigTab, &zOrigCol); + columnType(&sNC, p, &zOrigTab, &zOrigCol); @@ -1921,7 +1921,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */ for (i = 0, pCol = pTab->aCol; i < pTab->nCol; i++, pCol++) { enum field_type type; p = a[i].pExpr; - type = columnType(&sNC, p, 0, 0, 0); + type = columnType(&sNC, p, 0, 0); >> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h >> index 59662cf14..8ca8e808f 100644 >> --- a/src/box/sql/sqliteInt.h >> +++ b/src/box/sql/sqliteInt.h >> @@ -1396,6 +1396,11 @@ struct BusyHandler { >> */ >> #define IsPowerOfTwo(X) (((X)&((X)-1))==0) >> +#ifdef ZERO_OR_DIV >> +#undef ZERO_OR_DIV >> +#endif >> +#define DIV_OR_ZERO(NUM, DENOM) (((DENOM) != 0) ? ((NUM) / (DENOM)) : 0) > > 3. > > Divide by 0: *exists* > Programmers:https://pm1.narvii.com/6585/b5b717574d0d6250181c18aadd89fbe0b3c7bf3a_hq.jpg > > Lets just inline it. And what is ZERO_OR_DIV? It was just typo in naming. Anyway, I have removed this macro: +++ b/src/box/sql/sqliteInt.h @@ -1389,11 +1389,6 @@ struct BusyHandler { */ #define IsPowerOfTwo(X) (((X)&((X)-1))==0) -#ifdef ZERO_OR_DIV -#undef ZERO_OR_DIV -#endif -#define DIV_OR_ZERO(NUM, DENOM) (((DENOM) != 0) ? ((NUM) / (DENOM)) : 0) - >> diff --git a/src/box/sql/where.c b/src/box/sql/where.c >> index 2a2630281..51b53c2df 100644 >> --- a/src/box/sql/where.c >> +++ b/src/box/sql/where.c >> @@ -2545,15 +2537,31 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder, /* The WhereLoop factory */ >> * seek only. Then, if this is a non-covering index, add the cost of >> * visiting the rows in the main table. >> */ >> - rCostIdx = >> - pNew->nOut + 1 + >> - (15 * pProbe->szIdxRow) / pSrc->pTab->szTabRow; >> + struct space *space = >> + space_by_id(SQLITE_PAGENO_TO_SPACEID(pProbe->tnum)); >> + assert(space != NULL); >> + struct index *idx = >> + space_index(space, >> + SQLITE_PAGENO_TO_INDEXID(pProbe->tnum)); >> + assert(idx != NULL); >> + /* >> + * FIXME: currently, the procedure below makes no >> + * sense, since there are no partial indexes, so >> + * all indexes in the space feature the same >> + * average tuple size. > > 4. Do not forget about Vinyl. In it even with no partial indexes different > ones can contain different tuple count, tuples of different size (due to > specific of secondary indexes disk data structure). Now it does not support SQL, > but will do. Ok, updated comment (and inlined macro): --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -2544,14 +2544,17 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder, /* The WhereLoop factory */ * FIXME: currently, the procedure below makes no * sense, since there are no partial indexes, so * all indexes in the space feature the same - * average tuple size. + * average tuple size. Moreover, secondary + * indexes in Vinyl engine may contain different + * tuple count of different sizes. */ ssize_t avg_tuple_size = sql_index_tuple_size(space, idx); struct index *pk = space_index(space, 0); assert(pProbe->pTable == pSrc->pTab); ssize_t avg_tuple_size_pk = sql_index_tuple_size(space, pk); - uint32_t partial_index_cost = DIV_OR_ZERO((15 * avg_tuple_size), - avg_tuple_size_pk); + uint32_t partial_index_cost = + avg_tuple_size_pk != 0 ? + (15 * avg_tuple_size) / avg_tuple_size_pk : 0; >> diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua >> index 4ce575e90..3b3d52f67 100755 >> --- a/test/sql-tap/analyze9.test.lua >> +++ b/test/sql-tap/analyze9.test.lua >> +-- These tests are commented until query planer will be stable. > > 5. What do you mean saying 'unstable'? The test is flaky or incorrect? Both.