From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 043FB26424 for ; Thu, 5 Jul 2018 20:51:51 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fMeyi8et2rJv for ; Thu, 5 Jul 2018 20:51:50 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1EB8124D34 for ; Thu, 5 Jul 2018 20:51:50 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v11] sql: add index_def to struct Index From: "n.pettik" In-Reply-To: Date: Fri, 6 Jul 2018 03:51:41 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <9A0A687D-2E0B-4AB9-B223-1A1287817824@tarantool.org> <8900ae2a-59e8-d69a-7f5d-95436b23214a@tarantool.org> <0e795fb5-1ee8-e7e6-65e4-7347787f3248@tarantool.org> <898ec1ac-51b9-13a6-6298-3a20eeac5b2b@tarantool.org> <76c1ec30-4f33-bc91-8845-72a99fc4ef0f@tarantool.org> <6fbc3849-a204-6cd9-82cd-2fb22769ccf0@tarantool.org> <2d4908aa-0243-8dc3-e109-707cb482b7f6@tarantool.org> <35faf3de-c72a-e0b0-31fe-3d101c5e9754@tarantool.org> <155C98B2-2718-42F0-BC7E-B3A3FFB5C0F5@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Ivan Koptelov >>>> Then: >>>>=20 >>>> CREATE TABLE t11 (s1 INT, a, constraint c1 UNIQUE(s1) on conflict = replace, PRIMARY KEY(s1)); >>>>=20 >>>> 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=E2=80=99t 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 =3D (SELECT oid FROM = pg_class WHERE relname LIKE 't1=E2=80=99); 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 =3D (SELECT oid FROM = pg_class WHERE relname LIKE 't1=E2=80=99); 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=E2=80=99t 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 =3D ROUND8(sizeof(struct Index)); >> + int index_size =3D sizeof(struct Index) + >> + sizeof(LogEst) * (part_count + 1); >> struct Index *p =3D sqlite3DbMallocZero(db, index_size); >> if (p !=3D NULL) >> - p->aiRowLogEst =3D (LogEst *) ((char *)p + = ROUND8(sizeof(*p))); >> + p->aiRowLogEst =3D (LogEst *) ((char *)p + = sizeof(*p)); >> return p; >> } >>=20 >> @@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct = Token *token, >> if (sqlite3CheckIdentifierName(parse, name) !=3D SQLITE_OK) >> goto exit_create_index; >> - index =3D sql_index_alloc(db); >> + index =3D sql_index_alloc(db, col_list->nExpr); >>=20 >> @@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct = Token *token, >> if (sqlite3CheckIdentifierName(parse, name) !=3D SQLITE_OK) >> goto exit_create_index; >> - index =3D sql_index_alloc(db); >> + index =3D sql_index_alloc(db, col_list->nExpr); >> if (index =3D=3D NULL) >> goto exit_create_index; >> - assert(EIGHT_BYTE_ALIGNMENT(index->aiRowLogEst)); >>=20 >> 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 =3D false; >>>>> if (pIdx) { >>>>> int j =3D iColumn; >>>>> - iColumn =3D pIdx->aiColumn[j]; >>>>> + iColumn =3D pIdx->def->key_def->parts[j].fieldno; >>>>> + /* >>>>> + * pIdx->tnum =3D=3D 0 means that pIdx is a fake >>>>> + * integer primary key index. >>>>> + */ >>>>> + if (pIdx->tnum =3D=3D 0) >>>>> + iColumn =3D -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 =E2=80=98fake_autoindex' >> Well, if smb called index =E2=80=98fake_autoindex=E2=80=99, I guess = strange things would happen. >> Could we use for instance def->space_id =3D=3D 0 as a sign of = =E2=80=98fake_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 !=3D allocated on region). So, since index is in fact surrogate, lets just use one of its fields. >>=20 >> 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=E2=80=99t release original sql_stmt: fix it. >=20 > 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 =3D 0; > } > @@ -1311,9 +1313,10 @@ convertToWithoutRowidTable(Parse * pParse, = Table * pTab) > return; > pList->a[0].sort_order =3D pParse->iPkSortOrder; > assert(pParse->pNewTable =3D=3D 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 =3D 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 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=E2=80=99t 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 statement, but to drop constraint - + * syntax. + */ +enum sql_index_type { + SQL_INDEX_USER_DEFINED_UINIQUE =3D 0, + SQL_INDEX_USER_DEFINED =3D 1, + SQL_INDEX_CONSTRAINT_UNIQUE =3D 2, + SQL_INDEX_CONSTRAINT_PK =3D 3, +};