[patches] [PATCH 1/1] sql: improve Tarantool errors handling
Nikita Pettik
korablev at tarantool.org
Mon Feb 12 15:26:53 MSK 2018
It turns out that sometimes when tarantoolSqlite* routine returns error,
Tarantool doesn't actually set any error messages. Thus, call to
diag_last() will result in NULL dereference. This patch makes SQL print
default error message, if Tarantool doesn't set any error message. In
order to distinguish some types of errors, additional return codes have
been added.
Closes #3114
---
src/box/sql.c | 72 ++++++++++++++++++++++++++-------------------
src/box/sql/main.c | 7 +++--
src/box/sql/sqlite3.h | 9 ++++--
src/box/sql/tarantoolInt.h | 2 ++
src/box/sql/vdbe.c | 17 +++++++----
src/box/sql/vdbeaux.c | 2 +-
test/sql-tap/alter.test.lua | 2 +-
7 files changed, 69 insertions(+), 42 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index ef73d543a..3d202f3b3 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -184,9 +184,20 @@ cursor_ephemeral_advance(BtCursor *pCur, int *pRes);
const char *tarantoolErrorMessage()
{
+ if (diag_is_empty(&fiber()->diag))
+ return NULL;
return box_error_message(box_error_last());
}
+int
+is_tarantool_error(int rc)
+{
+ return (rc == SQL_TARANTOOL_ERROR ||
+ rc == SQL_TARANTOOL_ITERATOR_FAIL ||
+ rc == SQL_TARANTOOL_DELETE_FAIL ||
+ rc == SQL_TARANTOOL_INSERT_FAIL);
+}
+
int tarantoolSqlite3CloseCursor(BtCursor *pCur)
{
assert(pCur->curFlags & BTCF_TaCursor ||
@@ -483,7 +494,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
&key_list);
if (ephemer_new_space == NULL) {
diag_log();
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ERROR;
}
struct ta_cursor *c = NULL;
c = cursor_create(c, field_count /* key size */);
@@ -521,7 +532,7 @@ int tarantoolSqlite3EphemeralInsert(BtCursor *pCur, const CursorPayload *pX)
if (space_ephemeral_replace(space, pX->pKey,
pX->pKey + pX->nKey) != 0) {
diag_log();
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_INSERT_FAIL;
}
return SQLITE_OK;
}
@@ -549,7 +560,7 @@ static int insertOrReplace(BtCursor *pCur, const CursorPayload *pX,
char *buf = (char*)region_alloc(&fiber()->gc, pX->nKey);
if (buf == NULL) {
diag_set(OutOfMemory, pX->nKey, "region", "buf");
- return SQLITE_TARANTOOL_ERROR;
+ return SQLITE_NOMEM;
}
memcpy(buf, pX->pKey, pX->nKey);
@@ -564,7 +575,7 @@ static int insertOrReplace(BtCursor *pCur, const CursorPayload *pX,
NULL /* result */);
}
- return rc == 0 ? SQLITE_OK : SQLITE_TARANTOOL_ERROR;;
+ return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;;
}
int tarantoolSqlite3Insert(BtCursor *pCur, const CursorPayload *pX)
@@ -602,12 +613,12 @@ int tarantoolSqlite3EphemeralDelete(BtCursor *pCur)
box_iterator_key_def(c->iter),
&key_size);
if (key == NULL)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_DELETE_FAIL;
int rc = space_ephemeral_delete(ephem_space, key);
if (rc != 0) {
diag_log();
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_DELETE_FAIL;
}
return SQLITE_OK;
}
@@ -634,11 +645,11 @@ int tarantoolSqlite3Delete(BtCursor *pCur, u8 flags)
box_iterator_key_def(c->iter),
&key_size);
if (key == NULL)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_DELETE_FAIL;
rc = box_delete(space_id, index_id, key, key + key_size, NULL);
- return rc == 0 ? SQLITE_OK : SQLITE_TARANTOOL_ERROR;
+ return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_DELETE_FAIL;
}
/*
@@ -663,7 +674,7 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
0 /* part_count */);
if (it == NULL) {
pCur->eState = CURSOR_INVALID;
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
}
struct tuple *tuple;
@@ -674,7 +685,8 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
key = tuple_extract_key(tuple, box_iterator_key_def(it),
&key_size);
if (space_ephemeral_delete(ephem_space, key) != 0) {
- return SQLITE_TARANTOOL_ERROR;
+ iterator_delete(it);
+ return SQL_TARANTOOL_DELETE_FAIL;
}
}
iterator_delete(it);
@@ -706,19 +718,21 @@ int tarantoolSqlite3ClearTable(int iTable)
iter = box_index_iterator(space_id, primary_index_id, ITER_ALL,
nil_key, nil_key + sizeof(nil_key));
if (iter == NULL)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
while (box_iterator_next(iter, &tuple) == 0 && tuple != NULL) {
key = tuple_extract_key(tuple,
box_iterator_key_def(iter),
&key_size);
rc = box_delete(space_id, primary_index_id, key,
key + key_size, NULL);
- if (rc != 0)
- return SQLITE_TARANTOOL_ERROR;
+ if (rc != 0) {
+ box_iterator_free(iter);
+ return SQL_TARANTOOL_DELETE_FAIL;
+ }
}
box_iterator_free(iter);
} else if (box_truncate(space_id) != 0) {
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_DELETE_FAIL;
}
return SQLITE_OK;
@@ -796,7 +810,7 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
rename_fail:
box_iterator_free(iter);
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ERROR;
}
/*
@@ -915,9 +929,8 @@ int tarantoolSqlite3RenameTable(int iTab, const char *new_name, char **sql_stmt)
return SQLITE_OK;
rename_fail:
- sqlite3Error(db, SQLITE_ERROR);
box_iterator_free(iter);
- return SQLITE_ERROR;
+ return SQL_TARANTOOL_ERROR;
}
/*
@@ -1007,9 +1020,8 @@ int tarantoolSqlite3RenameParentTable(int iTab, const char *old_parent_name,
return SQLITE_OK;
rename_fail:
- sqlite3Error(db, SQLITE_ERROR);
box_iterator_free(iter);
- return SQLITE_ERROR;
+ return SQL_TARANTOOL_ERROR;
}
@@ -1144,7 +1156,7 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
0,
&res);
if (rc != 0 || res == NULL) {
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ERROR;
}
if (!c) {
c = cursor_create(NULL, 0);
@@ -1241,14 +1253,14 @@ cursor_ephemeral_seek(BtCursor *pCur, int *pRes, enum iterator_type type,
struct index *index = *ephem_space->index;
if (key_validate(index->def, type, key, part_count)) {
diag_log();
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
}
struct iterator *it = index_create_iterator(*ephem_space->index, type,
key, part_count);
if (it == NULL) {
pCur->eState = CURSOR_INVALID;
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
}
c->iter = it;
c->type = type;
@@ -1296,7 +1308,7 @@ cursor_seek(BtCursor *pCur, int *pRes, enum iterator_type type,
c->iter = box_index_iterator(space_id, index_id, type, k, ke);
if (c->iter == NULL) {
pCur->eState = CURSOR_INVALID;
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
}
c->type = type;
pCur->eState = CURSOR_VALID;
@@ -1323,9 +1335,9 @@ cursor_ephemeral_advance(BtCursor *pCur, int *pRes)
struct tuple *tuple;
if (iterator_next(c->iter, &tuple) != 0)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
if (tuple != NULL && tuple_bless(tuple) == NULL)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
if (c->tuple_last) box_tuple_unref(c->tuple_last);
if (tuple) {
box_tuple_ref(tuple);
@@ -1353,7 +1365,7 @@ cursor_advance(BtCursor *pCur, int *pRes)
rc = box_iterator_next(c->iter, &tuple);
if (rc)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ITERATOR_FAIL;
if (c->tuple_last) box_tuple_unref(c->tuple_last);
if (tuple) {
box_tuple_ref(tuple);
@@ -1492,7 +1504,7 @@ void tarantoolSqlite3LoadSchema(InitData *init)
/* Read _space */
if (space_foreach(space_foreach_put_cb, init) != 0) {
- init->rc = SQLITE_TARANTOOL_ERROR;
+ init->rc = SQL_TARANTOOL_ERROR;
return;
}
@@ -1501,7 +1513,7 @@ void tarantoolSqlite3LoadSchema(InitData *init)
nil_key, nil_key + sizeof(nil_key));
if (it == NULL) {
- init->rc = SQLITE_TARANTOOL_ERROR;
+ init->rc = SQL_TARANTOOL_ITERATOR_FAIL;
return;
}
@@ -1833,10 +1845,10 @@ int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno,
uint32_t part_count = primary_index->def->key_def->part_count;
if (index_max(primary_index, key, part_count, &tuple) != 0) {
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ERROR;
}
if (tuple != NULL && tuple_bless(tuple) == NULL)
- return SQLITE_TARANTOOL_ERROR;
+ return SQL_TARANTOOL_ERROR;
if (tuple == NULL) {
*max_id = 0;
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 4f43cd206..6f578459e 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1222,7 +1222,7 @@ sqlite3ErrName(int rc)
case SQLITE_NOTADB:
zName = "SQLITE_NOTADB";
break;
- case SQLITE_TARANTOOL_ERROR:
+ case SQL_TARANTOOL_ERROR:
zName = "SQLITE_TARANTOOL_ERROR";
break;
case SQLITE_ROW:
@@ -1295,7 +1295,10 @@ sqlite3ErrStr(int rc)
/* SQLITE_RANGE */ "bind or column index out of range",
/* SQLITE_NOTADB */
"file is encrypted or is not a database",
- /* SQLITE_TARANTOOL_ERROR */ "sqlite/tarantool error",
+ /* SQL_TARANTOOL_ITERATOR_FAIL */ "Tarantool's iterator failed",
+ /* SQL_TARANTOOL_INSERT_FAIL */ "Tarantool's insert failed",
+ /* SQL_TARANTOOL_DELETE_FAIL */ "Tarantool's delete failed",
+ /* SQL_TARANTOOL_ERROR */ "SQL-/Tarantool error",
};
const char *zErr = "unknown error";
switch (rc) {
diff --git a/src/box/sql/sqlite3.h b/src/box/sql/sqlite3.h
index a89d5405d..f9821454a 100644
--- a/src/box/sql/sqlite3.h
+++ b/src/box/sql/sqlite3.h
@@ -442,9 +442,12 @@ sqlite3_exec(sqlite3 *, /* An open database */
#define SQLITE_FORMAT 24 /* Auxiliary database format error */
#define SQLITE_RANGE 25 /* 2nd parameter to sqlite3_bind out of range */
#define SQLITE_NOTADB 26 /* File opened that is not a database file */
-#define SQLITE_TARANTOOL_ERROR 27
-#define SQLITE_NOTICE 28 /* Notifications from sqlite3_log() */
-#define SQLITE_WARNING 29 /* Warnings from sqlite3_log() */
+#define SQL_TARANTOOL_ITERATOR_FAIL 27
+#define SQL_TARANTOOL_INSERT_FAIL 28
+#define SQL_TARANTOOL_DELETE_FAIL 29
+#define SQL_TARANTOOL_ERROR 30
+#define SQLITE_NOTICE 31 /* Notifications from sqlite3_log() */
+#define SQLITE_WARNING 32 /* Warnings from sqlite3_log() */
#define SQLITE_ROW 100 /* sqlite3_step() has another row ready */
#define SQLITE_DONE 101 /* sqlite3_step() has finished executing */
/* end-of-error-codes */
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 134388d61..340936aca 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -49,6 +49,8 @@ void tarantoolSqlite3LoadSchema(InitData * init);
/* Misc */
const char *tarantoolErrorMessage();
+int is_tarantool_error(int rc);
+
/* Storage interface. */
int tarantoolSqlite3CloseCursor(BtCursor * pCur);
const void *tarantoolSqlite3PayloadFetch(BtCursor * pCur, u32 * pAmt);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c3d726672..413dc733d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2931,7 +2931,7 @@ case OP_FkCheckCommit: {
sqlite3VdbeHalt(p);
goto vdbe_return;
} else {
- rc = box_txn_commit() == 0 ? SQLITE_OK : SQLITE_TARANTOOL_ERROR;
+ rc = box_txn_commit() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR;
if (rc) goto abort_due_to_error;
}
break;
@@ -3067,7 +3067,7 @@ case OP_Transaction: {
*/
case OP_TTransaction: {
if (p->autoCommit) {
- rc = box_txn_begin() == 0 ? SQLITE_OK : SQLITE_TARANTOOL_ERROR;
+ rc = box_txn_begin() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR;
}
if (box_txn()
&& p->autoCommit == 0){
@@ -5688,9 +5688,16 @@ abort_due_to_error:
if (db->mallocFailed) rc = SQLITE_NOMEM_BKPT;
assert(rc);
if (p->zErrMsg==0 && rc!=SQLITE_IOERR_NOMEM) {
- const char *m = (rc!=SQLITE_TARANTOOL_ERROR) ? sqlite3ErrStr(rc) :
- tarantoolErrorMessage();
- sqlite3VdbeError(p, "%s", m);
+ const char *msg;
+ /* Avoiding situation when Tarantool error is set,
+ * but error message isn't.
+ */
+ if (is_tarantool_error(rc) && tarantoolErrorMessage()) {
+ msg = tarantoolErrorMessage();
+ } else {
+ msg = sqlite3ErrStr(rc);
+ }
+ sqlite3VdbeError(p, "%s", msg);
}
p->rc = rc;
sqlite3SystemError(db, rc);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index df6881488..c87aa5ca6 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2673,7 +2673,7 @@ sqlite3VdbeHalt(Vdbe * p)
*/
rc = box_txn_commit() ==
0 ? SQLITE_OK :
- SQLITE_TARANTOOL_ERROR;
+ SQL_TARANTOOL_ERROR;
closeCursorsAndFree(p);
}
if (rc == SQLITE_BUSY && p->readOnly) {
diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 1a4ceaaf3..801f8c178 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -93,7 +93,7 @@ test:do_catchsql_test(
ALTER TABLE "_space" RENAME TO space;
]], {
-- <alter-2.3>
- 1, "/logic error/"
+ 1, "SQL-\/Tarantool error"
-- </alter-2.3>
})
--
2.15.1
More information about the Tarantool-patches
mailing list