[tarantool-patches] Re: [PATCH 3/4] sql: refactor usages of table's tuple count

[n.pettik]

>>  +#ifdef DEFAULT_TUPLE_COUNT
>> +#undef DEFAULT_TUPLE_COUNT
> 
> 1. Why do you need these guards? sqliteInt.h already has #ifndef SQLITEINT_H
> #define SQLITEINT_H guard. If it does not help, then it is error and must be
> fixed with no ifdef + undef for each definition.

Just for case. Since you don’t like this way, I have removed them:

@@ -2206,15 +2206,9 @@ struct IndexSample {
        tRowcnt *anDLt;         /* Est. number of distinct keys less than this sample */
 };
 
-#ifdef DEFAULT_TUPLE_COUNT
-#undef DEFAULT_TUPLE_COUNT
-#endif
 #define DEFAULT_TUPLE_COUNT 1048576
-
-#ifdef DEFAULT_TUPLE_LOG_COUNT
-#undef DEFAULT_TUPLE_LOG_COUNT
-#endif
-#define DEFAULT_TUPLE_LOG_COUNT sqlite3LogEst(DEFAULT_TUPLE_COUNT)
+/** [10*log_{2}(1048576)] == 200 */
+#define DEFAULT_TUPLE_LOG_COUNT 200

> 
>> +#endif
>> +#define DEFAULT_TUPLE_COUNT 1048576
>> +
>> +#ifdef DEFAULT_TUPLE_LOG_COUNT
>> +#undef DEFAULT_TUPLE_LOG_COUNT
>> +#endif
>> +#define DEFAULT_TUPLE_LOG_COUNT sqlite3LogEst(DEFAULT_TUPLE_COUNT)
> 
> 2. In the original code it was 200 to avoid calculation of
> sqlite3LogEst(1048576) on each temporary table creation. Lets do the same.

I did it just to easily adjusting tuple count without carrying about logarithms.
M’kay, added two independent macroses and appropriate assertion on each usage
(so release build will not be affected by calling sqlite3LogEst()): 

+++ b/src/box/sql/analyze.c
@@ -1636,6 +1636,7 @@ sql_space_tuple_log_count(struct Table *tab)
        if (space == NULL)
                return tab->tuple_log_count;
        struct index *pk = space_index(space, 0);
+       assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == DEFAULT_TUPLE_LOG_COUNT);
        /* If space represents VIEW, return default number. */
        if (pk == NULL)
                return DEFAULT_TUPLE_LOG_COUNT;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index db3fb3948..c02322b40 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1966,6 +1966,7 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
        pTab->nTabRef = 1;
        pTab->zName = 0;
        pTab->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
+       assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == DEFAULT_TUPLE_LOG_COUNT);
        sqlite3ColumnsFromExprList(pParse, pSelect->pEList, &pTab->nCol,
                                   &pTab->aCol);
        sqlite3SelectAddColumnTypeAndCollation(pParse, pTab, pSelect);
@@ -4495,7 +4496,8 @@ withExpand(Walker * pWalker, struct SrcList_item *pFrom)
                pTab->zName = sqlite3DbStrDup(db, pCte->zName);
                pTab->iPKey = -1;
                pTab->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
-               assert(200 == sqlite3LogEst(1048576));
+               assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) ==
+                      DEFAULT_TUPLE_LOG_COUNT);
                pTab->tabFlags |= TF_Ephemeral;
                pFrom->pSelect = sqlite3SelectDup(db, pCte->pSelect, 0);
                if (db->mallocFailed)
@@ -4688,6 +4690,8 @@ selectExpander(Walker * pWalker, Select * p)
                                                   &pTab->nCol, &pTab->aCol);
                        pTab->iPKey = -1;
                        pTab->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
+                       assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) ==
+                              DEFAULT_TUPLE_LOG_COUNT);
                        pTab->tabFlags |= TF_Ephemeral;

> 
>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index 51b53c2df..e6f9ef431 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -1326,8 +1326,22 @@ whereRangeScanEst(Parse * pParse,	/* Parsing & code generating context */
>>  			}
>>  			/* Determine iLower and iUpper using ($P) only. */
>>  			if (nEq == 0) {
>> +				/*
>> +				 * In this simple case, there are no any
>> +				 * equality constraints, so initially all rows
>> +				 * are in range.
>> +				 */
>>  				iLower = 0;
>> -				iUpper = p->nRowEst0;
>> +				uint32_t space_id =
>> +					SQLITE_PAGENO_TO_SPACEID(p->tnum);
>> +				struct space *space = space_by_id(space_id);
>> +				assert(space != NULL);
>> +				uint32_t iid =
>> +					SQLITE_PAGENO_TO_INDEXID(p->tnum);
>> +				struct index *idx =
>> +					space_index(space, iid);
>> +				assert(idx != NULL);
>> +				iUpper = idx->vtab->size(idx);
> 
> 3. Lets use index_count.

I guess you mean index_size()?

-                               iUpper = idx->vtab->size(idx);
+                               iUpper = index_size(idx);

> And looks like this case is repeated not the first and
> even not the second time. Lets implement a function sql_table_size(), that takes
> a table pointer, and does PAGENO_TO... + space_by_id + space_index + index_size.
> I would put it in one of the previous patches, where this case appears first.

Well, I grepped and found index_size() only in two places across SQL module:
the first is here, and the second one in sql_index_tuple_size(), which already takes space/index arguments.