Please, follow our SOP convention: if you send next patch version,Sorry. Would send only diff this time.
then resend it as a separate patch (git format-patch -1 --subject-prefix='PATCH
v2’),
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.
I am not sure what to do with this. In Postgresql these two commands work silently, no UNIQUEI don’t understand how that ticket is related to given issue. Again, problemThen:It seems to be out of the scope of the patch. Appropriate ticket:
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.
https://github.com/tarantool/tarantool/issues/3498
lies in
silent absorption of unique constraint by primary index.
Why do we need to allocate 'part_count' part of memory? It does not seem
No, you don’t. Apply this diff:No. Removed.+struct Index *Do we really need this alignment?
+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 = 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 = sqlite3DbMallocZero(db, nByte + nExtra);
- if (p) {
- char *pExtra = ((char *)p) + ROUND8(sizeof(Index));
- p->coll_array = (struct coll **)pExtra;
- pExtra += ROUND8(sizeof(struct coll **) * nCol);
- p->coll_id_array = (uint32_t *) pExtra;
- pExtra += ROUND8(sizeof(uint32_t) * nCol);
- p->aiRowLogEst = (LogEst *) pExtra;
- pExtra += sizeof(LogEst) * (nCol + 1);
- p->aiColumn = (i16 *) pExtra;
- pExtra += sizeof(i16) * nCol;
- p->sort_order = (enum sort_order *) pExtra;
- p->nColumn = nCol;
- *ppExtra = ((char *)p) + nByte;
- }
+ /* Size of struct Index and aiRowLogEst. */
+ int nByte = ROUND8(sizeof(struct Index)) +
+ ROUND8(sizeof(LogEst) * (part_count + 1));
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);
Ok.
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 <CREATE INDEX ...> stmt.
Ok.You forgot about my comment:@@ -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 = sqlite3NameFromToken(db, token);
- if (zName == 0)
+ if (token != NULL) {
+ name = sqlite3NameFromToken(db, token);
+ if (name == NULL)
goto exit_create_index;
- assert(token->z != 0);
+ assert(token->z != NULL);
if (!db->init.busy) {
- if (sqlite3HashFind(&db->pSchema->tblHash, zName) !=
+ if (sqlite3HashFind(&db->pSchema->tblHash, name) !=
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;
}
}
There is no need to prohibit creating index with table name, sinceApply diff:
index name is local for given table. And most of other DBs also
allow to create index with table name.
@@ -2705,14 +2702,6 @@ sql_create_index(struct Parse *parse, struct Token
*token,
if (name == NULL)
goto exit_create_index;
assert(token->z != NULL);
- if (!db->init.busy) {
- if (sqlite3HashFind(&db->pSchema->tblHash, name) !=
- NULL) {
- sqlite3ErrorMsg(parse, "there is already a "\
- "table named %s", name);
- goto exit_create_index;
- }
- }
Ok, done. Also perform small refactoring in parser.What? Lets then change signature and from parser pass additional parameterNot sure about this. It seems that information about uniqueness is+It seems to be so messy defining uniqueness by ON_CONFLICT_ACTION.
+ bool is_unique = on_error != ON_CONFLICT_ACTION_NONE;
Lets refactor it somehow.
only in on_error parameter.
which would tell if index is unique or not.
Yes, it seems to be a better solution. What do you think about adding a field to
Well, if smb called index ‘fake_autoindex’, I guess strange things would happen.Removed with ‘fake_autoindex'diff --git a/src/box/sql/where.c b/src/box/sql/where.cWe are going to remove tnum from struct Index and struct Table.
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;
So, if it is possible, use index->def->iid instead (or smth else).
Could we use for instance def->space_id == 0 as a sign of ‘fake_index'?.
Renamed back.
src/box/errcode.h | 1 +Why have you renamed this test?
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 => 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 => collation1.test.lua} (97%)
Ok.
+ if (is_system_space && idx_type == SQLITE_IDXTYPE_APPDEF) {+ diag_set(ClientError, ER_MODIFY_INDEX, name, table->def->name,
+ diag_set(ClientError, ER_MODIFY_INDEX, name,
+ table->def->name, "creating indexes on system "
+ "spaces are prohibited”);
+ "can't create index on system space");
Sorry, fixed.+ char *sql_stmt = "";Wrong alignment:
+ if (!db->init.busy && tbl_name != NULL) {
+ int n = (int) (parse->sLastToken.z - token->z) +
+ parse->sLastToken.n;
+ if (token->z[n - 1] == ';')
+ n--;
+ sql_stmt = sqlite3MPrintf(db, "CREATE%s INDEX %.*s",
+ on_error == ON_CONFLICT_ACTION_NONE ?
+ "" : " UNIQUE", n, token->z);
@@ -2809,8 +2796,8 @@ sql_create_index(struct Parse *parse, struct Token *token,
if (token->z[n - 1] == ';')
n--;
sql_stmt = sqlite3MPrintf(db, "CREATE%s INDEX %.*s",
- on_error == ON_CONFLICT_ACTION_NONE ?
- "" : " UNIQUE", n, token->z);
+ on_error == ON_CONFLICT_ACTION_NONE ?
+ "" : " UNIQUE", n, token->z);
Moreover, sql_stmt is obtained from sqlite3Malloc() and assigned to index->opts,In index_def_new() (where we pass opts) given sql_stmt is copied using strdup, so
which in turn is released by common free().
The idea was to check that removing duplicates works fine. Fixed the test to check names of
diff --git a/test/sql-tap/index7.test.lua b/test/sql-tap/index7.test.luaWhy nil? What does this test check at all?
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 = 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)
Ok, done.
+Use indentation for nested select clause to make it more readable.
+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" = "_space"."id" AND
+ "_space"."name"='TEST4' AND
+ "_index"."name"='INDEX1’;
Moreover, add comment to the test which would explain what exactly this test
checks.