<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Thank you for the review!<br>
<blockquote type="cite"
cite="mid:513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org">
<div><br class="">
<blockquote type="cite" class="">
<pre class="">+ /*
+ * Here we handle cases like
+ * CREATE TABLE t1(a UNIQUE, PRIMARY KEY(a))
+ * In these cases (where UNIQUE constraints precede
+ * PRIMARY constraint) appropriate Index->def->cmp_def
+ * can not be set 'on the fly' for indexes implementing
+ * UNIQUE constraints as PK key_def is missing and
+ * needed for it.
+ * At this point we can set appropriate cmp_def, since
+ * all indexes (from CREATE TABLE statement) is created.
+ */
+ if (!p->def->opts.is_view) {
+ assert(p->pIndex != NULL);
+ struct Index *pk;
+ for (struct Index *idx = p->pIndex; idx != NULL;
+ idx = idx->pNext) {
+ if (idx->index_type == SQL_INDEX_TYPE_CONSTRAINT_PK)
+ pk = idx;
+ }
+ struct key_def *pk_def = pk->def->key_def;
+
+ for (struct Index *idx = p->pIndex; idx != NULL;
+ idx = idx->pNext) {
+ if (idx->index_type != SQL_INDEX_TYPE_CONSTRAINT_PK) {
+ struct index_def *def = idx->def;
+ struct key_def *cmp_def =
+ key_def_merge(def->key_def,
+ pk_def);
+ if (cmp_def == NULL) {
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ ++pParse->nErr;
+ return;
+ }
+ if (!def->opts.is_unique) {
+ def->cmp_def->unique_part_count =
+ def->cmp_def->part_count;
+ } else {
+ def->cmp_def->unique_part_count =
+ def->key_def->part_count;
+ }
+ }
+ }
+ }
</pre>
</blockquote>
<div>I don’t understand: why do you need at all this cmp_def?</div>
<div>In SQL it is extremely unlikely to be useful.</div>
</div>
</blockquote>
SQL has memtx/vinyl under it, where cmp_def is used, isn't it?<br>
As far as I understand, there is some mechanism, which creates<br>
struct index with struct index_def inside memtx (for example, for<br>
memtx_tree_create()) based on what we passed inside in<br>
createIndex() in sql/build.c<br>
Is it right? I am confused.<br>
If it is right, then we can simplify code in build by always <br>
using key_def for cmp_def ?<br>
<span style="color:#808080;font-style:italic;"></span>
<blockquote type="cite"
cite="mid:513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org">
<div>
<blockquote type="cite" class="">
<pre class=""> if (db->init.busy) {
/*
* As rebuild creates a new ExpList tree and
@@ -2398,8 +2441,7 @@
struct Index *
sql_index_alloc(struct sqlite3 *db)
{
- int index_size = ROUND8(sizeof(struct Index));
- struct Index *p = sqlite3DbMallocZero(db, index_size);
+ struct Index *p = sqlite3DbMallocZero(db, sizeof(struct Index));
return p;
}
@@ -2566,12 +2608,25 @@
if (parse->nErr > 0)
goto cleanup;
- struct key_def *pk_key_def;
- if (idx_type == SQL_INDEX_TYPE_UNIQUE ||
- idx_type == SQL_INDEX_TYPE_NON_UNIQUE)
+ struct key_def *pk_key_def = NULL;
+ if (idx_type != SQL_INDEX_TYPE_CONSTRAINT_PK &&
+ parse->pNewTable == NULL)
pk_key_def = table->pIndex->def->key_def;
- else
- pk_key_def = NULL;
+ /*
+ * In cases like CREATE TABLE t1(a UNIQUE, PRIMARY KEY(a))
+ * fill_index_def() will be firstly executed for creating
+ * index_def for UNIQUE constraint and then for creating
+ * index_def for PRIMARY KEY constraint. So when
+ * fill_index_def will be called for UNIQUE constraint
+ * there will be no PK, while PK->def->key_def is needed
+ * to create cmp_def (inside index_def_new()). </pre>
</blockquote>
<div>Again: explain pls why do you need cmp_def on client-side
(i.e. parser)?</div>
<blockquote type="cite" class="">
<pre class="">To handle
+ * this case we set here pk_key_def to key_def and later
+ * in sqlite3EndTable (when PK index is already created)
+ * we go over all indexes and set appropriate cmp_def in
+ * them.
+ */
+ if (parse->pNewTable != NULL)
+ pk_key_def = key_def;
index->def = index_def_new(space_def->id, iid, name, name_len, TREE,
&opts, key_def, pk_key_def);
@@ -2618,12 +2673,6 @@
*/
struct Table *table = NULL;
if (tbl_name != NULL) {
- /*
- * Use the two-part index name to determine the
- * database to search for the table. 'Fix' the
- * table name to this db before looking up the
- * table.
- */
assert(token != NULL && token->z != NULL);
table = sqlite3LocateTable(parse, 0, tbl_name->a[0].zName);
assert(db->mallocFailed == 0 || table == NULL);
@@ -2642,7 +2691,7 @@
}
/*
* Find the name of the index. Make sure there is not
- * already another index or table with the same name.
+ * already another index with the same name.
*
* Exception: If we are reading the names of permanent
* indices from the Tarantool schema (because some other
@@ -2660,7 +2709,7 @@
* 2) UNIQUE constraint is non-named and standard
* auto-index name will be generated.
*/
- bool is_constraint_named = false;
+ bool is_constraint_named;
if (token != NULL) {
name = sqlite3NameFromToken(db, token);
if (name == NULL)
@@ -2691,14 +2740,26 @@
}
name = sqlite3MPrintf(db, "sql_autoindex_%s_%d",
table->def->name, n);
+ is_constraint_named = false;
} else {
- name = sqlite3MPrintf(db, "sql_autoindex_%s",
- constraint_name);
+ /*
+ * This naming is temporary. Now it's not
+ * possible (since we implement UNIQUE
+ * and PK constraints with indexes and
+ * indexes can not have same names), but
+ * in future we would use names exactly
+ * as they are set by user.
+ */
+ if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE)
+ name = sqlite3MPrintf(db,
+ "unique_constraint_%s",
+ constraint_name);
+ if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK)
+ name = sqlite3MPrintf(db, "pk_constraint_%s",
+ constraint_name);
is_constraint_named = true;
}
- if (constraint_name != NULL) {
- sqlite3DbFree(db, constraint_name);
- }
+ sqlite3DbFree(db, constraint_name);
}
if (name == NULL)
@@ -2708,7 +2769,8 @@
table->def->id < BOX_SYSTEM_ID_MAX;
if (is_system_space && (idx_type == SQL_INDEX_TYPE_NON_UNIQUE ||
idx_type == SQL_INDEX_TYPE_UNIQUE)) {
- diag_set(ClientError, ER_MODIFY_INDEX, name, table->def->name,
+ diag_set(ClientError, ER_MODIFY_INDEX, name,
+ table->def->name,
"can't create index on system space");
parse->nErr++;
parse->rc = SQL_TARANTOOL_ERROR;
@@ -2745,7 +2807,7 @@
index->pTable = table;
index->onError = (u8) on_error;
- index->index_type = idx_type;
+ index->index_type = idx_type;
index->pSchema = db->pSchema;
/* Tarantool have access to each column by any index. */
if (where != NULL) {
@@ -2775,8 +2837,22 @@
goto exit_create_index;
}
- /* If it is parsing stage, then iid may have any value. */
- uint32_t iid = 1;
+ /*
+ * We set no SQL statement for indexes created during
+ * CREATE TABLE statement. Instead we use
+ * Index->index_def->opts.sql to store information if
+ * this index implements named or non-named constraint.
+ */
+ if (tbl_name == NULL &&
+ idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE)</pre>
</blockquote>
If <span class="">idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE
than </span>tbl_name must be NULL:</div>
<div>you can’t declare unique constraint outside CREATE TABLE
statement.<br class="">
</div>
</blockquote>
What about ALTER TABLE statements? We don't support ALTER TABLE ADD
UNIQUE<br>
now, but in future we want to have it, don't we?<br>
<blockquote type="cite"
cite="mid:513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org">
<div>
<div class=""><br class="">
</div>
<blockquote type="cite" class="">
<pre class="">+ sql_stmt = is_constraint_named ?
+ "named constraint" : "non-named constraint”;
</pre>
</blockquote>
<div>You can set ‘sql_stmt’ only for name constraints:</div>
<div><br class="">
</div>
<div>sql_stmt = is_constraint_name ? “_named_constr” : “”;</div>
<div><br class="">
</div>
<div>And use strncmp when comparing with it. </div>
<div><br class="">
</div>
<div>Also, if <if-clause> contains mode than one row,</div>
<div>we wrap it in brackets.</div>
</div>
</blockquote>
Fixed this.<br>
<blockquote type="cite"
cite="mid:513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org">
<div>
<blockquote type="cite" class="">
<pre class="">+
+ /*
+ * If it is parsing stage, then iid may have any value,
+ * but PK still must have iid == 0
+ */
+ uint32_t iid = idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK ? 0 : 1;
if (db->init.busy)
iid = SQLITE_PAGENO_TO_INDEXID(db->init.newTnum);
</pre>
</blockquote>
You can just do this:</div>
<div><span class=""><br class="">
</span></div>
<div><span class="">uint32_t iid = idx_type ==
SQL_INDEX_TYPE_CONSTRAINT_PK;</span></div>
</blockquote>
Ok. But =! instead of ==<br>
<blockquote type="cite"
cite="mid:513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org">
<div>
<blockquote type="cite" class="">
<pre class="">+
+ if (k != key_def->part_count)
+ continue;
+
+ if (index->onError != existing_idx->onError) {
+ if (index->onError !=
+ ON_CONFLICT_ACTION_DEFAULT &&
+ existing_idx->onError !=
+ ON_CONFLICT_ACTION_DEFAULT)
+ sqlite3ErrorMsg(parse,
+ "conflicting "\
+ "ON CONFLICT "\
+ "clauses specified”);
</pre>
</blockquote>
<div>Again: here you have already set error. What is the reason
to</div>
<div>continue process smth? You are able to go to
exit_create_index.</div>
</div>
</blockquote>
Sorry, didn't understand you first time. Added goto.<br>
<br>
</body>
</html>