[tarantool-patches] Re: [PATCH] sql: refactor primary index creation

[n.pettik]

Now you also can remove keyConf field from struct Table.

> @@ -931,6 +925,22 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
> 		}
> 		if (pList)
> 			pParse->iPkSortOrder = pList->a[0].sort_order;
> +
> +		struct sqlite3 *db = pParse->db;
> +		struct ExprList *list;
> +		struct Token ipkToken;
> +		sqlite3TokenInit(&ipkToken, pTab->def->fields[iCol].name);
> +		list = sql_expr_list_append(db, NULL,
> +					    sqlite3ExprAlloc(db, TK_ID,
> +							     &ipkToken, 0));
> +		if (list == NULL)
> +			return;

You don’t need to create list from scratch: sql_create_index accepts
NULL as list of columns and processes it exactly in the same way
(i.e. uses only last column).

> +		list->a[0].sort_order = pParse->iPkSortOrder;
> +		sql_create_index(pParse, 0, 0, list, pTab->keyConf, 0, 0,
> +				 SORT_ORDER_ASC, false,
> +				 SQL_INDEX_TYPE_CONSTRAINT_PK);
> +		if (db->mallocFailed)
> +			return;

goto primary_key_exit?

> 	} else if (autoInc) {
> 		sqlite3ErrorMsg(pParse, "AUTOINCREMENT is only allowed on an "
> 				"INTEGER PRIMARY KEY or INT PRIMARY KEY");
> @@ -941,7 +951,16 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
> 		pList = 0;
> 	}
> 
> - primary_key_exit:
> + 	/* Mark every PRIMARY KEY column as NOT NULL (except for imposter tables)
> +	 */

Fix comment: we don’t have imposter tables; fit it into 66 chars.

> +	for (uint32_t i = 0; i < pTab->def->field_count; i++) {
> +		if (pTab->aCol[i].is_primkey) {
> +			pTab->def->fields[i].nullable_action
> +				= ON_CONFLICT_ACTION_ABORT;
> +			pTab->def->fields[i].is_nullable = false;
> +		}
> +	}
> +primary_key_exit:
> 	sql_expr_list_delete(pParse->db, pList);
> 	return;
> }
> @@ -1215,63 +1234,6 @@ createTableStmt(sqlite3 * db, Table * p)
> 	return zStmt;
> }
> 
> -/*
> - * This routine runs at the end of parsing a CREATE TABLE statement.
> - * The job of this routine is to convert both
> - * internal schema data structures and the generated VDBE code.
> - * Changes include:
> - *
> - *     (1)  Set all columns of the PRIMARY KEY schema object to be NOT NULL.
> - *     (2)  Set the Index.tnum of the PRIMARY KEY Index object in the
> - *          schema to the rootpage from the main table.
> - *     (3)  Add all table columns to the PRIMARY KEY Index object
> - *          so that the PRIMARY KEY is a covering index.
> - */
> -static void
> -convertToWithoutRowidTable(Parse * pParse, Table * pTab)

I see reference to this function in comment to struct Index.
Remove it as well.

> 
> @@ -2559,7 +2519,9 @@ tnt_error:
> static bool
> constraint_is_named(const char *name)
> {
> -	return strncmp(name, "sql_autoindex_", strlen("sql_autoindex_"));
> +	return strncmp(name, "sql_autoindex_", strlen("sql_autoindex_")) &&
> +		strncmp(name, "pk_unnamed_", strlen("pk_unnamed_")) &&
> +		strncmp(name, "unique_unnamed_", strlen("unique_unnamed_"));
> }
> 
> void
> @@ -2648,29 +2610,36 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 				sqlite3NameFromToken(db,
> 						     &parse->constraintName);
> 
> +	       /*
> +		* 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.
> +		*/
> +		const char *prefix = "sql_autoindex_%s_%d”;

If we trapped so far, index is definitely UNIQUE or PK constraint,
so this initialisation with default prefix is redundant.

And actually I don’t understand why do you need this renaming...

> +		if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE) {
> +			prefix = constraint_name == NULL ?
> +				"unique_unnamed_%s_%d" : "unique_%s_%d";
> +		}
> +		else {
> +			if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK)
> +				prefix = constraint_name == NULL ?
> +					"pk_unnamed_%s_%d" : "pk_%s_%d";
> +		}
> +
> +		uint32_t n = 1;
> +		for (struct Index *idx = table->pIndex; idx != NULL;
> +		     idx = idx->pNext, n++);
> +
> 		if (constraint_name == NULL ||
> 		    strcmp(constraint_name, "") == 0) {
> -			uint32_t n = 1;
> -			for (struct Index *idx = table->pIndex; idx != NULL;
> -			     idx = idx->pNext, n++);
> -			name = sqlite3MPrintf(db, "sql_autoindex_%s_%d",
> +			name = sqlite3MPrintf(db, prefix,
> 					      table->def->name, n);
> 		} else {
> -			/*
> -			 * 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);
> +			name = sqlite3MPrintf(db, prefix,
> +					      constraint_name, n);
> 		}
> 		sqlite3DbFree(db, constraint_name);
> 	}
> 
> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index 7b1f6ec..cabe22b 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -726,22 +726,20 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
> 						int iKey = pFK->aCol[0].iFrom;
> 						assert(iKey >= 0 && iKey <
> 						       (int)pTab->def->field_count);
> -						if (iKey != pTab->iPKey) {
> -							sqlite3VdbeAddOp3(v,
> -									  OP_Column,
> -									  0,
> -									  iKey,
> -									  regRow);
> -							sqlite3ColumnDefault(v,
> -									     pTab->def,
> -									     iKey,
> -									     regRow);
> -							sqlite3VdbeAddOp2(v,
> -									  OP_IsNull,
> -									  regRow,
> -									  addrOk);
> -							VdbeCoverage(v);
> -						}
> +						sqlite3VdbeAddOp3(v,
> +								  OP_Column,
> +								  0,
> +								  iKey,
> +								  regRow);
> +						sqlite3ColumnDefault(v,
> +								     pTab->def,
> +								     iKey,
> +								     regRow);
> +						sqlite3VdbeAddOp2(v,
> +								  OP_IsNull,
> +								  regRow,
> +								  addrOk);
> +						VdbeCoverage(v);
> 						VdbeCoverage(v);

Double call of VdbeCoverage();