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 43F0E26803 for ; Wed, 4 Jul 2018 15:28:44 -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 gppeA5qk5iA0 for ; Wed, 4 Jul 2018 15:28:44 -0400 (EDT) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 5C396267E4 for ; Wed, 4 Jul 2018 15:28:43 -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: <35faf3de-c72a-e0b0-31fe-3d101c5e9754@tarantool.org> Date: Wed, 4 Jul 2018 22:28:40 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <155C98B2-2718-42F0-BC7E-B3A3FFB5C0F5@tarantool.org> 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> 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 Please, follow our SOP convention: if you send next patch version, then resend it as a separate patch (git format-patch -1 = --subject-prefix=3D'PATCH v2=E2=80=99),=20 and add changelog. If you simply want to send diff, then add it to each = pace of provided changes. In your case, it is better to send only diff AFTER = changes (not the whole patch), it makes review process MUCH easier. >> 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. >>=20 >>> +struct Index * >>> +sql_index_alloc(struct sqlite3 *db, uint32_t part_count) >>> { >>> - Index *p; /* Allocated index object */ >>> - int nByte; /* Bytes of space for Index object + = arrays */ >>> - >>> - nByte =3D ROUND8(sizeof(Index)) + /* Index = structure */ >>> - ROUND8(sizeof(struct coll *) * nCol) + /* Index.coll_array = */ >>> - ROUND8(sizeof(uint32_t) * nCol) + /* = Index.coll_id_array*/ >>> - ROUND8(sizeof(LogEst) * (nCol + 1) + /* Index.aiRowLogEst = */ >>> - sizeof(i16) * nCol + /* Index.aiColumn = */ >>> - sizeof(enum sort_order) * nCol); /* Index.sort_order = */ >>> - p =3D sqlite3DbMallocZero(db, nByte + nExtra); >>> - if (p) { >>> - char *pExtra =3D ((char *)p) + ROUND8(sizeof(Index)); >>> - p->coll_array =3D (struct coll **)pExtra; >>> - pExtra +=3D ROUND8(sizeof(struct coll **) * nCol); >>> - p->coll_id_array =3D (uint32_t *) pExtra; >>> - pExtra +=3D ROUND8(sizeof(uint32_t) * nCol); >>> - p->aiRowLogEst =3D (LogEst *) pExtra; >>> - pExtra +=3D sizeof(LogEst) * (nCol + 1); >>> - p->aiColumn =3D (i16 *) pExtra; >>> - pExtra +=3D sizeof(i16) * nCol; >>> - p->sort_order =3D (enum sort_order *) pExtra; >>> - p->nColumn =3D nCol; >>> - *ppExtra =3D ((char *)p) + nByte; >>> - } >>> + /* Size of struct Index and aiRowLogEst. */ >>> + int nByte =3D ROUND8(sizeof(struct Index)) + >>> + ROUND8(sizeof(LogEst) * (part_count + 1)); >> Do we really need this alignment? > No. Removed. No, you don=E2=80=99t. Apply this diff: 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) } =20 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; } @@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct = Token *token, if (sqlite3CheckIdentifierName(parse, name) !=3D SQLITE_OK) goto exit_create_index; =20 - index =3D sql_index_alloc(db); + index =3D sql_index_alloc(db, col_list->nExpr); @@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct = Token *token, if (sqlite3CheckIdentifierName(parse, name) !=3D SQLITE_OK) goto exit_create_index; =20 - index =3D sql_index_alloc(db); + index =3D sql_index_alloc(db, col_list->nExpr); if (index =3D=3D NULL) goto exit_create_index; =20 - 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); > Ok, removed detailed description, left a short one. @@ -2538,19 +2539,15 @@ 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. >>=20 >>> @@ -2731,42 +2765,38 @@ sql_create_index(struct Parse *parse, struct = Token *token, >>> * primary key or UNIQUE constraint. We have to invent >>> * our own name. >>> */ >>> - if (token) { >>> - zName =3D sqlite3NameFromToken(db, token); >>> - if (zName =3D=3D 0) >>> + if (token !=3D NULL) { >>> + name =3D sqlite3NameFromToken(db, token); >>> + if (name =3D=3D NULL) >>> goto exit_create_index; >>> - assert(token->z !=3D 0); >>> + assert(token->z !=3D NULL); >>> if (!db->init.busy) { >>> - if (sqlite3HashFind(&db->pSchema->tblHash, = zName) !=3D >>> + if (sqlite3HashFind(&db->pSchema->tblHash, name) = !=3D >>> NULL) { >>> - sqlite3ErrorMsg(parse, >>> - "there is already a = table named %s", >>> - zName); >>> + sqlite3ErrorMsg(parse, "there is already = a "\ >>> + "table named %s", name); >>> goto exit_create_index; >>> } >>> } You forgot about my comment: >There is no need to prohibit creating index with table name, since >index name is local for given table. And most of other DBs also >allow to create index with table name. Apply diff: @@ -2705,14 +2702,6 @@ sql_create_index(struct Parse *parse, struct = Token *token, if (name =3D=3D NULL) goto exit_create_index; assert(token->z !=3D NULL); - if (!db->init.busy) { - if (sqlite3HashFind(&db->pSchema->tblHash, name) = !=3D - NULL) { - sqlite3ErrorMsg(parse, "there is already = a "\ - "table named %s", name); - goto exit_create_index; - } - } >>> + >>> + bool is_unique =3D on_error !=3D ON_CONFLICT_ACTION_NONE; >> It seems to be so messy defining uniqueness by ON_CONFLICT_ACTION. >> Lets refactor it somehow. > Not sure about this. It seems that information about uniqueness is > only in on_error parameter. What? Lets then change signature and from parser pass additional = parameter which would tell if index is unique or not. >>> 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'?. > src/box/errcode.h | 1 + > src/box/sql.c | 54 +- > src/box/sql/analyze.c | 85 +-- > src/box/sql/build.c | 816 = ++++++++++----------- > src/box/sql/delete.c | 10 +- > src/box/sql/expr.c | 61 +- > src/box/sql/fkey.c | 216 +++--- > src/box/sql/insert.c | 145 ++-- > src/box/sql/pragma.c | 30 +- > src/box/sql/select.c | 2 +- > src/box/sql/sqliteInt.h | 116 +-- > src/box/sql/update.c | 39 +- > src/box/sql/vdbeaux.c | 2 +- > src/box/sql/vdbemem.c | 21 +- > src/box/sql/where.c | 192 ++--- > src/box/sql/wherecode.c | 102 +-- > test/box/misc.result | 1 + > test/sql-tap/analyze6.test.lua | 6 +- > .../{collation.test.lua =3D> collation1.test.lua} | 7 +- > test/sql-tap/colname.test.lua | 4 +- > test/sql-tap/gh-2931-savepoints.test.lua | 2 +- > test/sql-tap/gh2140-trans.test.lua | 2 +- > test/sql-tap/gh2259-in-stmt-trans.test.lua | 8 +- > test/sql-tap/gh2964-abort.test.lua | 2 +- > test/sql-tap/identifier-characters.test.lua | 2 +- > test/sql-tap/identifier_case.test.lua | 4 +- > test/sql-tap/index1.test.lua | 14 +- > test/sql-tap/index7.test.lua | 21 +- > test/sql-tap/intpkey.test.lua | 4 +- > test/sql-tap/misc1.test.lua | 2 +- > test/sql-tap/unique.test.lua | 8 +- > test/sql-tap/update.test.lua | 6 +- > test/sql/insert-unique.result | 3 +- > test/sql/iproto.result | 2 +- > test/sql/message-func-indexes.result | 8 +- > test/sql/on-conflict.result | 2 +- > test/sql/persistency.result | 6 +- > test/sql/transition.result | 6 +- > 38 files changed, 965 insertions(+), 1047 deletions(-) > rename test/sql-tap/{collation.test.lua =3D> collation1.test.lua} = (97%) Why have you renamed this test? > + if (is_system_space && idx_type =3D=3D SQLITE_IDXTYPE_APPDEF) { > + diag_set(ClientError, ER_MODIFY_INDEX, name, > + table->def->name, "creating indexes on system " > + "spaces are prohibited=E2=80=9D= ); + diag_set(ClientError, ER_MODIFY_INDEX, name, = table->def->name, + "can't create index on system space"); > + char *sql_stmt =3D ""; > + if (!db->init.busy && tbl_name !=3D NULL) { > + int n =3D (int) (parse->sLastToken.z - token->z) + > + parse->sLastToken.n; > + if (token->z[n - 1] =3D=3D ';') > + n--; > + sql_stmt =3D sqlite3MPrintf(db, "CREATE%s INDEX %.*s", > + on_error =3D=3D = ON_CONFLICT_ACTION_NONE ? > + "" : " UNIQUE", n, token->z); Wrong alignment: @@ -2809,8 +2796,8 @@ sql_create_index(struct Parse *parse, struct Token = *token, if (token->z[n - 1] =3D=3D ';') n--; sql_stmt =3D sqlite3MPrintf(db, "CREATE%s INDEX %.*s", - on_error =3D=3D = ON_CONFLICT_ACTION_NONE ? - "" : " UNIQUE", n, token->z); + on_error =3D=3D = ON_CONFLICT_ACTION_NONE ? + "" : " UNIQUE", n, token->z); Moreover, sql_stmt is obtained from sqlite3Malloc() and assigned to = index->opts, which in turn is released by common free(). > diff --git a/test/sql-tap/index7.test.lua = b/test/sql-tap/index7.test.lua > index 336f42796..4bd01b8b3 100755 > --- a/test/sql-tap/index7.test.lua > +++ b/test/sql-tap/index7.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(5) > +test:plan(7) > --!./tcltestrunner.lua > -- 2013-11-04 > @@ -48,7 +48,7 @@ end > -- do_test index7-1.1a { > -- capture_pragma db out {PRAGMA index_list(t1)} > -- db eval {SELECT "name", "partial", '|' FROM out ORDER BY "name"} > --- } {sqlite_autoindex_t1_1 0 | t1a 1 | t1b 1 |} > +-- } {sql_autoindex_t1_1 0 | t1a 1 | t1b 1 |} > -- # Make sure the count(*) optimization works correctly with > -- # partial indices. Ticket [a5c8ed66cae16243be6] 2013-10-03. > -- # > @@ -303,4 +303,21 @@ test:do_catchsql_test( > 1, "keyword \"WHERE\" is reserved" > }) > +test:do_catchsql_test( > + "index7-6.6", > + 'CREATE TABLE test2 (a int, b int, c int, PRIMARY KEY (a, a, = a, b, b, a, c))', > + nil) Why nil? What does this test check at all? > + > +test:do_catchsql_test( > + "index7-6.7", > + [[ > + CREATE TABLE test4(a,b,c,d, PRIMARY KEY(a,a,a,b,c)); > + CREATE INDEX index1 on test4(b,c,a,c); > + SELECT "_index"."name" FROM "_index" JOIN "_space" WHERE > + "_index"."id" =3D "_space"."id" AND > + "_space"."name"=3D'TEST4' AND > + "_index"."name"=3D'INDEX1=E2=80=99; Use indentation for nested select clause to make it more readable. Moreover, add comment to the test which would explain what exactly this = test checks.