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 DC64C2745E for ; Fri, 13 Jul 2018 15:19:15 -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 ZfPJo_BjoPUN for ; Fri, 13 Jul 2018 15:19:15 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 2513B27450 for ; Fri, 13 Jul 2018 15:19:15 -0400 (EDT) From: "n.pettik" Message-Id: <513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_444B179B-E46F-440C-9811-F2CB3AFB3A86" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2] sql: add index_def to struct Index Date: Fri, 13 Jul 2018 22:19:08 +0300 In-Reply-To: <0566bdde-5972-6c5e-707d-0ad58b6b45da@tarantool.org> References: <35c7947b-cb54-bac8-fed2-680c65f76cb6@tarantool.org> <0566bdde-5972-6c5e-707d-0ad58b6b45da@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 --Apple-Mail=_444B179B-E46F-440C-9811-F2CB3AFB3A86 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > + /* > + * 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 !=3D NULL); > + struct Index *pk; > + for (struct Index *idx =3D p->pIndex; idx !=3D NULL; > + idx =3D idx->pNext) { > + if (idx->index_type =3D=3D = SQL_INDEX_TYPE_CONSTRAINT_PK) > + pk =3D idx; > + } > + struct key_def *pk_def =3D pk->def->key_def; > + > + for (struct Index *idx =3D p->pIndex; idx !=3D NULL; > + idx =3D idx->pNext) { > + if (idx->index_type !=3D = SQL_INDEX_TYPE_CONSTRAINT_PK) { > + struct index_def *def =3D idx->def; > + struct key_def *cmp_def =3D > + key_def_merge(def->key_def, > + pk_def); > + if (cmp_def =3D=3D NULL) { > + pParse->rc =3D = SQL_TARANTOOL_ERROR; > + ++pParse->nErr; > + return; > + } > + if (!def->opts.is_unique) { > + def->cmp_def->unique_part_count = =3D > + = def->cmp_def->part_count; > + } else { > + def->cmp_def->unique_part_count = =3D > + = def->key_def->part_count; > + } > + } > + } > + } I don=E2=80=99t understand: why do you need at all this cmp_def? In SQL it is extremely unlikely to be useful. > 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 =3D ROUND8(sizeof(struct Index)); > - struct Index *p =3D sqlite3DbMallocZero(db, index_size); > + struct Index *p =3D sqlite3DbMallocZero(db, sizeof(struct = Index)); > return p; > } > =20 > @@ -2566,12 +2608,25 @@ > if (parse->nErr > 0) > goto cleanup; > =20 > - struct key_def *pk_key_def; > - if (idx_type =3D=3D SQL_INDEX_TYPE_UNIQUE || > - idx_type =3D=3D SQL_INDEX_TYPE_NON_UNIQUE) > + struct key_def *pk_key_def =3D NULL; > + if (idx_type !=3D SQL_INDEX_TYPE_CONSTRAINT_PK && > + parse->pNewTable =3D=3D NULL) > pk_key_def =3D table->pIndex->def->key_def; > - else > - pk_key_def =3D 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()).=20 Again: explain pls why do you need cmp_def on client-side (i.e. parser)? > 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 !=3D NULL) > + pk_key_def =3D key_def; > =20 > index->def =3D index_def_new(space_def->id, iid, name, name_len, = TREE, > &opts, key_def, pk_key_def); > @@ -2618,12 +2673,6 @@ > */ > struct Table *table =3D NULL; > if (tbl_name !=3D 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 !=3D NULL && token->z !=3D NULL); > table =3D sqlite3LocateTable(parse, 0, = tbl_name->a[0].zName); > assert(db->mallocFailed =3D=3D 0 || table =3D=3D 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 =3D false; > + bool is_constraint_named; > if (token !=3D NULL) { > name =3D sqlite3NameFromToken(db, token); > if (name =3D=3D NULL) > @@ -2691,14 +2740,26 @@ > } > name =3D sqlite3MPrintf(db, = "sql_autoindex_%s_%d", > table->def->name, n); > + is_constraint_named =3D false; > } else { > - name =3D 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 =3D=3D = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE) > + name =3D sqlite3MPrintf(db, > + = "unique_constraint_%s", > + constraint_name); > + if (idx_type =3D=3D = SQL_INDEX_TYPE_CONSTRAINT_PK) > + name =3D sqlite3MPrintf(db, = "pk_constraint_%s", > + constraint_name); > is_constraint_named =3D true; > } > - if (constraint_name !=3D NULL) { > - sqlite3DbFree(db, constraint_name); > - } > + sqlite3DbFree(db, constraint_name); > } > =20 > if (name =3D=3D NULL) > @@ -2708,7 +2769,8 @@ > table->def->id < BOX_SYSTEM_ID_MAX; > if (is_system_space && (idx_type =3D=3D = SQL_INDEX_TYPE_NON_UNIQUE || > idx_type =3D=3D 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 =3D SQL_TARANTOOL_ERROR; > @@ -2745,7 +2807,7 @@ > =20 > index->pTable =3D table; > index->onError =3D (u8) on_error; > - index->index_type =3D idx_type; > + index->index_type =3D idx_type; > index->pSchema =3D db->pSchema; > /* Tarantool have access to each column by any index. */ > if (where !=3D NULL) { > @@ -2775,8 +2837,22 @@ > goto exit_create_index; > } > =20 > - /* If it is parsing stage, then iid may have any value. */ > - uint32_t iid =3D 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 =3D=3D NULL && > + idx_type =3D=3D SQL_INDEX_TYPE_CONSTRAINT_UNIQUE) If idx_type =3D=3D SQL_INDEX_TYPE_CONSTRAINT_UNIQUE than tbl_name must = be NULL: you can=E2=80=99t declare unique constraint outside CREATE TABLE = statement. > + sql_stmt =3D is_constraint_named ? > + "named constraint" : "non-named = constraint=E2=80=9D; You can set =E2=80=98sql_stmt=E2=80=99 only for name constraints: sql_stmt =3D is_constraint_name ? =E2=80=9C_named_constr=E2=80=9D : = =E2=80=9C=E2=80=9D; And use strncmp when comparing with it.=20 Also, if contains mode than one row, we wrap it in brackets. > + > + /* > + * If it is parsing stage, then iid may have any value, > + * but PK still must have iid =3D=3D 0 > + */ > + uint32_t iid =3D idx_type =3D=3D SQL_INDEX_TYPE_CONSTRAINT_PK ? = 0 : 1; > if (db->init.busy) > iid =3D SQLITE_PAGENO_TO_INDEXID(db->init.newTnum); You can just do this: uint32_t iid =3D idx_type =3D=3D SQL_INDEX_TYPE_CONSTRAINT_PK; > + > + if (k !=3D key_def->part_count) > + continue; > + > + if (index->onError !=3D existing_idx->onError) { > + if (index->onError !=3D > + ON_CONFLICT_ACTION_DEFAULT && > + existing_idx->onError !=3D > + ON_CONFLICT_ACTION_DEFAULT) > + sqlite3ErrorMsg(parse, > + "conflicting "\ > + "ON CONFLICT "\ > + "clauses = specified=E2=80=9D); Again: here you have already set error. What is the reason to continue process smth? You are able to go to exit_create_index.= --Apple-Mail=_444B179B-E46F-440C-9811-F2CB3AFB3A86 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 

+	/*
+	 * 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 !=3D NULL);
+		struct Index *pk;
+		for (struct Index *idx =3D p->pIndex; idx !=3D NULL;
+		     idx =3D idx->pNext) {
+			if (idx->index_type =3D=3D =
SQL_INDEX_TYPE_CONSTRAINT_PK)
+				pk =3D idx;
+		}
+		struct key_def *pk_def =3D pk->def->key_def;
+
+		for (struct Index *idx =3D p->pIndex; idx !=3D NULL;
+		     idx =3D idx->pNext) {
+			if (idx->index_type !=3D =
SQL_INDEX_TYPE_CONSTRAINT_PK) {
+				struct index_def *def =3D idx->def;
+				struct key_def *cmp_def =3D
+					key_def_merge(def->key_def,
+						      pk_def);
+				if (cmp_def =3D=3D NULL) {
+					pParse->rc =3D =
SQL_TARANTOOL_ERROR;
+					++pParse->nErr;
+					return;
+				}
+				if (!def->opts.is_unique) {
+					=
def->cmp_def->unique_part_count =3D
+						=
def->cmp_def->part_count;
+				} else {
+					=
def->cmp_def->unique_part_count =3D
+						=
def->key_def->part_count;
+				}
+			}
+		}
+	}
I don=E2=80=99t understand: why do you need at = all this cmp_def?
In SQL it is extremely unlikely to be = useful.
 	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 =3D ROUND8(sizeof(struct Index));
-	struct Index *p =3D sqlite3DbMallocZero(db, index_size);
+	struct Index *p =3D sqlite3DbMallocZero(db, sizeof(struct =
Index));
 	return p;
 }
=20
@@ -2566,12 +2608,25 @@
 	if (parse->nErr > 0)
 		goto cleanup;
=20
-	struct key_def *pk_key_def;
-	if (idx_type =3D=3D SQL_INDEX_TYPE_UNIQUE ||
-	    idx_type =3D=3D SQL_INDEX_TYPE_NON_UNIQUE)
+	struct key_def *pk_key_def =3D NULL;
+	if (idx_type !=3D SQL_INDEX_TYPE_CONSTRAINT_PK &&
+	    parse->pNewTable =3D=3D NULL)
 		pk_key_def =3D table->pIndex->def->key_def;
-	else
-		pk_key_def =3D 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()). =
Again: explain pls why do you need cmp_def on = client-side (i.e. parser)?
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 !=3D NULL)
+		pk_key_def =3D key_def;
=20
 	index->def =3D index_def_new(space_def->id, iid, name, =
name_len, TREE,
 				   &opts, key_def, pk_key_def);
@@ -2618,12 +2673,6 @@
 	 */
 	struct Table *table =3D NULL;
 	if (tbl_name !=3D 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 !=3D NULL && token->z !=3D =
NULL);
 		table =3D sqlite3LocateTable(parse, 0, =
tbl_name->a[0].zName);
 		assert(db->mallocFailed =3D=3D 0 || table =3D=3D =
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 =3D false;
+	bool is_constraint_named;
 	if (token !=3D NULL) {
 		name =3D sqlite3NameFromToken(db, token);
 		if (name =3D=3D NULL)
@@ -2691,14 +2740,26 @@
 			}
 			name =3D sqlite3MPrintf(db, =
"sql_autoindex_%s_%d",
 					      table->def->name, =
n);
+			is_constraint_named =3D false;
 		} else {
-			name =3D 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 =3D=3D =
SQL_INDEX_TYPE_CONSTRAINT_UNIQUE)
+				name =3D sqlite3MPrintf(db,
+						      =
"unique_constraint_%s",
+						      constraint_name);
+			if (idx_type =3D=3D =
SQL_INDEX_TYPE_CONSTRAINT_PK)
+				name =3D sqlite3MPrintf(db, =
"pk_constraint_%s",
+						      constraint_name);
 			is_constraint_named =3D true;
 		}
-		if (constraint_name !=3D NULL) {
-			sqlite3DbFree(db, constraint_name);
-		}
+		sqlite3DbFree(db, constraint_name);
 	}
=20
 	if (name =3D=3D NULL)
@@ -2708,7 +2769,8 @@
 			       table->def->id < =
BOX_SYSTEM_ID_MAX;
 	if (is_system_space && (idx_type =3D=3D =
SQL_INDEX_TYPE_NON_UNIQUE ||
 				idx_type =3D=3D 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 =3D SQL_TARANTOOL_ERROR;
@@ -2745,7 +2807,7 @@
=20
 	index->pTable =3D table;
 	index->onError =3D (u8) on_error;
- 	index->index_type =3D idx_type;
+	index->index_type =3D idx_type;
 	index->pSchema =3D db->pSchema;
 	/* Tarantool have access to each column by any index. */
 	if (where !=3D NULL) {
@@ -2775,8 +2837,22 @@
 			goto exit_create_index;
 	}
=20
-	/* If it is parsing stage, then iid may have any value. */
-	uint32_t iid =3D 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 =3D=3D NULL &&
+	    idx_type =3D=3D =
SQL_INDEX_TYPE_CONSTRAINT_UNIQUE)
If idx_type =3D=3D= SQL_INDEX_TYPE_CONSTRAINT_UNIQUE than tbl_name must be = NULL:
you can=E2=80=99t declare unique constraint outside = CREATE TABLE statement.

+		sql_stmt =3D is_constraint_named ?
+			   "named constraint" : "non-named =
constraint=E2=80=9D;
You can set =E2=80=98sql_stmt=E2=80=99 only for = name constraints:

sql_stmt =3D = is_constraint_name ? =E2=80=9C_named_constr=E2=80=9D : =  =E2=80=9C=E2=80=9D;

And use = strncmp when comparing with it. 

Also, if <if-clause> contains mode than one = row,
we wrap it in brackets.
+
+	/*
+	 * If it is parsing stage, then iid may have any value,
+	 * but PK still must have iid =3D=3D 0
+	 */
+	uint32_t iid =3D idx_type =3D=3D SQL_INDEX_TYPE_CONSTRAINT_PK ? =
0 : 1;
 	if (db->init.busy)
 		iid =3D SQLITE_PAGENO_TO_INDEXID(db->init.newTnum);
You can just do this:

uint32_t iid =3D idx_type =3D=3D = SQL_INDEX_TYPE_CONSTRAINT_PK;
+
+			if (k !=3D key_def->part_count)
+				continue;
+
+			if (index->onError !=3D =
existing_idx->onError) {
+				if (index->onError !=3D
+				    ON_CONFLICT_ACTION_DEFAULT =
&&
+				    existing_idx->onError !=3D
+				    ON_CONFLICT_ACTION_DEFAULT)
+					sqlite3ErrorMsg(parse,
+							"conflicting "\
+							"ON CONFLICT "\
+							"clauses =
specified=E2=80=9D);
Again: here you have already set error. What is = the reason to
continue process smth? You are able to go to = exit_create_index.
= --Apple-Mail=_444B179B-E46F-440C-9811-F2CB3AFB3A86--