[tarantool-patches] Re: [PATCH v11] sql: add index_def to struct Index
n.pettik
korablev at tarantool.org
Fri Jul 6 03:51:41 MSK 2018
>>>> 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,
+};
More information about the Tarantool-patches
mailing list