[tarantool-patches] Re: [PATCH v11] sql: add index_def to struct Index

["n.pettik" <korablev@tarantool.org>]

>>>> Then:
>>>> 
>>>> CREATE TABLE t11 (s1 INT, a, constraint c1 UNIQUE(s1) on conflict replace, PRIMARY KEY(s1));
>>>> 
>>>> In this case creation of unique constraint c1 is omitted, but no errors or warnings are shown.
>>>> It is not a problem now, but when ALTER TABLE DROP CONSTRAINT is implemented,
>>>> it will be possible to drop c1 constraint. Eventually, user would be disappointed if tried to drop
>>>> this constraint but got an error.
>>> It seems to be out of the scope of the patch. Appropriate ticket:
>>> https://github.com/tarantool/tarantool/issues/3498
>> I don’t understand how that ticket is related to given issue. Again, problem lies in
>> silent absorption of unique constraint by primary index.
> I am not sure what to do with this. In Postgresql these two commands work silently, no UNIQUE
> constraint is created

Really? I tried it on PostgreSQL 9.6.2 and got this:

CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT c1 UNIQUE(id));

SELECT conname FROM pg_constraint WHERE conrelid = (SELECT oid  FROM pg_class WHERE relname LIKE 't1’);

    conname
1	c1

As we can see, this unique constraint has been successfully created.

> and ALTER TABLE works silently and do nothing.

And after drop:

ALTER TABLE t1 DROP CONSTRAINT c1;

SELECT conname FROM pg_constraint WHERE conrelid = (SELECT oid  FROM pg_class WHERE relname LIKE 't1’);

I got nothing.

> In MySQL both constraints
> will be created with no warnings or errors. What do you propose to do?

Lets dive into ANSI: if there is nothing about that (i.e. explicit contradictions), then
we shouldn’t omit creation of unique constraint over PK (at least in case it is named constraint).

>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index d66777f73..d2fed97eb 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2430,13 +2430,14 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
>>  }
>>    struct Index *
>> -sql_index_alloc(struct sqlite3 *db)
>> +sql_index_alloc(struct sqlite3 *db, uint32_t part_count)
>>  {
>>         /* Size of struct Index and aiRowLogEst. */
>> -       int index_size = ROUND8(sizeof(struct Index));
>> +       int index_size = sizeof(struct Index) +
>> +                        sizeof(LogEst) * (part_count + 1);
>>         struct Index *p = sqlite3DbMallocZero(db, index_size);
>>         if (p != NULL)
>> -               p->aiRowLogEst = (LogEst *) ((char *)p + ROUND8(sizeof(*p)));
>> +               p->aiRowLogEst = (LogEst *) ((char *)p + sizeof(*p));
>>         return p;
>>  }
>> 
>> @@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct Token *token,
>>         if (sqlite3CheckIdentifierName(parse, name) != SQLITE_OK)
>>                 goto exit_create_index;
>>  -       index = sql_index_alloc(db);
>> +       index = sql_index_alloc(db, col_list->nExpr);
>> 
>> @@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct Token *token,
>>         if (sqlite3CheckIdentifierName(parse, name) != SQLITE_OK)
>>                 goto exit_create_index;
>>  -       index = sql_index_alloc(db);
>> +       index = sql_index_alloc(db, col_list->nExpr);
>>         if (index == NULL)
>>                 goto exit_create_index;
>>  -       assert(EIGHT_BYTE_ALIGNMENT(index->aiRowLogEst));
>> 
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index ae31dfae5..2082d6ca9 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -3631,7 +3631,7 @@ void sqlite3SrcListDelete(sqlite3 *, SrcList *);
>>   * @retval not NULL Index object.
>>   */
>>  struct Index *
>> -sql_index_alloc(struct sqlite3 *db);
>> +sql_index_alloc(struct sqlite3 *db, uint32_t part_count);
> Why do we need to allocate 'part_count' part of memory? It does not seem
> to be used for anything.

It is used for array aiRowLogEst. Btw, you really can remove and now use
index_def->opts.stat->tuple_log_est;  for fake indexes - then you are able to
avoid allocating this memory.

>>>>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>>>>> index 85143ed20..7ca02095f 100644
>>>>> --- a/src/box/sql/where.c
>>>>> +++ b/src/box/sql/where.c
>>>>> @@ -372,13 +372,19 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
>>>>> 	pScan->is_column_seen = false;
>>>>> 	if (pIdx) {
>>>>> 		int j = iColumn;
>>>>> -		iColumn = pIdx->aiColumn[j];
>>>>> +		iColumn = pIdx->def->key_def->parts[j].fieldno;
>>>>> +		/*
>>>>> +		 * pIdx->tnum == 0 means that pIdx is a fake
>>>>> +		 * integer primary key index.
>>>>> +		 */
>>>>> +		if (pIdx->tnum == 0)
>>>>> +			iColumn = -1;
>>>> We are going to remove tnum from struct Index and struct Table.
>>>> So, if it is possible, use index->def->iid instead (or smth else).
>>> Removed with ‘fake_autoindex'
>> Well, if smb called index ‘fake_autoindex’, I guess strange things would happen.
>> Could we use for instance def->space_id == 0 as a sign of ‘fake_index'?.
> Yes, it seems to be a better solution. What do you think about adding a field to
> index_def.opts - index_def.opts.is_fake_pk?

I am strictly against - it sounds like over-engineering. In parser, for example,
space_def->opts.is_temporary is used as a sing of space_def allocated on a region
(but initially this property is set to be true if space is temporary and temporary != allocated on region).
So, since index is in fact surrogate, lets just use one of its fields.

>> 
>> Moreover, sql_stmt is obtained from sqlite3Malloc() and assigned to index->opts,
>> which in turn is released by common free().
> In index_def_new() (where we pass opts) given sql_stmt is copied using strdup, so
> it's okay to free() it, isn't it?

Ok, then you don’t release original sql_stmt: fix it.

> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d66777f73..0cb0b8e08 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -984,8 +984,10 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
> 		sqlite3ErrorMsg(pParse, "AUTOINCREMENT is only allowed on an "
> 				"INTEGER PRIMARY KEY or INT PRIMARY KEY");
> 	} else {
> +		/* Since this index implements PK, it is unique. */
> 		sql_create_index(pParse, 0, 0, pList, onError, 0,
> -				 0, sortOrder, false, SQLITE_IDXTYPE_PRIMARYKEY);
> +				 0, sortOrder, false, SQLITE_IDXTYPE_PRIMARYKEY,
> +				 true);
> 		pList = 0;
> 	}
> @@ -1311,9 +1313,10 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
> 			return;
> 		pList->a[0].sort_order = pParse->iPkSortOrder;
> 		assert(pParse->pNewTable == pTab);
> +		/* Since this index implements PK, it is unique. */
> 		sql_create_index(pParse, 0, 0, pList, pTab->keyConf, 0, 0,
> 				 SORT_ORDER_ASC, false,
> -				 SQLITE_IDXTYPE_PRIMARYKEY);
> +				 SQLITE_IDXTYPE_PRIMARYKEY, true);
> 		if (db->mallocFailed)
> 			return;
> 		pPk = sqlite3PrimaryKeyIndex(pTab);
> @@ -2538,10 +2541,8 @@ addIndexToTable(Index * pIndex, Table * pTab)
>  * @param expr_list List of expressions, describe which columns
>  *                  of 'table' are used in index and also their
>  *                  collations, orders, etc.
> - * @param idx_type Index type, one of the following:
> - *                 SQLITE_IDXTYPE_APPDEF     0
> - *                 SQLITE_IDXTYPE_UNIQUE     1
> - *                 SQLITE_IDXTYPE_PRIMARYKEY 2.
> + * @param idx_type Index type: UNIQUE constraint, PK constraint,
> + *                 or created by <CREATE INDEX ...> stmt.
>  * @param sql_stmt SQL statement, which creates the index.
>  * @retval 0 Success.
>  * @retval -1 Error.
> @@ -2632,7 +2633,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 		 struct SrcList *tbl_name, struct ExprList *col_list,
> 		 enum on_conflict_action on_error, struct Token *start,
> 		 struct Expr *where, enum sort_order sort_order,
> -		 bool if_not_exist, u8 idx_type)
> +		 bool if_not_exist, u8 idx_type, bool is_unique)

Wait, you already have idx_type, you don’t need another one argument.
Lets simply turn defines into appropriate enum
(probably you should pick better names, but follow code style):

-/*
- * Allowed values for Index.idxType
- */
-#define SQLITE_IDXTYPE_APPDEF      0   /* Created using CREATE INDEX */
-#define SQLITE_IDXTYPE_UNIQUE      1   /* Implements a UNIQUE constraint */
-#define SQLITE_IDXTYPE_PRIMARYKEY  2   /* Is the PRIMARY KEY for the table */
+/**
+ * There are some differences between explicitly created indexes
+ * and unique table constraints (despite the fact that they are
+ * the same in implementation). For instance, to drop index user
+ * must use <DROP INDEX ...> statement, but to drop constraint -
+ * <ALTER TABLE DROP CONSTRAINT ...> syntax.
+ */
+enum sql_index_type {
+       SQL_INDEX_USER_DEFINED_UINIQUE = 0,
+       SQL_INDEX_USER_DEFINED = 1,
+       SQL_INDEX_CONSTRAINT_UNIQUE = 2,
+       SQL_INDEX_CONSTRAINT_PK = 3,
+};