Hello. Thanks for the patch! But it again is raw. You skipped
field_def.coll initialization for non-sql code, alter.cc etc.
On 16/07/2018 19:33, Kirill Shcherbatov wrote:
Thank you for review.
1. And why do you need nAlloc now? Please, do not hurry. It is not ok that- nAlloc = (((pNew->def->field_count - 1) / 8) * 8) + 8;
you overlook such conspicuous things.
- assert((uint32_t)nAlloc >= pNew->def->field_count && nAlloc % 8 == 0 &&
- nAlloc - pNew->def->field_count < 8);
Dropped.
2. What does this comment mean?- for (i = 0; i < pNew->def->field_count; i++) {
3. What about other fields? Please, use = field_def_default.
- /* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */
- pNew->def->fields[i].coll = NULL;
- pNew->def->fields[i].coll_id = COLL_NONE;
- }
+ for (uint32_t i = 0; i < pNew->def->field_count; i++)
+ pNew->def->fields[i] = field_def_default;
4. No. As I said you on the first review, you should not use herediff --git a/src/box/sql/build.c b/src/box/sql/build.c
scans or is_primkey. This check can be spread over other column check
functions, involved in the first commit with no additional scans.
index a6f3559..3449d3c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -126,38 +126,48 @@ index_has_column(const i16 *parts, int parts_count, int
column_idx)
/**
* This is a function which should be called during execution
- * of sqlite3EndTable. It ensures that only PRIMARY KEY
- * constraint may have ON CONFLICT REPLACE clause.
+ * of sqlite3EndTable. It set defaults for columns having no
+ * separate NULL/NOT NULL specifiers and ensures that only
+ * PRIMARY KEY constraint may have ON CONFLICT REPLACE clause.
*
+ * @param parser SQL Parser object.
* @param table Space which should be checked.
- * @retval False, if only primary key constraint has
- * ON CONFLICT REPLACE clause or if there are no indexes
- * with REPLACE as error action. True otherwise.
+ * @retval -1 on error. Parser SQL_TARANTOOL_ERROR is set.
+ * @retval 0 on success.
*/
-static bool
-check_on_conflict_replace_entries(struct Table *table)
-{
- /* Check all NOT NULL constraints. */
- for (int i = 0; i < (int)table->def->field_count; i++) {
- enum on_conflict_action on_error =
- table->def->fields[i].nullable_action;
- struct Index *pk = sqlite3PrimaryKeyIndex(table);
- if (on_error == ON_CONFLICT_ACTION_REPLACE &&
+static int
+check_on_conflict_replace_entries(struct Parse *parser, struct Table *table)
+{
+ const char *err_msg = NULL;
+ struct field_def *field = table->def->fields;
+ struct Index *pk = sqlite3PrimaryKeyIndex(table);
+ for (uint32_t i = 0; i < table->def->field_count; ++i, ++field) {
+ if (field->nullable_action == on_conflict_action_MAX) {
+ /* Set default. */
+ field->nullable_action = ON_CONFLICT_ACTION_NONE;
+ field->is_nullable = true;
+ }
+ if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
!index_has_column(pk->aiColumn, pk->nColumn, i))
- return true;
+ goto non_pk_on_conflict_error;
}
- /* Check all UNIQUE constraints. */
+
for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
if (idx->onError == ON_CONFLICT_ACTION_REPLACE &&
- !IsPrimaryKeyIndex(idx)) {
- return true;
- }
+ !IsPrimaryKeyIndex(idx))
+ goto non_pk_on_conflict_error;
}
- /*
- * CHECK constraints are not allowed to have REPLACE as
- * error action and therefore can be skipped.
- */
- return false;
+
+ return 0;
+
+non_pk_on_conflict_error:
+ err_msg = tt_sprintf("only PRIMARY KEY constraint can have "
+ "ON CONFLICT REPLACE clause - %s",
+ table->def->name);
+ diag_set(ClientError, ER_SQL, err_msg);
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
+ return -1;
}
/*
@@ -1748,22 +1758,8 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
}
}
- /* Set default on_nullable action if required. */
- struct field_def *field = p->def->fields;
- for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) {
- if (field->nullable_action == on_conflict_action_MAX) {
- field->nullable_action = ON_CONFLICT_ACTION_NONE;
- field->is_nullable = true;
- }
- }
-
- if (check_on_conflict_replace_entries(p)) {
- sqlite3ErrorMsg(pParse,
- "only PRIMARY KEY constraint can "
- "have ON CONFLICT REPLACE clause "
- "- %s", p->def->name);
+ if (check_on_conflict_replace_entries(pParse, p))
goto cleanup;
- }
if (db->init.busy) {
/*
@@ -1906,6 +1902,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
cleanup:
sql_expr_list_delete(db, p->def->opts.checks);
p->def->opts.checks = NULL;
+ return;
}
5. Please, make the code more readable. You should avoid such long names@@ -905,16 +915,15 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing
as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields
in a separate variable etc.
context */
sqlite3ExprSkipCollate(pList->a[i].pExpr);
assert(pCExpr != 0);
if (pCExpr->op != TK_ID) {
- sqlite3ErrorMsg(pParse, "expressions prohibited in
PRIMARY KEY");
+ sqlite3ErrorMsg(pParse, "expressions prohibited"
+ " in PRIMARY KEY");
goto primary_key_exit;
}
- const char *zCName = pCExpr->u.zToken;
- for (uint32_t collumn_id = 0;
- collumn_id < pTab->def->field_count; collumn_id++)
{
- if (strcmp(zCName,
- pTab->def->fields[collumn_id].name)
- == 0) {
- iCol = collumn_id;
+ const char *name = pCExpr->u.zToken;
+ struct space_def *def = pTab->def;
+ for (uint32_t idx = 0; idx < def->field_count; idx++) {
+ if (strcmp(name, def->fields[idx].name) == 0) {
+ iCol = idx;
break;
}
}
6. Why do you need to enclose != NULL into the brackets?- (expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0;
+ expr_list != NULL ? (uint32_t)expr_list->nExpr : 0;
Hm, It's is a little more convenient to read for me. I'll try to be a little
more pedantic.
