iproto: IPROTO_SQL_EXECUTE review fixes

* better messages
* fold sql_prepare()
* rewrite the message decode

src/box/errcode.h

@@ -113,7 +113,7 @@ struct errcode_record {
 	/* 58 */_(ER_RELOAD_CFG,		"Can't set option '%s' dynamically") \
 	/* 59 */_(ER_CFG,			"Incorrect value for option '%s': %s") \
 	/* 60 */_(ER_SQL,			"SQL error: %s") \
-	/* 61 */_(ER_SQL_BIND_NOT_FOUND,	"Illegal named parameter '%s', the name was not found in the statement") \
+	/* 61 */_(ER_SQL_BIND_NOT_FOUND,	"Parameter %s was not found in the statement") \
 	/* 62 */_(ER_UNKNOWN_REPLICA,		"Replica %s is not registered with replica set %s") \
 	/* 63 */_(ER_REPLICASET_UUID_MISMATCH,	"Replica set UUID of the replica %s doesn't match replica set UUID of the master %s") \
 	/* 64 */_(ER_INVALID_UUID,		"Invalid UUID: %s") \
@@ -192,8 +192,8 @@ struct errcode_record {
 	/*137 */_(ER_TRUNCATE_SYSTEM_SPACE,	"Can't truncate a system space, space '%s'") \
 	/*138 */_(ER_SQL_BIND_VALUE,		"Bind value for parameter %s is out of range for type %s") \
 	/*139 */_(ER_SQL_BIND_TYPE,		"Bind value type %s for parameter %s is not supported") \
-	/*140 */_(ER_SQL_BIND_COUNT_MAX,	"Too many SQL variables") \
-	/*141 */_(ER_SQL_EXECUTE,		"%s") \
+	/*140 */_(ER_SQL_BIND_PARAMETER_MAX,	"SQL bind parameter limit reached: %d") \
+	/*141 */_(ER_SQL_EXECUTE,		"Failed to execute SQL statement: %s") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file

src/box/execute.c

@@ -205,8 +205,9 @@ sql_bind_list_decode(const char *data, uint32_t bind_count,
 {
 	assert(bind_count > 0);
 	assert(data != NULL);
-	if (bind_count > SQL_VARIABLE_NUMBER_MAX) {
-		diag_set(ClientError, ER_SQL_BIND_COUNT_MAX);
+	if (bind_count >= SQL_VARIABLE_NUMBER_MAX) {
+		diag_set(ClientError, ER_SQL_BIND_PARAMETER_MAX,
+			 (int) bind_count);
 		return NULL;
 	}
 	uint32_t used = region_used(region);
@@ -261,43 +262,39 @@ sql_request_decode(struct sql_request *request, const char *data, uint32_t len,
 {
 	const char *end = data + len;
 	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
-mp_error:
+error:
 		diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
 		return -1;
 	}
-	uint32_t size = mp_decode_map(&data);
+	uint32_t map_size = mp_decode_map(&data);
 	request->sql_text = NULL;
 	request->bind = NULL;
 	request->bind_count = 0;
 	request->sync = sync;
-	for (uint32_t i = 0; i < size; ++i) {
-		uint64_t key = mp_decode_uint(&data);
-		const char *value = data;
-		if (mp_check(&data, end) != 0)
-			goto mp_error;
-		switch (key) {
-			case IPROTO_SQL_BIND:
-				if (sql_bind_parse(request, value, region) != 0)
-					return -1;
-				break;
-			case IPROTO_SQL_TEXT:
-				request->sql_text = value;
-				break;
-			default:
-				break;
+	for (uint32_t i = 0; i < map_size; ++i) {
+		uint8_t key = *data;
+		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
+			mp_check(&data, end);   /* skip the key */
+			mp_check(&data, end);   /* skip the value */
+			continue;
+		}
+		const char *value = ++data;     /* skip the key */
+		if (mp_check(&data, end) != 0)  /* check the value */
+			goto error;
+		if (key == IPROTO_SQL_BIND) {
+			if (sql_bind_parse(request, value, region) != 0)
+				return -1;
+		} else {
+			request->sql_text = value;
 		}
 	}
-#ifndef NDEBUG
-	if (data != end) {
-		diag_set(ClientError, ER_INVALID_MSGPACK, "packet end");
-		return -1;
-	}
-#endif
 	if (request->sql_text == NULL) {
 		diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
 			 iproto_key_name(IPROTO_SQL_TEXT));
 		return -1;
 	}
+	if (data != end)
+		goto error;
 	return 0;
 }
 
@@ -422,7 +419,7 @@ sql_bind_column(struct sqlite3_stmt *stmt, const struct sql_bind *p,
 		pos = sqlite3_bind_parameter_lindex(stmt, p->name, p->name_len);
 		if (pos == 0) {
 			diag_set(ClientError, ER_SQL_BIND_NOT_FOUND,
-				 tt_cstr(p->name, p->name_len));
+				 sql_bind_name(p));
 			return -1;
 		}
 	}
@@ -490,28 +487,6 @@ sql_bind(struct sql_request *request, struct sqlite3_stmt *stmt)
 	return 0;
 }
 
-/**
- * Prepare an SQL query.
- * @param db SQLite engine.
- * @param[out] stmt SQL statement to prepare.
- * @param sql SQL query.
- * @param length Length of the @sql.
- *
- * @retval  0 Success.
- * @retval -1 Client or memory error.
- */
-static inline int
-sql_prepare(sqlite3 *db, struct sqlite3_stmt **stmt, const char *sql,
-	    uint32_t length)
-{
-	if (sqlite3_prepare_v2(db, sql, length, stmt, &sql) != SQLITE_OK) {
-		diag_set(ClientError, ER_SQL, sqlite3_errmsg(db));
-		sqlite3_finalize(*stmt);
-		return -1;
-	}
-	return 0;
-}
-
 /**
  * Serialize a description of the prepared statement.
  * @param stmt Prepared statement.
@@ -602,47 +577,48 @@ int
 sql_prepare_and_execute(struct sql_request *request, struct obuf *out)
 {
 	const char *sql = request->sql_text;
-	uint32_t length;
-	sql = mp_decode_str(&sql, &length);
+	uint32_t len;
+	sql = mp_decode_str(&sql, &len);
 	struct sqlite3_stmt *stmt;
 	sqlite3 *db = sql_get();
 	if (db == NULL) {
-		diag_set(ClientError, ER_SQL, "sql processor is not ready");
+		diag_set(ClientError, ER_LOADING);
 		return -1;
 	}
 
-	if (sql_prepare(db, &stmt, sql, length) != 0)
+	if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
+		diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
 		return -1;
+	}
 	if (sql_bind(request, stmt) != 0)
-		goto bind_error;
+		goto err_stmt;
 
 	/* Prepare memory for the iproto header. */
-	struct obuf_svp svp;
-	if (iproto_prepare_header(out, &svp, PREPARE_SQL_SIZE) != 0)
-		goto error;
+	struct obuf_svp header_svp;
+	if (iproto_prepare_header(out, &header_svp, PREPARE_SQL_SIZE) != 0)
+		goto err_stmt;
 
 	/* Encode description. */
 	struct obuf_svp sql_svp;
 	if (iproto_prepare_body_key(out, &sql_svp) != 0)
-		goto error;
-	if (sql_get_description(stmt, out, &length) != 0)
-		goto error;
-	iproto_reply_body_key(out, &sql_svp, length, IPROTO_DESCRIPTION);
+		goto err_svp;
+	if (sql_get_description(stmt, out, &len) != 0)
+		goto err_svp;
+	iproto_reply_body_key(out, &sql_svp, len, IPROTO_DESCRIPTION);
 
 	/* Encode data set. */
 	if (iproto_prepare_body_key(out, &sql_svp) != 0)
-		goto error;
-	if (sql_execute(db, stmt, out, &length) != 0)
-		goto error;
-	sqlite3_finalize(stmt);
-	iproto_reply_body_key(out, &sql_svp, length, IPROTO_DATA);
+		goto err_svp;
+	if (sql_execute(db, stmt, out, &len) != 0)
+		goto err_svp;
+	iproto_reply_body_key(out, &sql_svp, len, IPROTO_DATA);
 
-	iproto_reply_sql(out, &svp, request->sync, schema_version);
+	iproto_reply_sql(out, &header_svp, request->sync, schema_version);
+	sqlite3_finalize(stmt);
 	return 0;
-
-error:
-	obuf_rollback_to_svp(out, &svp);
-bind_error:
+err_svp:
+	obuf_rollback_to_svp(out, &header_svp);
+err_stmt:
 	sqlite3_finalize(stmt);
 	return -1;
 }

src/box/lua/space.cc

@@ -363,6 +363,6 @@ box_lua_space_init(struct lua_State *L)
 	lua_pushnumber(L, VCLOCK_MAX);
 	lua_setfield(L, -2, "REPLICA_MAX");
 	lua_pushnumber(L, SQL_VARIABLE_NUMBER_MAX);
-	lua_setfield(L, -2, "SQL_VARIABLE_NUMBER_MAX");
+	lua_setfield(L, -2, "SQL_BIND_PARAMETER_MAX");
 	lua_pop(L, 2); /* box, schema */
 }

test/box/misc.result

@@ -308,6 +308,7 @@ t;
   - 'box.error.NO_SUCH_TRIGGER : 34'
   - 'box.error.CHECKPOINT_IN_PROGRESS : 120'
   - 'box.error.FIELD_TYPE : 23'
+  - 'box.error.SQL_BIND_PARAMETER_MAX : 140'
   - 'box.error.SQL_BIND_TYPE : 139'
   - 'box.error.UNKNOWN_UPDATE_OP : 28'
   - 'box.error.TUPLE_REF_OVERFLOW : 86'
@@ -375,11 +376,10 @@ t;
   - 'box.error.NO_SUCH_ENGINE : 57'
   - 'box.error.COMMIT_IN_SUB_STMT : 122'
   - 'box.error.LAST_DROP : 15'
-  - 'box.error.injection : table: <address>
   - 'box.error.DECOMPRESSION : 124'
-  - 'box.error.SQL_EXECUTE : 141'
+  - 'box.error.injection : table: <address>
   - 'box.error.CREATE_USER : 43'
-  - 'box.error.UNSUPPORTED : 5'
+  - 'box.error.SQL_EXECUTE : 141'
   - 'box.error.INSTANCE_UUID_MISMATCH : 66'
   - 'box.error.TRUNCATE_SYSTEM_SPACE : 137'
   - 'box.error.SYSTEM : 115'
@@ -408,7 +408,7 @@ t;
   - 'box.error.ROLE_NOT_GRANTED : 92'
   - 'box.error.SPACE_EXISTS : 10'
   - 'box.error.TUPLE_FOUND : 3'
-  - 'box.error.SQL_BIND_COUNT_MAX : 140'
+  - 'box.error.UNSUPPORTED : 5'
   - 'box.error.NO_SUCH_SPACE : 36'
   - 'box.error.WRONG_INDEX_PARTS : 107'
   - 'box.error.REPLICASET_UUID_MISMATCH : 63'

test/sql/iproto.result

@@ -58,11 +58,11 @@ cn:execute('delete from test where a = 5')
 -- SQL errors.
 cn:execute('insert into not_existing_table values ("kek")')
 ---
-- error: 'SQL error: no such table: not_existing_table'
+- error: 'Failed to execute SQL statement: no such table: not_existing_table'
 ...
 cn:execute('insert qwerty gjsdjq  q  qwd qmq;; q;qwd;')
 ---
-- error: 'SQL error: near "qwerty": syntax error'
+- error: 'Failed to execute SQL statement: near "qwerty": syntax error'
 ...
 -- Empty result.
 cn:execute('select id as identifier from test where a = 5;')
@@ -73,7 +73,7 @@ cn:execute('select id as identifier from test where a = 5;')
 -- netbox API errors.
 cn:execute(100)
 ---
-- error: 'SQL error: near "100": syntax error'
+- error: 'Failed to execute SQL statement: near "100": syntax error'
 ...
 cn:execute('select 1', nil, {dry_run = true})
 ---
@@ -192,15 +192,15 @@ parameters[1]['name'] = 300
 ...
 cn:execute('select ? as kek', parameters)
 ---
-- error: Illegal named parameter 'name', the name was not found in the statement
+- error: Parameter 'name' was not found in the statement
 ...
--- Try too many parameters in the query.
-sql = 'select '..string.rep('?, ', box.schema.SQL_VARIABLE_NUMBER_MAX)..'?'
+-- Try too many parameters in a statement.
+sql = 'select '..string.rep('?, ', box.schema.SQL_BIND_PARAMETER_MAX)..'?'
 ---
 ...
 cn:execute(sql)
 ---
-- error: 'SQL error: too many SQL variables'
+- error: 'Failed to execute SQL statement: too many SQL variables'
 ...
 -- Try too many parameter values.
 sql = 'select ?'
@@ -209,12 +209,12 @@ sql = 'select ?'
 parameters = {}
 ---
 ...
-for i = 1, box.schema.SQL_VARIABLE_NUMBER_MAX + 1 do parameters[i] = i end
+for i = 1, box.schema.SQL_BIND_PARAMETER_MAX + 1 do parameters[i] = i end
 ---
 ...
 cn:execute(sql, parameters)
 ---
-- error: Too many SQL variables
+- error: 'SQL bind parameter limit reached: 65001'
 ...
 --
 -- Errors during parameters binding.

test/sql/iproto.test.lua

@@ -75,14 +75,14 @@ parameters[1] = {}
 parameters[1]['name'] = 300
 cn:execute('select ? as kek', parameters)
 
--- Try too many parameters in the query.
-sql = 'select '..string.rep('?, ', box.schema.SQL_VARIABLE_NUMBER_MAX)..'?'
+-- Try too many parameters in a statement.
+sql = 'select '..string.rep('?, ', box.schema.SQL_BIND_PARAMETER_MAX)..'?'
 cn:execute(sql)
 
 -- Try too many parameter values.
 sql = 'select ?'
 parameters = {}
-for i = 1, box.schema.SQL_VARIABLE_NUMBER_MAX + 1 do parameters[i] = i end
+for i = 1, box.schema.SQL_BIND_PARAMETER_MAX + 1 do parameters[i] = i end
 cn:execute(sql, parameters)
 
 --