* [tarantool-patches] [PATCH v1 0/4] sql: Checks on server side
@ 2019-01-10 13:54 Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-01-10 13:54 UTC (permalink / raw)
To: tarantool-patches, korablev; +Cc: Kirill Shcherbatov
Implemented SQL Checks validations on server side.
Introduced a new sql_check class containing precompiled reusable VDBE making
required validations. It has been parameterized with binding parameters to map
a tuple to be inserted in VDBE memory and to manage checks to be performed on
UPDATE operation.
http://github.com/tarantool/tarantool/tree/kshch/gh-3691-checks-on-server-side
https://github.com/tarantool/tarantool/issues/3691
Kirill Shcherbatov (4):
box: rename space->opts checks to checks_ast
sql: disallow use of TYPEOF in Check
box: exported sql_bind structure and API
sql: make sql checks on server side
src/box/alter.cc | 4 +-
src/box/execute.c | 48 +------
src/box/execute.h | 52 ++++++++
src/box/space.c | 49 +++++++
src/box/space.h | 5 +
src/box/space_def.c | 15 ++-
src/box/space_def.h | 4 +-
src/box/sql.c | 179 +++++++++++++++++++++++++-
src/box/sql.h | 66 ++++++++++
src/box/sql/build.c | 42 ++++--
src/box/sql/insert.c | 107 ++++-----------
src/box/sql/sqliteInt.h | 32 +++++
src/box/sql/vdbe.c | 2 +-
src/box/sql/vdbeapi.c | 41 ++----
test/sql-tap/check.test.lua | 113 +++-------------
test/sql-tap/fkey2.test.lua | 4 +-
test/sql-tap/subquery.test.lua | 2 +-
test/sql-tap/table.test.lua | 8 +-
test/sql/checks.result | 51 ++++++++
test/sql/checks.test.lua | 18 +++
test/sql/gh-2981-check-autoinc.result | 8 +-
21 files changed, 563 insertions(+), 287 deletions(-)
--
2.19.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast
2019-01-10 13:54 [tarantool-patches] [PATCH v1 0/4] sql: Checks on server side Kirill Shcherbatov
@ 2019-01-10 13:54 ` Kirill Shcherbatov
2019-01-11 14:05 ` [tarantool-patches] " Konstantin Osipov
2019-01-18 18:00 ` Konstantin Osipov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-01-10 13:54 UTC (permalink / raw)
To: tarantool-patches, korablev; +Cc: Kirill Shcherbatov
The checks field of space_opts object renamed to checks_ast to avoid a
confusion due to introduction of a new object sql_check, representing
precompiled VDBE program to execute.
Need for #3691
---
src/box/alter.cc | 4 ++--
src/box/space_def.c | 15 ++++++++-------
src/box/space_def.h | 4 ++--
src/box/sql.c | 2 +-
src/box/sql/build.c | 17 +++++++++--------
5 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 0589c9678..dcbb1ca66 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -532,8 +532,8 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode,
engine_name, engine_name_len, &opts, fields,
field_count);
auto def_guard = make_scoped_guard([=] { space_def_delete(def); });
- if (def->opts.checks != NULL &&
- sql_checks_resolve_space_def_reference(def->opts.checks,
+ if (def->opts.checks_ast != NULL &&
+ sql_checks_resolve_space_def_reference(def->opts.checks_ast,
def) != 0) {
box_error_t *err = box_error_last();
if (box_error_code(err) != ENOMEM) {
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 3516bdd8d..1ed9d4280 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -55,7 +55,7 @@ const struct space_opts space_opts_default = {
/* .is_temporary = */ false,
/* .view = */ false,
/* .sql = */ NULL,
- /* .checks = */ NULL,
+ /* .checks_ast = */ NULL,
};
const struct opt_def space_opts_reg[] = {
@@ -63,7 +63,7 @@ const struct opt_def space_opts_reg[] = {
OPT_DEF("temporary", OPT_BOOL, struct space_opts, is_temporary),
OPT_DEF("view", OPT_BOOL, struct space_opts, is_view),
OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql),
- OPT_DEF_ARRAY("checks", struct space_opts, checks,
+ OPT_DEF_ARRAY("checks", struct space_opts, checks_ast,
checks_array_decode),
OPT_END,
};
@@ -112,15 +112,16 @@ space_def_dup_opts(struct space_def *def, const struct space_opts *opts)
return -1;
}
}
- if (opts->checks != NULL) {
- def->opts.checks = sql_expr_list_dup(sql_get(), opts->checks, 0);
- if (def->opts.checks == NULL) {
+ if (opts->checks_ast != NULL) {
+ def->opts.checks_ast =
+ sql_expr_list_dup(sql_get(), opts->checks_ast, 0);
+ if (def->opts.checks_ast == NULL) {
free(def->opts.sql);
diag_set(OutOfMemory, 0, "sql_expr_list_dup",
"def->opts.checks");
return -1;
}
- sql_checks_update_space_def_reference(def->opts.checks, def);
+ sql_checks_update_space_def_reference(def->opts.checks_ast, def);
}
return 0;
}
@@ -284,7 +285,7 @@ void
space_opts_destroy(struct space_opts *opts)
{
free(opts->sql);
- sql_expr_list_delete(sql_get(), opts->checks);
+ sql_expr_list_delete(sql_get(), opts->checks_ast);
TRASH(opts);
}
diff --git a/src/box/space_def.h b/src/box/space_def.h
index 8044f88fd..c23c578ff 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -66,8 +66,8 @@ struct space_opts {
bool is_view;
/** SQL statement that produced this space. */
char *sql;
- /** SQL Checks expressions list. */
- struct ExprList *checks;
+ /** Checks AST expressions list. */
+ struct ExprList *checks_ast;
};
extern const struct space_opts space_opts_default;
diff --git a/src/box/sql.c b/src/box/sql.c
index 081a038f1..26b84c5db 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1048,7 +1048,7 @@ sql_encode_table_opts(struct region *region, struct Table *table,
int checks_cnt = 0;
struct ExprList_item *a;
bool is_view = table->def->opts.is_view;
- struct ExprList *checks = table->def->opts.checks;
+ struct ExprList *checks = table->def->opts.checks_ast;
if (checks != NULL) {
checks_cnt = checks->nExpr;
a = checks->a;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 49b90b5d0..7e5bcc518 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -276,7 +276,7 @@ table_delete(struct sqlite3 *db, struct Table *tab)
for (uint32_t i = 0; i < tab->space->index_count; ++i)
index_def_delete(tab->space->index[i]->def);
/* Do not delete table->def allocated on region. */
- sql_expr_list_delete(db, tab->def->opts.checks);
+ sql_expr_list_delete(db, tab->def->opts.checks_ast);
} else if (tab->def->id == 0) {
space_def_delete(tab->def);
}
@@ -767,15 +767,16 @@ sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span)
(int)(span->zEnd - span->zStart));
if (expr->u.zToken == NULL)
goto release_expr;
- table->def->opts.checks =
+ table->def->opts.checks_ast =
sql_expr_list_append(parser->db,
- table->def->opts.checks, expr);
- if (table->def->opts.checks == NULL) {
+ table->def->opts.checks_ast, expr);
+ if (table->def->opts.checks_ast == NULL) {
sqlite3DbFree(parser->db, expr->u.zToken);
goto release_expr;
}
if (parser->constraintName.n) {
- sqlite3ExprListSetName(parser, table->def->opts.checks,
+ sqlite3ExprListSetName(parser,
+ table->def->opts.checks_ast,
&parser->constraintName, 1);
}
} else {
@@ -852,7 +853,7 @@ space_checks_expr_list(uint32_t space_id)
space = space_by_id(space_id);
assert(space != NULL);
assert(space->def != NULL);
- return space->def->opts.checks;
+ return space->def->opts.checks_ast;
}
int
@@ -1375,8 +1376,8 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
vdbe_emit_fkey_create(pParse, fk);
}
cleanup:
- sql_expr_list_delete(db, p->def->opts.checks);
- p->def->opts.checks = NULL;
+ sql_expr_list_delete(db, p->def->opts.checks_ast);
+ p->def->opts.checks_ast = NULL;
}
void
--
2.19.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check
2019-01-10 13:54 [tarantool-patches] [PATCH v1 0/4] sql: Checks on server side Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
@ 2019-01-10 13:54 ` Kirill Shcherbatov
2019-01-11 14:06 ` [tarantool-patches] " Konstantin Osipov
` (2 more replies)
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 3/4] box: exported sql_bind structure and API Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
3 siblings, 3 replies; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-01-10 13:54 UTC (permalink / raw)
To: tarantool-patches, korablev; +Cc: Kirill Shcherbatov
Due to the fact that we are going to perform CHECKs validations
on the server side, checks are performed on the fields with
affinity already applied that may differ with type of original
data.
After the introduction of static types, the need for type checks
based on the CHECKs disappeared.
Need for #3691
---
src/box/sql/build.c | 23 ++++++++++
test/sql-tap/check.test.lua | 89 ++-----------------------------------
2 files changed, 26 insertions(+), 86 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 7e5bcc518..9c6b1b49a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -756,12 +756,35 @@ primary_key_exit:
return;
}
+static int
+sql_check_expr_test_callback(struct Walker *walker, struct Expr *expr)
+{
+ if (expr->op == TK_FUNCTION && strcmp(expr->u.zToken, "TYPEOF") == 0) {
+ diag_set(ClientError, ER_SQL, "TYPEOF function is forbidden "
+ "for usage in Check constraint");
+ walker->eCode = 1;
+ return WRC_Abort;
+ }
+ return WRC_Continue;
+}
+
void
sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span)
{
struct Expr *expr = span->pExpr;
struct Table *table = parser->pNewTable;
if (table != NULL) {
+ /* Test if Check expression valid. */
+ struct Walker w;
+ memset(&w, 0, sizeof(w));
+ w.xExprCallback = sql_check_expr_test_callback;
+ sqlite3WalkExpr(&w, expr);
+ if (w.eCode != 0) {
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
+ goto release_expr;
+ }
+
expr->u.zToken =
sqlite3DbStrNDup(parser->db, (char *)span->zStart,
(int)(span->zEnd - span->zStart));
diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
index 1f369fb02..1176ad500 100755
--- a/test/sql-tap/check.test.lua
+++ b/test/sql-tap/check.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(58)
+test:plan(51)
--!./tcltestrunner.lua
-- 2005 November 2
@@ -201,7 +201,7 @@ test:do_execsql_test(
-- </check-1.17>
})
-test:do_execsql_test(
+test:do_catchsql_test(
"check-2.1",
[[
CREATE TABLE t2(
@@ -212,93 +212,10 @@ test:do_execsql_test(
);
]], {
-- <check-2.1>
-
+ 1, "SQL error: TYPEOF function is forbidden for usage in Check constraint"
-- </check-2.1>
})
-test:do_execsql_test(
- "check-2.2",
- [[
- INSERT INTO t2 VALUES(1, 1,2.2,'three');
- SELECT x, y, z FROM t2;
- ]], {
- -- <check-2.2>
- 1, 2.2, "three"
- -- </check-2.2>
- })
-
---db("close")
---sqlite3("db", "test.db")
-test:do_execsql_test(
- "check-2.3",
- [[
- INSERT INTO t2 VALUES(2, NULL, NULL, NULL);
- SELECT x, y, z FROM t2;
- ]], {
- -- <check-2.3>
- 1, 2.2, "three", "", "", ""
- -- </check-2.3>
- })
-
-test:do_catchsql_test(
- "check-2.4",
- [[
- INSERT INTO t2 VALUES(3, 1.1, NULL, NULL);
- ]], {
- -- <check-2.4>
- 1, "CHECK constraint failed: ONE"
- -- </check-2.4>
- })
-
-test:do_catchsql_test(
- "check-2.5",
- [[
- INSERT INTO t2 VALUES(4, NULL, 5, NULL);
- ]], {
- -- <check-2.5>
- 1, "CHECK constraint failed: TWO"
- -- </check-2.5>
- })
-
-test:do_catchsql_test(
- "check-2.6",
- [[
- INSERT INTO t2 VALUES(5, NULL, NULL, 3.14159);
- ]], {
- -- <check-2.6>
- 1, "CHECK constraint failed: THREE"
- -- </check-2.6>
- })
-
--- gh-3504: Check the CONSTRAINT name clause can't follow a constraint.
-
-test:do_catchsql_test(
- "check-2.10",
- [[
- CREATE TABLE t2b(
- x INTEGER CHECK( typeof(coalesce(x,0))=='integer' ) CONSTRAINT one,
- PRIMARY KEY (x)
- );
- ]], {
- -- <check-2.10>
- 1,"near \",\": syntax error"
- -- </check-2.10>
- })
-
-test:do_catchsql_test(
- "check-2.11",
- [[
- CREATE TABLE t2c(
- x INTEGER CONSTRAINT one CHECK( typeof(coalesce(x,0))=='integer' )
- CONSTRAINT two,
- PRIMARY KEY (x)
- );
- ]], {
- -- <check-2.10>
- 1,"near \",\": syntax error"
- -- </check-2.10>
- })
-
test:do_execsql_test(
"check-2.cleanup",
[[
--
2.19.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] [PATCH v1 3/4] box: exported sql_bind structure and API
2019-01-10 13:54 [tarantool-patches] [PATCH v1 0/4] sql: Checks on server side Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
@ 2019-01-10 13:54 ` Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
3 siblings, 0 replies; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-01-10 13:54 UTC (permalink / raw)
To: tarantool-patches, korablev; +Cc: Kirill Shcherbatov
We need exprort sql_bind structure, sql_bind_decode and
sql_bind_column routines to make SQL Vars bindings for Vdbe code
outside of execute module, preparing Checks stmt for execution.
Need for #3691
---
src/box/execute.c | 48 ++-----------------------------------------
src/box/execute.h | 52 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+), 46 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 7fff5fdff..9498b99be 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -57,31 +57,6 @@ const char *sql_info_key_strs[] = {
"row count",
};
-/**
- * Name and value of an SQL prepared statement parameter.
- * @todo: merge with sqlite3_value.
- */
-struct sql_bind {
- /** Bind name. NULL for ordinal binds. */
- const char *name;
- /** Length of the @name. */
- uint32_t name_len;
- /** Ordinal position of the bind, for ordinal binds. */
- uint32_t pos;
-
- /** Byte length of the value. */
- uint32_t bytes;
- /** SQL type of the value. */
- uint8_t type;
- /** Bind value. */
- union {
- double d;
- int64_t i64;
- /** For string or blob. */
- const char *s;
- };
-};
-
/**
* Return a string name of a parameter marker.
* @param Bind to get name.
@@ -96,17 +71,7 @@ sql_bind_name(const struct sql_bind *bind)
return tt_sprintf("%d", (int) bind->pos);
}
-/**
- * Decode a single bind column from the binary protocol packet.
- * @param[out] bind Bind to decode to.
- * @param i Ordinal bind number.
- * @param packet MessagePack encoded parameter value. Either
- * scalar or map: {string_name: scalar_value}.
- *
- * @retval 0 Success.
- * @retval -1 Memory or client error.
- */
-static inline int
+int
sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
{
bind->pos = i + 1;
@@ -363,16 +328,7 @@ error:
return -1;
}
-/**
- * Bind SQL parameter value to its position.
- * @param stmt Prepared statement.
- * @param p Parameter value.
- * @param pos Ordinal bind position.
- *
- * @retval 0 Success.
- * @retval -1 SQL error.
- */
-static inline int
+int
sql_bind_column(struct sqlite3_stmt *stmt, const struct sql_bind *p,
uint32_t pos)
{
diff --git a/src/box/execute.h b/src/box/execute.h
index 9c1bc4f05..e0b730407 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -51,6 +51,7 @@ extern const char *sql_info_key_strs[];
struct obuf;
struct region;
struct sql_bind;
+struct sqlite3_stmt;
/** Response on EXECUTE request. */
struct sql_response {
@@ -132,6 +133,57 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
uint32_t bind_count, struct sql_response *response,
struct region *region);
+/**
+ * Name and value of an SQL prepared statement parameter.
+ * @todo: merge with sqlite3_value.
+ */
+struct sql_bind {
+ /** Bind name. NULL for ordinal binds. */
+ const char *name;
+ /** Length of the @name. */
+ uint32_t name_len;
+ /** Ordinal position of the bind, for ordinal binds. */
+ uint32_t pos;
+
+ /** Byte length of the value. */
+ uint32_t bytes;
+ /** SQL type of the value. */
+ uint8_t type;
+ /** Bind value. */
+ union {
+ double d;
+ int64_t i64;
+ /** For string or blob. */
+ const char *s;
+ };
+};
+
+/**
+ * Decode a single bind column from the binary protocol packet.
+ * @param[out] bind Bind to decode to.
+ * @param i Ordinal bind number.
+ * @param packet MessagePack encoded parameter value. Either
+ * scalar or map: {string_name: scalar_value}.
+ *
+ * @retval 0 Success.
+ * @retval -1 Memory or client error.
+ */
+int
+sql_bind_decode(struct sql_bind *bind, int i, const char **packet);
+
+/**
+ * Bind SQL parameter value to its position.
+ * @param stmt Prepared statement.
+ * @param p Parameter value.
+ * @param pos Ordinal bind position.
+ *
+ * @retval 0 Success.
+ * @retval -1 SQL error.
+ */
+int
+sql_bind_column(struct sqlite3_stmt *stmt, const struct sql_bind *p,
+ uint32_t pos);
+
#if defined(__cplusplus)
} /* extern "C" { */
#endif
--
2.19.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side
2019-01-10 13:54 [tarantool-patches] [PATCH v1 0/4] sql: Checks on server side Kirill Shcherbatov
` (2 preceding siblings ...)
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 3/4] box: exported sql_bind structure and API Kirill Shcherbatov
@ 2019-01-10 13:54 ` Kirill Shcherbatov
2019-01-11 14:12 ` [tarantool-patches] " Konstantin Osipov
` (3 more replies)
3 siblings, 4 replies; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-01-10 13:54 UTC (permalink / raw)
To: tarantool-patches, korablev; +Cc: Kirill Shcherbatov
To make SQL CHECKS on space insertion in LUA we have introduced
a new sql_check class containing precompiled reusable VDBE making
required validations. It has been parameterized with binding
parameters to map a tuple to be inserted in VDBE memory and
to manage checks to be performed on UPDATE operation.
Closes #3691
---
src/box/space.c | 49 +++++++
src/box/space.h | 5 +
src/box/sql.c | 177 +++++++++++++++++++++++++-
src/box/sql.h | 66 ++++++++++
src/box/sql/build.c | 2 +-
src/box/sql/insert.c | 107 ++++------------
src/box/sql/sqliteInt.h | 32 +++++
src/box/sql/vdbe.c | 2 +-
src/box/sql/vdbeapi.c | 41 ++----
test/sql-tap/check.test.lua | 24 ++--
test/sql-tap/fkey2.test.lua | 4 +-
test/sql-tap/subquery.test.lua | 2 +-
test/sql-tap/table.test.lua | 8 +-
test/sql/checks.result | 51 ++++++++
test/sql/checks.test.lua | 18 +++
test/sql/gh-2981-check-autoinc.result | 8 +-
16 files changed, 461 insertions(+), 135 deletions(-)
diff --git a/src/box/space.c b/src/box/space.c
index 4d174f7ec..b139a0905 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -29,6 +29,7 @@
* SUCH DAMAGE.
*/
#include "space.h"
+#include "sql.h"
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
@@ -108,6 +109,33 @@ space_fill_index_map(struct space *space)
}
}
+/**
+ * SQL-specific actions for space.
+ */
+static void
+on_space_before_replace(struct trigger *trigger, void *event)
+{
+ uint32_t space_id = (uint32_t)(uintptr_t)trigger->data;
+ struct space *space = space_cache_find(space_id);
+ assert(space != NULL);
+ struct txn *txn = (struct txn *) event;
+ struct txn_stmt *stmt = txn_current_stmt(txn);
+ if (stmt == NULL)
+ return;
+ struct tuple *new_tuple = stmt->new_tuple;
+ struct tuple *old_tuple = stmt->old_tuple;
+ if (new_tuple != NULL && space->sql_checks != NULL) {
+ const char *new_tuple_raw = tuple_data(new_tuple);
+ const char *old_tuple_raw = old_tuple != NULL ?
+ tuple_data(old_tuple) : NULL;
+ if (sql_checks_run(space->sql_checks,
+ space->def->opts.checks_ast, space->def,
+ old_tuple_raw, new_tuple_raw) != 0) {
+ diag_raise();
+ }
+ }
+}
+
int
space_create(struct space *space, struct engine *engine,
const struct space_vtab *vtab, struct space_def *def,
@@ -165,6 +193,25 @@ space_create(struct space *space, struct engine *engine,
space_fill_index_map(space);
rlist_create(&space->parent_fkey);
rlist_create(&space->child_fkey);
+ if (def->opts.checks_ast != NULL) {
+ struct sql_checks *checks =
+ sql_checks_create(sql_get(), def->opts.checks_ast, def);
+ if (unlikely(checks == NULL))
+ goto fail_free_indexes;
+ space->sql_checks = checks;
+
+ struct trigger *sql_actions_trigger =
+ (struct trigger *)malloc(sizeof(struct trigger));
+ if (sql_actions_trigger == NULL) {
+ diag_set(OutOfMemory, sizeof(struct trigger),
+ "calloc", "on_space_before_replace");
+ goto fail_free_indexes;
+ }
+ trigger_create(sql_actions_trigger, on_space_before_replace,
+ (void *)(uintptr_t)space->def->id,
+ (trigger_f0)free);
+ trigger_add(&space->before_replace, sql_actions_trigger);
+ }
return 0;
fail_free_indexes:
@@ -216,6 +263,8 @@ space_delete(struct space *space)
trigger_destroy(&space->before_replace);
trigger_destroy(&space->on_replace);
trigger_destroy(&space->on_stmt_begin);
+ if (space->sql_checks != NULL)
+ sql_checks_destroy(space->sql_checks);
space_def_delete(space->def);
/*
* SQL Triggers should be deleted with
diff --git a/src/box/space.h b/src/box/space.h
index 7eb7ae292..e83609992 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -150,6 +150,11 @@ struct space {
struct rlist on_stmt_begin;
/** SQL Trigger list. */
struct sql_trigger *sql_triggers;
+ /**
+ * The SQL Checks to perform on INSERT, REPLACE and
+ * UPDATE operations.
+ */
+ struct sql_checks *sql_checks;
/**
* The number of *enabled* indexes in the space.
*
diff --git a/src/box/sql.c b/src/box/sql.c
index 26b84c5db..8961c4fd9 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -35,9 +35,10 @@
#include "sql/tarantoolInt.h"
#include "sql/vdbeInt.h"
#include "mpstream.h"
+#include "execute.h"
#include "index.h"
-#include <info.h>
+#include "info.h"
#include "schema.h"
#include "box.h"
#include "txn.h"
@@ -1377,3 +1378,177 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
sql_parser_destroy(&parser);
return rc;
}
+
+static int
+check_is_enabled_callback(struct Walker *w, Expr *expr)
+{
+ /* CHECK constraint uses a changing column. */
+ const int marker = 0x01;
+ if (expr->op == TK_COLUMN) {
+ assert(expr->iColumn >= 0 || expr->iColumn == -1);
+ if (expr->iColumn >= 0) {
+ if (w->u.aiCol[expr->iColumn] >= 0)
+ w->eCode |= marker;
+ }
+ }
+ return WRC_Continue;
+}
+
+/**
+ * Return true if this CHECK constraint can't be skipped when
+ * validating the new row in the UPDATE statement. In other
+ * words return false if CHECK constraint check does not use any of the
+ * changing columns.
+ * @param check CHECK constraint on a row that is being UPDATE-ed.
+ * The only columns that are modified by the UPDATE
+ * are those for which update_mask[i] >= 0.
+ * @param update_mask Array of items that were modified.
+ * 0 for unchanged field, -1 for modified.
+ * @retval 1 when specified check is required, 0 elsewhere.
+ */
+static bool
+sql_check_is_enabled(struct Expr *check, int *update_mask)
+{
+ struct Walker w;
+ memset(&w, 0, sizeof(w));
+ w.eCode = 0;
+ w.xExprCallback = check_is_enabled_callback;
+ w.u.aiCol = update_mask;
+ sqlite3WalkExpr(&w, check);
+ return w.eCode == 0 ? 1 : 0;
+}
+
+struct sql_checks *
+sql_checks_create(struct sqlite3 *db, struct ExprList *checks_ast,
+ struct space_def *def)
+{
+ assert(checks_ast != NULL);
+ struct sql_checks *checks = malloc(sizeof(struct sql_checks));
+ if (unlikely(checks == NULL)) {
+ diag_set(OutOfMemory, sizeof(struct sql_checks), "malloc",
+ "checks");
+ return NULL;
+ }
+
+ struct Parse parser;
+ sql_parser_create(&parser, db);
+ struct Vdbe *v = sqlite3GetVdbe(&parser);
+ if (unlikely(v == NULL)) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ free(checks);
+ return NULL;
+ }
+ /* Compile VDBE with default sql parameters. */
+ struct session *user_session = current_session();
+ uint32_t sql_flags = user_session->sql_flags;
+ user_session->sql_flags = default_sql_flags;
+
+ /* Make continuous registers allocation. */
+ uint32_t reg_count = def->field_count + checks_ast->nExpr;
+ int new_tuple_reg = sqlite3GetTempRange(&parser, reg_count);
+ int checks_state_reg = def->field_count + 1;
+ /*
+ * Generate a prologe code to bind variables new_tuple_var
+ * and checks_state_var to new_tuple_reg and
+ * checks_state_reg respectively.
+ */
+ struct Expr bind = {.op = TK_VARIABLE, .u.zToken = "?"};
+ checks->new_tuple_var = parser.nVar + 1;
+ checks->checks_state_var = checks->new_tuple_var + def->field_count;
+ for (uint32_t i = 0; i < reg_count; i++) {
+ sqlite3ExprAssignVarNumber(&parser, &bind, 1);
+ sqlite3ExprCodeTarget(&parser, &bind, new_tuple_reg + i);
+ }
+ /* Generate Checks code. */
+ vdbe_emit_checks_test(&parser, checks_ast, def, new_tuple_reg,
+ checks_state_reg);
+ sql_finish_coding(&parser);
+ sql_parser_destroy(&parser);
+
+ user_session->sql_flags = sql_flags;
+ checks->stmt = (struct sqlite3_stmt *)v;
+ return checks;
+}
+
+void
+sql_checks_destroy(struct sql_checks *checks)
+{
+ sqlite3_finalize(checks->stmt);
+ free(checks);
+}
+
+int
+sql_checks_run(const struct sql_checks *checks,
+ const struct ExprList *checks_ast, struct space_def *def,
+ const char *old_tuple_raw, const char *new_tuple_raw)
+{
+ assert(new_tuple_raw != NULL);
+ uint32_t field_count = def->field_count;
+ struct sqlite3_stmt *stmt = checks->stmt;
+ struct region *region = &fiber()->gc;
+ int *update_mask = NULL;
+ if (old_tuple_raw != NULL) {
+ /* The update_mask is required only for UPDATE. */
+ uint32_t update_mask_sz = sizeof(int) * field_count;
+ update_mask = region_alloc(region, update_mask_sz);
+ if (unlikely(update_mask == NULL)) {
+ diag_set(OutOfMemory, update_mask_sz, "region_alloc",
+ "update_mask");
+ return -1;
+ }
+ }
+ /*
+ * Prepare parameters for checks->stmt execution:
+ * - Unpacked new tuple fields mapped to Vdbe memory from
+ * variables from range:
+ * [new_tuple_var,new_tuple_var+field_count]
+ * - Sequence of Checks status parameters(is Check with
+ * index i enabled) mapped to Vdbe memory
+ * [checks_state_var,checks_state_var+checks_ast->nExpr]
+ * Items are calculated with sql_check_is_enabled
+ * routine called with update_mask for UPDATE operation.
+ * In case of regular INSERT, REPLACE operation all
+ * Checks should be called so all checks_state_reg items
+ * are 1.
+ */
+ mp_decode_array(&new_tuple_raw);
+ if (old_tuple_raw != NULL)
+ mp_decode_array(&old_tuple_raw);
+ /* Reset VDBE to make new bindings. */
+ sql_stmt_reset(stmt);
+ for (uint32_t i = 0; i < field_count; i++) {
+ const char *new_tuple_field = new_tuple_raw;
+ struct sql_bind bind;
+ if (sql_bind_decode(&bind, checks->new_tuple_var + i,
+ &new_tuple_raw) != 0)
+ return -1;
+ if (sql_bind_column(stmt, &bind,
+ checks->new_tuple_var + i) != 0)
+ return -1;
+ /* Skip calculations in case of INSERT. */
+ if (update_mask == NULL)
+ continue;
+ /* Form the tuple update mask. */
+ const char *old_tuple_field = old_tuple_raw;
+ mp_next(&old_tuple_raw);
+ update_mask[i] = (old_tuple_raw - old_tuple_field !=
+ new_tuple_raw - new_tuple_field ||
+ memcmp(new_tuple_field, old_tuple_field,
+ old_tuple_raw -
+ old_tuple_field) != 0) ? -1 : 0;
+ }
+ /* Map Checks "is enabled" paremeters. */
+ for (int i = 0; i < checks_ast->nExpr; i++) {
+ struct Expr *expr = checks_ast->a[i].pExpr;
+ int val = update_mask != NULL ?
+ sql_check_is_enabled(expr, update_mask) : 1;
+ sqlite3_bind_int64(stmt, checks->checks_state_var + i, val);
+ }
+ /* Checks VDBE can't expire, reset expired flag & Burn. */
+ struct Vdbe *v = (struct Vdbe *)stmt;
+ v->expired = 0;
+ int rc = sqlite3_step(stmt) == SQLITE_DONE ? 0 : -1;
+ if (rc != 0)
+ diag_set(ClientError, ER_SQL_EXECUTE, v->zErrMsg);
+ return rc;
+}
diff --git a/src/box/sql.h b/src/box/sql.h
index 028a15245..f2a6587da 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -68,6 +68,29 @@ struct Select;
struct Table;
struct sql_trigger;
+/** SQL Checks object. */
+struct sql_checks {
+ /**
+ * The first in an array variables representing the
+ * new tuple to be inserted.
+ */
+ int new_tuple_var;
+ /**
+ * The first in array of variables representing the state
+ * (1 for ON and 0 for OFF) of corresponding Check. This
+ * is the UPDATE optimization - disabled checks would be
+ * skipped.
+ */
+ int checks_state_var;
+ /**
+ * Precompiled reusable VDBE program for SQL Checks
+ * raising error on Check constraint failure. VDBE
+ * program assumes new_tuple and checks_state actual
+ * parameters to be placed in VDBE memory before run.
+ */
+ struct sqlite3_stmt *stmt;
+};
+
/**
* Perform parsing of provided expression. This is done by
* surrounding the expression w/ 'SELECT ' prefix and perform
@@ -411,6 +434,49 @@ void
sqlite3SrcListDelete(struct sqlite3 *db, struct SrcList *list);
+/**
+ * Create sql_checks object by Checks AST for specified space.
+ * @param db Database handler.
+ * @param checks_ast Checks AST expression list. Checks space def
+ * references may be modified during
+ * construction.
+ * @param def Space definition that checks_ast should refer to.
+ * @retval NULL on error.
+ * @retval not NULL SQL Checks object pointer on success.
+*/
+struct sql_checks *
+sql_checks_create(struct sqlite3 *db, struct ExprList *checks_ast,
+ struct space_def *def);
+
+/**
+ * Destroy SQL Checks object, release related resources.
+ * @param checks SQL Checks object to destroy.
+ */
+void
+sql_checks_destroy(struct sql_checks *checks);
+
+/**
+ * Run SQL Checks VDBE with parameters constructed by Checks AST,
+ * Space definition and alter tuples. Checks AST and Space
+ * definition must match be the same that were used to create the
+ * SQL Checks object.
+ * @param checks SQL Checks object containing VDBE to execute.
+ * @param checks_ast Checks AST object to determine Checks that
+ * may be omitted on execution (to prepare
+ * checks_state parameter).
+ * @param def Space definition describing alter tuples format.
+ * @param old_tuple_raw Old tuple to be replaced in UPDATE operation.
+ * (NULL for INSERT)
+ * @param new_tuple_raw New tuple to be inserted.
+ * @retval 0 on all required Checks were passed.
+ * @retval -1 on system error or Check constrints failure
+ * indicating an diag_msg to be raised.
+ */
+int
+sql_checks_run(const struct sql_checks *checks,
+ const struct ExprList *checks_ast, struct space_def *def,
+ const char *old_tuple_raw, const char *new_tuple_raw);
+
#if defined(__cplusplus)
} /* extern "C" { */
#endif
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 9c6b1b49a..40675fd32 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -873,7 +873,7 @@ struct ExprList *
space_checks_expr_list(uint32_t space_id)
{
struct space *space;
- space = space_by_id(space_id);
+ space = space_cache_find(space_id);
assert(space != NULL);
assert(space->def != NULL);
return space->def->opts.checks_ast;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index f147f6a50..ab0e12def 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -813,51 +813,36 @@ sqlite3Insert(Parse * pParse, /* Parser context */
sqlite3DbFree(db, aRegIdx);
}
-/*
- * Meanings of bits in of pWalker->eCode for checkConstraintUnchanged()
- */
-#define CKCNSTRNT_COLUMN 0x01 /* CHECK constraint uses a changing column */
-
-/* This is the Walker callback from checkConstraintUnchanged(). Set
- * bit 0x01 of pWalker->eCode if
- * pWalker->eCode to 0 if this expression node references any of the
- * columns that are being modifed by an UPDATE statement.
- */
-static int
-checkConstraintExprNode(Walker * pWalker, Expr * pExpr)
+void
+vdbe_emit_checks_test(struct Parse *parser, struct ExprList *checks_ast,
+ struct space_def *def, int new_tuple_reg,
+ int checks_state_reg)
{
- if (pExpr->op == TK_COLUMN) {
- assert(pExpr->iColumn >= 0 || pExpr->iColumn == -1);
- if (pExpr->iColumn >= 0) {
- if (pWalker->u.aiCol[pExpr->iColumn] >= 0) {
- pWalker->eCode |= CKCNSTRNT_COLUMN;
- }
- }
+ struct Vdbe *v = sqlite3GetVdbe(parser);
+ assert(checks_ast != NULL);
+ int check_state_off = sqlite3GetTempReg(parser);
+ sqlite3VdbeAddOp2(v, OP_Integer, 0, check_state_off);
+ /*
+ * For CHECK constraint and for INSERT/UPDATE conflict
+ * action DEFAULT and ABORT in fact has the same meaning.
+ */
+ parser->ckBase = new_tuple_reg;
+ for (int i = 0; i < checks_ast->nExpr; i++) {
+ struct Expr *expr = checks_ast->a[i].pExpr;
+ int all_ok = sqlite3VdbeMakeLabel(v);
+ /* Skip check when it turned off. */
+ sqlite3VdbeAddOp3(v, OP_Eq, checks_state_reg + i, all_ok,
+ check_state_off);
+ sqlite3ExprIfTrue(parser, expr, all_ok, SQLITE_JUMPIFNULL);
+ char *name = checks_ast->a[i].zName;
+ if (name == NULL)
+ name = def->name;
+ sqlite3HaltConstraint(parser, SQLITE_CONSTRAINT_CHECK,
+ ON_CONFLICT_ACTION_ABORT, name,
+ P4_TRANSIENT, P5_ConstraintCheck);
+ sqlite3VdbeResolveLabel(v, all_ok);
}
- return WRC_Continue;
-}
-
-/*
- * pExpr is a CHECK constraint on a row that is being UPDATE-ed. The
- * only columns that are modified by the UPDATE are those for which
- * aiChng[i]>=0.
- *
- * Return true if CHECK constraint pExpr does not use any of the
- * changing columns. In other words, return true if this CHECK constraint
- * can be skipped when validating the new row in the UPDATE statement.
- */
-static int
-checkConstraintUnchanged(Expr * pExpr, int *aiChng)
-{
- Walker w;
- memset(&w, 0, sizeof(w));
- w.eCode = 0;
- w.xExprCallback = checkConstraintExprNode;
- w.u.aiCol = aiChng;
- sqlite3WalkExpr(&w, pExpr);
- testcase(w.eCode == 0);
- testcase(w.eCode == CKCNSTRNT_COLUMN);
- return !w.eCode;
+ sqlite3ReleaseTempReg(parser, check_state_off);
}
void
@@ -928,42 +913,6 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
unreachable();
}
}
- /*
- * For CHECK constraint and for INSERT/UPDATE conflict
- * action DEFAULT and ABORT in fact has the same meaning.
- */
- if (on_conflict == ON_CONFLICT_ACTION_DEFAULT)
- on_conflict = ON_CONFLICT_ACTION_ABORT;
- /* Test all CHECK constraints. */
- struct ExprList *checks = space_checks_expr_list(def->id);
- enum on_conflict_action on_conflict_check = on_conflict;
- if (on_conflict == ON_CONFLICT_ACTION_REPLACE)
- on_conflict_check = ON_CONFLICT_ACTION_ABORT;
- if (checks != NULL) {
- parse_context->ckBase = new_tuple_reg;
- for (int i = 0; i < checks->nExpr; i++) {
- struct Expr *expr = checks->a[i].pExpr;
- if (is_update &&
- checkConstraintUnchanged(expr, upd_cols))
- continue;
- int all_ok = sqlite3VdbeMakeLabel(v);
- sqlite3ExprIfTrue(parse_context, expr, all_ok,
- SQLITE_JUMPIFNULL);
- if (on_conflict == ON_CONFLICT_ACTION_IGNORE) {
- sqlite3VdbeGoto(v, ignore_label);
- } else {
- char *name = checks->a[i].zName;
- if (name == NULL)
- name = def->name;
- sqlite3HaltConstraint(parse_context,
- SQLITE_CONSTRAINT_CHECK,
- on_conflict_check, name,
- P4_TRANSIENT,
- P5_ConstraintCheck);
- }
- sqlite3VdbeResolveLabel(v, all_ok);
- }
- }
sql_emit_table_affinity(v, tab->def, new_tuple_reg);
/*
* Other actions except for REPLACE and UPDATE OR IGNORE
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 7e16edc9a..5a11d9d13 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -594,6 +594,19 @@ sqlite3_column_value(sqlite3_stmt *,
int
sqlite3_finalize(sqlite3_stmt * pStmt);
+/*
+ * Terminate the current execution of an SQL statement and reset
+ * it back to its starting state so that it can be reused. A
+ * success code from the prior execution is returned.
+ *
+ * This routine sets the error code and string returned by
+ * sqlite3_errcode(), sqlite3_errmsg() and sqlite3_errmsg16().
+ * @param stmt VDBE program, may be NULL.
+ * @retval SQLITE_OK on success, sql_ret_code error code.
+ */
+int
+sql_stmt_reset(struct sqlite3_stmt *stmt);
+
int
sqlite3_exec(sqlite3 *, /* An open database */
const char *sql, /* SQL to be evaluated */
@@ -3869,6 +3882,25 @@ vdbe_emit_constraint_checks(struct Parse *parse_context,
struct Table *tab, int new_tuple_reg,
enum on_conflict_action on_conflict,
int ignore_label, int *upd_cols);
+/**
+ * This routine generates code to make Check constraints
+ * validations on tuple insertion during INSERT, REPLACE or
+ * UPDATE operation.
+ *
+ * @param parser Current parsing context.
+ * @param checks_ast Checks AST Expression list.
+ * @param def Destination space definition.
+ * @param new_tuple_reg The first register of group containing
+ * fields of tuple to be inserted.
+ * @param checks_state_reg The first register of group contining
+ * enabled state of corresponding checks:
+ * 0 for disabled check to be skipped,
+ * 1 - for enabled.
+ */
+void
+vdbe_emit_checks_test(struct Parse *parser, struct ExprList *checks_ast,
+ struct space_def *def, int new_tuple_reg,
+ int checks_state_reg);
/**
* This routine generates code to finish the INSERT or UPDATE
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9f9d0eacf..74a2e366c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -992,7 +992,7 @@ case OP_HaltIfNull: { /* in3 */
* automatically.
*
* P1 is the result code returned by sqlite3_exec(),
- * sqlite3_reset(), or sqlite3_finalize(). For a normal halt,
+ * sql_stmt_reset(), or sqlite3_finalize(). For a normal halt,
* this should be SQLITE_OK (0).
* For errors, it can be some other value. If P1!=0 then P2 will
* determine whether or not to rollback the current transaction.
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 9e57af051..b62de9d39 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -132,23 +132,15 @@ sqlite3_finalize(sqlite3_stmt * pStmt)
return rc;
}
-/*
- * Terminate the current execution of an SQL statement and reset it
- * back to its starting state so that it can be reused. A success code from
- * the prior execution is returned.
- *
- * This routine sets the error code and string returned by
- * sqlite3_errcode(), sqlite3_errmsg() and sqlite3_errmsg16().
- */
int
-sqlite3_reset(sqlite3_stmt * pStmt)
+sql_stmt_reset(struct sqlite3_stmt *stmt)
{
int rc;
- if (pStmt == 0) {
+ if (stmt == NULL) {
rc = SQLITE_OK;
} else {
- Vdbe *v = (Vdbe *) pStmt;
- sqlite3 *db = v->db;
+ struct Vdbe *v = (struct Vdbe *)stmt;
+ struct sqlite3 *db = v->db;
checkProfileCallback(db, v);
rc = sqlite3VdbeReset(v);
sqlite3VdbeRewind(v);
@@ -509,30 +501,19 @@ sqlite3Step(Vdbe * p)
assert(p);
if (p->magic != VDBE_MAGIC_RUN) {
- /* We used to require that sqlite3_reset() be called before retrying
- * sqlite3_step() after any error or after SQLITE_DONE. But beginning
- * with version 3.7.0, we changed this so that sqlite3_reset() would
- * be called automatically instead of throwing the SQLITE_MISUSE error.
- * This "automatic-reset" change is not technically an incompatibility,
- * since any application that receives an SQLITE_MISUSE is broken by
- * definition.
- *
- * Nevertheless, some published applications that were originally written
- * for version 3.6.23 or earlier do in fact depend on SQLITE_MISUSE
- * returns, and those were broken by the automatic-reset change. As a
- * a work-around, the SQLITE_OMIT_AUTORESET compile-time restores the
- * legacy behavior of returning SQLITE_MISUSE for cases where the
- * previous sqlite3_step() returned something other than a SQLITE_LOCKED
- * or SQLITE_BUSY error.
+ /*
+ * The sql_stmt_reset() routine would be called
+ * automatically instead of throwing the
+ * SQLITE_MISUSE error.
*/
#ifdef SQLITE_OMIT_AUTORESET
if ((rc = p->rc & 0xff) == SQLITE_BUSY || rc == SQLITE_LOCKED) {
- sqlite3_reset((sqlite3_stmt *) p);
+ sql_stmt_reset((sqlite3_stmt *) p);
} else {
return SQLITE_MISUSE_BKPT;
}
#else
- sqlite3_reset((sqlite3_stmt *) p);
+ sql_stmt_reset((sqlite3_stmt *) p);
#endif
}
@@ -632,7 +613,7 @@ sqlite3_step(sqlite3_stmt * pStmt)
rc2 = rc = sqlite3Reprepare(v);
if (rc != SQLITE_OK)
break;
- sqlite3_reset(pStmt);
+ sql_stmt_reset(pStmt);
if (savedPc >= 0)
v->doingRerun = 1;
assert(v->expired == 0);
diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
index 1176ad500..08b7edcc7 100755
--- a/test/sql-tap/check.test.lua
+++ b/test/sql-tap/check.test.lua
@@ -55,7 +55,7 @@ test:do_catchsql_test(
INSERT INTO t1 VALUES(6,7, 2);
]], {
-- <check-1.3>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-1.3>
})
@@ -75,7 +75,7 @@ test:do_catchsql_test(
INSERT INTO t1 VALUES(4,3, 2);
]], {
-- <check-1.5>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-1.5>
})
@@ -147,7 +147,7 @@ test:do_catchsql_test(
UPDATE t1 SET x=7 WHERE x==2
]], {
-- <check-1.12>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-1.12>
})
@@ -167,7 +167,7 @@ test:do_catchsql_test(
UPDATE t1 SET x=5 WHERE x==2
]], {
-- <check-1.14>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-1.14>
})
@@ -330,7 +330,7 @@ test:do_catchsql_test(
INSERT INTO t3 VALUES(111,222,333);
]], {
-- <check-3.9>
- 1, "CHECK constraint failed: T3"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T3"
-- </check-3.9>
})
@@ -401,7 +401,7 @@ test:do_catchsql_test(
UPDATE t4 SET x=0, y=1;
]], {
-- <check-4.6>
- 1, "CHECK constraint failed: T4"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T4"
-- </check-4.6>
})
@@ -421,7 +421,7 @@ test:do_catchsql_test(
UPDATE t4 SET x=0, y=2;
]], {
-- <check-4.9>
- 1, "CHECK constraint failed: T4"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T4"
-- </check-4.9>
})
@@ -498,7 +498,7 @@ test:do_catchsql_test(
UPDATE OR FAIL t1 SET x=7-x, y=y+1;
]], {
-- <check-6.5>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-6.5>
})
@@ -520,7 +520,7 @@ test:do_catchsql_test(
INSERT OR ROLLBACK INTO t1 VALUES(8,40.0, 10);
]], {
-- <check-6.7>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-6.7>
})
@@ -553,7 +553,7 @@ test:do_catchsql_test(
REPLACE INTO t1 VALUES(6,7, 11);
]], {
-- <check-6.12>
- 1, "CHECK constraint failed: T1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T1"
-- </check-6.12>
})
@@ -617,7 +617,7 @@ test:do_catchsql_test(
7.3,
" INSERT INTO t6 VALUES(11) ", {
-- <7.3>
- 1, "CHECK constraint failed: T6"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T6"
-- </7.3>
})
@@ -672,7 +672,7 @@ test:do_test(
return test:catchsql(" INSERT INTO t6 VALUES(12) ", "db2")
end, {
-- <7.8>
- 1, "CHECK constraint failed: T6"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T6"
-- </7.8>
})
end
diff --git a/test/sql-tap/fkey2.test.lua b/test/sql-tap/fkey2.test.lua
index 03bf025f3..1a22b5b55 100755
--- a/test/sql-tap/fkey2.test.lua
+++ b/test/sql-tap/fkey2.test.lua
@@ -362,7 +362,7 @@ test:do_catchsql_test(
UPDATE ab SET a = 5;
]], {
-- <fkey2-3.2>
- 1, "CHECK constraint failed: EF"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: EF"
-- </fkey2-3.2>
})
@@ -382,7 +382,7 @@ test:do_catchsql_test(
UPDATE ab SET a = 5;
]], {
-- <fkey2-3.4>
- 1, "CHECK constraint failed: EF"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: EF"
-- </fkey2-3.4>
})
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index fb9a737d1..38988dbdc 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -651,7 +651,7 @@ test:do_execsql_test(
--------------------------------------------------------------------
-- These tests - subquery-4.* - use the TCL statement cache to try
-- and expose bugs to do with re-using statements that have been
--- passed to sqlite3_reset().
+-- passed to sql_stmt_reset().
--
-- One problem was that VDBE memory cells were not being initialized
-- to NULL on the second and subsequent executions.
diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 7057f6b0f..43f8fc4cb 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -1217,7 +1217,7 @@ test:do_catchsql_test(
INSERT INTO T21 VALUES(1, -1, 1);
]], {
-- <table-21.3>
- 1, "CHECK constraint failed: T21"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T21"
-- </table-21.3>
})
@@ -1227,7 +1227,7 @@ test:do_catchsql_test(
INSERT INTO T21 VALUES(1, 1, -1);
]], {
-- <table-21.4>
- 1, "CHECK constraint failed: T21"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: T21"
-- </table-21.4>
})
@@ -1368,7 +1368,7 @@ test:do_catchsql_test(
INSERT INTO T28 VALUES(0);
]], {
-- <table-22.10>
- 1, "CHECK constraint failed: CHECK1"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: CHECK1"
-- </table-22.10>
})
@@ -1378,7 +1378,7 @@ test:do_catchsql_test(
INSERT INTO T28 VALUES(9);
]], {
-- <table-22.11>
- 1, "CHECK constraint failed: CHECK2"
+ 1, "Failed to execute SQL statement: CHECK constraint failed: CHECK2"
-- </table-22.11>
})
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 12a3aa14c..2078ef111 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -139,6 +139,57 @@ s = box.space._space:insert(t)
- error: 'Wrong space options (field 5): invalid expression specified (SQL error:
bindings are not allowed in DDL)'
...
+--
+-- gh-3691: Do SQL checks on server side
+--
+box.sql.execute("CREATE TABLE t1(x INTEGER CONSTRAINT ONE CHECK( x<5 ), y REAL CONSTRAINT TWO CHECK( y>x ), z INTEGER PRIMARY KEY);")
+---
+...
+box.space.T1:insert({7, 1, 1})
+---
+- error: 'Failed to execute SQL statement: CHECK constraint failed: ONE'
+...
+box.space.T1:insert({2, 1, 1})
+---
+- error: 'Failed to execute SQL statement: CHECK constraint failed: TWO'
+...
+box.space.T1:insert({2, 4, 1})
+---
+- [2, 4, 1]
+...
+box.space.T1:update({1}, {{"=", 1, 7}})
+---
+- error: 'Failed to execute SQL statement: CHECK constraint failed: ONE'
+...
+box.sql.execute("DROP TABLE t1");
+---
+...
+opts = {checks = {{expr ='X > 5', name = 'ONE'}}}
+---
+...
+format = {{name = 'X', type = 'unsigned'}}
+---
+...
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+---
+...
+s = box.space._space:insert(t)
+---
+...
+_ = box.space.test:create_index('pk')
+---
+...
+box.space.test:insert({1})
+---
+- error: 'Failed to execute SQL statement: CHECK constraint failed: ONE'
+...
+box.space.test:insert({6})
+---
+- [6]
+...
+box.space.test:drop()
+---
+...
test_run:cmd("clear filter")
---
- true
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index 0582bbb63..5fb6f65dc 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -60,5 +60,23 @@ format = {{name = 'X', type = 'unsigned'}}
t = {513, 1, 'test', 'memtx', 0, opts, format}
s = box.space._space:insert(t)
+--
+-- gh-3691: Do SQL checks on server side
+--
+box.sql.execute("CREATE TABLE t1(x INTEGER CONSTRAINT ONE CHECK( x<5 ), y REAL CONSTRAINT TWO CHECK( y>x ), z INTEGER PRIMARY KEY);")
+box.space.T1:insert({7, 1, 1})
+box.space.T1:insert({2, 1, 1})
+box.space.T1:insert({2, 4, 1})
+box.space.T1:update({1}, {{"=", 1, 7}})
+box.sql.execute("DROP TABLE t1");
+
+opts = {checks = {{expr ='X > 5', name = 'ONE'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+_ = box.space.test:create_index('pk')
+box.space.test:insert({1})
+box.space.test:insert({6})
+box.space.test:drop()
test_run:cmd("clear filter")
diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result
index b0f55e61d..06b79e14f 100644
--- a/test/sql/gh-2981-check-autoinc.result
+++ b/test/sql/gh-2981-check-autoinc.result
@@ -24,28 +24,28 @@ box.sql.execute("insert into t1 values (18, null);")
...
box.sql.execute("insert into t1(s2) values (null);")
---
-- error: 'CHECK constraint failed: T1'
+- error: 'Failed to execute SQL statement: CHECK constraint failed: T1'
...
box.sql.execute("insert into t2 values (18, null);")
---
...
box.sql.execute("insert into t2(s2) values (null);")
---
-- error: 'CHECK constraint failed: T2'
+- error: 'Failed to execute SQL statement: CHECK constraint failed: T2'
...
box.sql.execute("insert into t2 values (24, null);")
---
...
box.sql.execute("insert into t2(s2) values (null);")
---
-- error: 'CHECK constraint failed: T2'
+- error: 'Failed to execute SQL statement: CHECK constraint failed: T2'
...
box.sql.execute("insert into t3 values (9, null)")
---
...
box.sql.execute("insert into t3(s2) values (null)")
---
-- error: 'CHECK constraint failed: T3'
+- error: 'Failed to execute SQL statement: CHECK constraint failed: T3'
...
box.sql.execute("DROP TABLE t1")
---
--
2.19.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/4] box: rename space->opts checks to checks_ast
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
@ 2019-01-11 14:05 ` Konstantin Osipov
2019-01-18 18:00 ` Konstantin Osipov
1 sibling, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-11 14:05 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> + /** Checks AST expressions list. */
> + struct ExprList *checks_ast;
I don't understand the comment. Is it a list of syntax trees or a
syntax tree containing all checks? You can't use plural in an
adjective. This is a systematic error which I have been pointing
out before.
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1048,7 +1048,7 @@ sql_encode_table_opts(struct region *region, struct Table *table,
> int checks_cnt = 0;
> struct ExprList_item *a;
> bool is_view = table->def->opts.is_view;
> - struct ExprList *checks = table->def->opts.checks;
> + struct ExprList *checks = table->def->opts.checks_ast;
It's good manners to consistently rename stack variables when
renaming the corresponding member.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 2/4] sql: disallow use of TYPEOF in Check
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
@ 2019-01-11 14:06 ` Konstantin Osipov
2019-01-11 14:07 ` Konstantin Osipov
2019-01-18 18:04 ` Konstantin Osipov
2 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-11 14:06 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> Due to the fact that we are going to perform CHECKs validations
> on the server side, checks are performed on the fields with
> affinity already applied that may differ with type of original
> data.
> After the introduction of static types, the need for type checks
> based on the CHECKs disappeared.
SQL is server side as well. I would say that we're changing the
timing when check constraint validation is invoked: it is now
invoked inside a space trigger, rather than in vdbe.
>
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 2/4] sql: disallow use of TYPEOF in Check
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
2019-01-11 14:06 ` [tarantool-patches] " Konstantin Osipov
@ 2019-01-11 14:07 ` Konstantin Osipov
2019-01-18 18:04 ` Konstantin Osipov
2 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-11 14:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> Due to the fact that we are going to perform CHECKs validations
> on the server side, checks are performed on the fields with
> affinity already applied that may differ with type of original
> data.
> After the introduction of static types, the need for type checks
> based on the CHECKs disappeared.
>
Why are you silently removing the tests?
> +++ b/test/sql-tap/check.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(58)
> +test:plan(51)
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
@ 2019-01-11 14:12 ` Konstantin Osipov
2019-01-11 14:14 ` Konstantin Osipov
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-11 14:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> + if (sql_checks_run(space->sql_checks,
> + space->def->opts.checks_ast, space->def,
> + old_tuple_raw, new_tuple_raw) != 0) {
> + diag_raise();
Do you have a module named sql_checks? I believe you don't, one
reason is that we don't use plural for module names. Then
your method name should be sql_run_checks. But since this name is
meaningless, why not use a straightforward and self-explanatory
name, for example sql_validate_check_constraints()?
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
2019-01-11 14:12 ` [tarantool-patches] " Konstantin Osipov
@ 2019-01-11 14:14 ` Konstantin Osipov
2019-01-18 18:11 ` Konstantin Osipov
2019-01-21 14:47 ` n.pettik
3 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-11 14:14 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> To make SQL CHECKS on space insertion in LUA we have introduced
> a new sql_check class containing precompiled reusable VDBE making
> required validations. It has been parameterized with binding
> parameters to map a tuple to be inserted in VDBE memory and
> to manage checks to be performed on UPDATE operation.
What happens when I disable triggers on a space? Does this disable
check constraints as well?
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/4] box: rename space->opts checks to checks_ast
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
2019-01-11 14:05 ` [tarantool-patches] " Konstantin Osipov
@ 2019-01-18 18:00 ` Konstantin Osipov
1 sibling, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-18 18:00 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> The checks field of space_opts object renamed to checks_ast to avoid a
> confusion due to introduction of a new object sql_check, representing
> precompiled VDBE program to execute.
Please use name check_constraint_expr
A common abbreviation for foreign key constraint is fk, check
constarint - ck, not null - nn, so an alternative family of names
would be fk_constraint, ck_constraint, nn_constraint. Feel free to
use it if you like a bit shorter form.
> + if (def->opts.checks_ast != NULL &&
> + sql_checks_resolve_space_def_reference(def->opts.checks_ast,
Please name the module 'check_constraint' or ck_constraint, and rename all the
functions accordingly.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 2/4] sql: disallow use of TYPEOF in Check
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
2019-01-11 14:06 ` [tarantool-patches] " Konstantin Osipov
2019-01-11 14:07 ` Konstantin Osipov
@ 2019-01-18 18:04 ` Konstantin Osipov
2 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-18 18:04 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> Due to the fact that we are going to perform CHECKs validations
> on the server side, checks are performed on the fields with
> affinity already applied that may differ with type of original
> data.
> After the introduction of static types, the need for type checks
> based on the CHECKs disappeared.
>
> Need for #3691
> ---
> src/box/sql/build.c | 23 ++++++++++
> test/sql-tap/check.test.lua | 89 ++-----------------------------------
> 2 files changed, 26 insertions(+), 86 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 7e5bcc518..9c6b1b49a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -756,12 +756,35 @@ primary_key_exit:
> return;
> }
>
> +static int
> +sql_check_expr_test_callback(struct Walker *walker, struct Expr *expr)
> +{
> + if (expr->op == TK_FUNCTION && strcmp(expr->u.zToken, "TYPEOF") == 0) {
> + diag_set(ClientError, ER_SQL, "TYPEOF function is forbidden "
> + "for usage in Check constraint");
> + walker->eCode = 1;
> + return WRC_Abort;
> + }
> + return WRC_Continue;
> +}
This callback needs a comment. All functions need a comment, even
static ones. A commonly accepted abbreviation for callbacks is _cb
ending.
A better option for function name would be ck_constraint_walk_expr_cb.
Please reply to my questions in previous emails.
> void
> sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span)
> {
> struct Expr *expr = span->pExpr;
> struct Table *table = parser->pNewTable;
> if (table != NULL) {
> + /* Test if Check expression valid. */
is valid. valid is a general term. What problems specifically are
you looking for? What is forbidden in a check expression? please
add a better comment.
> + struct Walker w;
> + memset(&w, 0, sizeof(w));
> + w.xExprCallback = sql_check_expr_test_callback;
> + sqlite3WalkExpr(&w, expr);
> + if (w.eCode != 0) {
> + parser->rc = SQL_TARANTOOL_ERROR;
> + parser->nErr++;
> + goto release_expr;
> + }
> +
I don't understand the premise for this code. Why do yo uneed a
separate walker, aren't these expressions checked already
someplace else in the code?
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
2019-01-11 14:12 ` [tarantool-patches] " Konstantin Osipov
2019-01-11 14:14 ` Konstantin Osipov
@ 2019-01-18 18:11 ` Konstantin Osipov
2019-01-21 14:47 ` n.pettik
3 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-01-18 18:11 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/01/10 23:12]:
> diff --git a/src/box/space.c b/src/box/space.c
> index 4d174f7ec..b139a0905 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -29,6 +29,7 @@
> * SUCH DAMAGE.
> */
> #include "space.h"
> +#include "sql.h"
Just STOP. Please read the SOP. You can't add an arbitrary header
to another file just because you happen to need it. What the hell
is going on here, why do you need to add entire sql layer to
space.c?
> +/**
> + * SQL-specific actions for space.
> + */
> +static void
> +on_space_before_replace(struct trigger *trigger, void *event)
> +{
> + uint32_t space_id = (uint32_t)(uintptr_t)trigger->data;
> + struct space *space = space_cache_find(space_id);
> + assert(space != NULL);
> + struct txn *txn = (struct txn *) event;
> + struct txn_stmt *stmt = txn_current_stmt(txn);
> + if (stmt == NULL)
> + return;
> + struct tuple *new_tuple = stmt->new_tuple;
> + struct tuple *old_tuple = stmt->old_tuple;
> + if (new_tuple != NULL && space->sql_checks != NULL) {
> + const char *new_tuple_raw = tuple_data(new_tuple);
> + const char *old_tuple_raw = old_tuple != NULL ?
> + tuple_data(old_tuple) : NULL;
> + if (sql_checks_run(space->sql_checks,
> + space->def->opts.checks_ast, space->def,
> + old_tuple_raw, new_tuple_raw) != 0) {
> + diag_raise();
> + }
> + }
> +}
Why couldn't you define this function wherever you like? Please
read the sop carefully about introducing new header file dependencies.
> + if (def->opts.checks_ast != NULL) {
> + struct sql_checks *checks =
> + sql_checks_create(sql_get(), def->opts.checks_ast, def);
> + if (unlikely(checks == NULL))
> + goto fail_free_indexes;
> + space->sql_checks = checks;
> +
> + struct trigger *sql_actions_trigger =
> + (struct trigger *)malloc(sizeof(struct trigger));
Please don't use plural. Why do you call it a different name? Why
not ck_constraint_trigger? Even if it does an action, well, action
is part of a check, so why invent a separate name for it?
> + * UPDATE operations.
> + */
> + struct sql_checks *sql_checks;
struct ck_constraint
> index 26b84c5db..8961c4fd9 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -35,9 +35,10 @@
> #include "sql/tarantoolInt.h"
> #include "sql/vdbeInt.h"
> #include "mpstream.h"
> +#include "execute.h"
You're not writing PHP! Please watch your dependencies carefully.
> +/** SQL Checks object. */
> +struct sql_checks {
> + /**
> + * The first in an array variables representing the
> + * new tuple to be inserted.
> + */
> + int new_tuple_var;
> + /**
> + * The first in array of variables representing the state
> + * (1 for ON and 0 for OFF) of corresponding Check. This
> + * is the UPDATE optimization - disabled checks would be
> + * skipped.
> + */
> + int checks_state_var;
I don't understand what these variables are for even though there are
comments :/
> + /**
> + * Precompiled reusable VDBE program for SQL Checks
> + * raising error on Check constraint failure. VDBE
> + * program assumes new_tuple and checks_state actual
> + * parameters to be placed in VDBE memory before run.
> + */
> + struct sqlite3_stmt *stmt;
> +};
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
` (2 preceding siblings ...)
2019-01-18 18:11 ` Konstantin Osipov
@ 2019-01-21 14:47 ` n.pettik
3 siblings, 0 replies; 14+ messages in thread
From: n.pettik @ 2019-01-21 14:47 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
You can’t ‘make’ check constraints.
> On 10 Jan 2019, at 16:54, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
>
> To make SQL CHECKS on space insertion in LUA we have introduced
> a new sql_check class containing precompiled reusable VDBE making
> required validations. It has been parameterized with binding
> parameters to map a tuple to be inserted in VDBE memory and
> to manage checks to be performed on UPDATE operation.
>
> Closes #3691
Commit message lacks doc bot request.
As far as users will be able to add and execute check
constraints apart from SQL module, it deserves to be
mentioned.
> diff --git a/src/box/space.c b/src/box/space.c
> index 4d174f7ec..b139a0905 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -29,6 +29,7 @@
> * SUCH DAMAGE.
> */
> #include "space.h"
> +#include "sql.h"
> #include <stdbool.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -108,6 +109,33 @@ space_fill_index_map(struct space *space)
> }
> }
>
> +/**
> + * SQL-specific actions for space.
> + */
Such comment looks like a joke
(as well as function’s name).
> +static void
> +on_space_before_replace(struct trigger *trigger, void *event)
> +{
> + uint32_t space_id = (uint32_t)(uintptr_t)trigger->data;
> + struct space *space = space_cache_find(space_id);
Why not simple space_by_id() if you anyway assert that
space ptr is not null?
> + assert(space != NULL);
> + struct txn *txn = (struct txn *) event;
> + struct txn_stmt *stmt = txn_current_stmt(txn);
> + if (stmt == NULL)
> + return;
> + struct tuple *new_tuple = stmt->new_tuple;
> + struct tuple *old_tuple = stmt->old_tuple;
> + if (new_tuple != NULL && space->sql_checks != NULL) {
> + const char *new_tuple_raw = tuple_data(new_tuple);
> + const char *old_tuple_raw = old_tuple != NULL ?
> + tuple_data(old_tuple) : NULL;
> + if (sql_checks_run(space->sql_checks,
I agree with Konstantin: we can't ‘run’ check constraint.
> + space->def->opts.checks_ast, space->def,
> + old_tuple_raw, new_tuple_raw) != 0) {
> + diag_raise();
> + }
Nit: don’t use brackets for one line if.
> + }
> +}
> +
> int
> space_create(struct space *space, struct engine *engine,
> const struct space_vtab *vtab, struct space_def *def,
> @@ -165,6 +193,25 @@ space_create(struct space *space, struct engine *engine,
> space_fill_index_map(space);
> rlist_create(&space->parent_fkey);
> rlist_create(&space->child_fkey);
> + if (def->opts.checks_ast != NULL) {
> + struct sql_checks *checks =
I guess it is no more sql specific thing.
Simple expression syntax has a little in common with SQL.
> + sql_checks_create(sql_get(), def->opts.checks_ast, def);
> + if (unlikely(checks == NULL))
Is this path considered to be hot? I mean OK, it is unlikely that
smth fails here, but only few OOM goto ifs has such attribute:
I grepped 46 unlikely usages for hundreds OOM.
It a nit, but this snippet looks strange.
> diff --git a/src/box/space.h b/src/box/space.h
> index 7eb7ae292..e83609992 100644
> --- a/src/box/space.h
> +++ b/src/box/space.h
> @@ -150,6 +150,11 @@ struct space {
> struct rlist on_stmt_begin;
> /** SQL Trigger list. */
> struct sql_trigger *sql_triggers;
> + /**
> + * The SQL Checks to perform on INSERT, REPLACE and
> + * UPDATE operations.
> + */
> + struct sql_checks *sql_checks;
I suggest to move check constraints to a separate space:
(_check_constraint or _ck_constraint) and remove raw string
from field_def and Expr from opts (in the same way as fk and
unique constraints are implemented).
Otherwise, check constraints are stored in three different places
in three different representations: as a raw string in field_def,
as an AST in space opts, and in space as a compiled VDBE
program.
What is more, even without that I guess we can remove AST
representation of check constraints: it is used only to compile
VDBE program. Anyway, if you still need it, I propose to add
it to sql_check structure.
> /**
> * The number of *enabled* indexes in the space.
> *
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 26b84c5db..8961c4fd9 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -35,9 +35,10 @@
> #include "sql/tarantoolInt.h"
> #include "sql/vdbeInt.h"
> #include "mpstream.h"
> +#include "execute.h"
>
> #include "index.h"
> -#include <info.h>
> +#include “info.h"
Do you really need this change?
> #include "schema.h"
> #include "box.h"
> #include "txn.h"
> @@ -1377,3 +1378,177 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
> sql_parser_destroy(&parser);
> return rc;
> }
> +
> +static int
> +check_is_enabled_callback(struct Walker *w, Expr *expr)
is_enabled sounds like smth mode of constraint execution.
On the other hand, it is kind of optimisation for UPDATE statement.
Hence, I suggest to rename it to “ck_constraint_can_be_skippep_cb” ,
“ck_constraint_is_skippable” or sort of.
Moreover, I see no functional changes after refactoring, so I guess
we can also move it to a separate commit (to split functional and code
style fixes).
> +{
> + /* CHECK constraint uses a changing column. */
> + const int marker = 0x01;
As a rule we move such constants to enums.
“marker" name is too general.
> + if (expr->op == TK_COLUMN) {
> + assert(expr->iColumn >= 0 || expr->iColumn == -1);
> + if (expr->iColumn >= 0) {
> + if (w->u.aiCol[expr->iColumn] >= 0)
> + w->eCode |= marker;
> + }
> + }
> + return WRC_Continue;
> +}
> +
> +/**
> + * Return true if this CHECK constraint can't be skipped when
> + * validating the new row in the UPDATE statement. In other
> + * words return false if CHECK constraint check does not use any of the
> + * changing columns.
> + * @param check CHECK constraint on a row that is being UPDATE-ed.
> + * The only columns that are modified by the UPDATE
> + * are those for which update_mask[i] >= 0.
> + * @param update_mask Array of items that were modified.
> + * 0 for unchanged field, -1 for modified.
> + * @retval 1 when specified check is required, 0 elsewhere.
Nit: not ‘elsewhere’, but ‘otherwise’.
> + */
> +static bool
> +sql_check_is_enabled(struct Expr *check, int *update_mask)
> +{
> + struct Walker w;
> + memset(&w, 0, sizeof(w));
> + w.eCode = 0;
> + w.xExprCallback = check_is_enabled_callback;
> + w.u.aiCol = update_mask;
> + sqlite3WalkExpr(&w, check);
> + return w.eCode == 0 ? 1 : 0;
> +}
> +
> +struct sql_checks *
> +sql_checks_create(struct sqlite3 *db, struct ExprList *checks_ast,
> + struct space_def *def)
You don’t need to pass db as an argument: you simply can
call sql_get() inside this function.
> +{
> + assert(checks_ast != NULL);
> + struct sql_checks *checks = malloc(sizeof(struct sql_checks));
> + if (unlikely(checks == NULL)) {
The same: lets remove ‘unlikely’.
> + diag_set(OutOfMemory, sizeof(struct sql_checks), "malloc",
> + "checks");
> + return NULL;
> + }
> +
> + struct Parse parser;
> + sql_parser_create(&parser, db);
> + struct Vdbe *v = sqlite3GetVdbe(&parser);
> + if (unlikely(v == NULL)) {
> + diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
> + free(checks);
> + return NULL;
> + }
> + /* Compile VDBE with default sql parameters. */
> + struct session *user_session = current_session();
> + uint32_t sql_flags = user_session->sql_flags;
> + user_session->sql_flags = default_sql_flags;
I’ve noticed that vdbe trace output now looks quite entangled:
it’s hard to understand where subprogram ends. For instance:
VDBE Trace:
101> 0 Init 0 9 0 00 Start at 9
SQL-trace: insert into t41 values(-11)
101> 9 TTransaction 0 0 0 00
101> 10 Goto 0 1 0 00
101> 1 Null 0 1 0 00 r[1]=NULL
REG[1] = NULL
101> 2 Integer -11 2 0 00 r[2]=-11
REG[2] = i:-11
101> 3 HaltIfNull 1294 2 2 T41.ID 01 if r[2]=null halt
REG[2] = i:-11
101> 4 Cast 2 68 0 00 affinity(r[2])
REG[2] = i:-11
101> 5 Affinity 2 1 0 D 00 affinity(r[2])
101> 6 MakeRecord 2 1 3 00 r[3]=mkrec(r[2])
REG[3] = e2[91F5..](8)
101> 7 IdxInsert 3 0 0 space<name=T41> 01 key=r[3]
VDBE Program Listing:
101> 0 Init 1 8 0 00 Start at 8
101> 1 Variable 1 1 0 00 r[1]=parameter(1,)
101> 2 Variable 2 2 0 00 r[2]=parameter(2,)
101> 3 Integer 0 3 0 00 r[3]=0
101> 4 Eq 2 7 3 00 if r[3]==r[2] goto 7
101> 5 Gt 5 7 1 (binary) 53 if r[1]>r[5] goto 7
101> 6 Halt 270 2 0 T41 03
101> 7 Halt 0 0 0 00
101> 8 Integer 0 5 0 00 r[5]=0
101> 9 Goto 0 1 0 00
VDBE Trace:
101> 0 Init 1 8 0 00 Start at 8
101> 8 Integer 0 5 0 00 r[5]=0
REG[5] = i:0
101> 9 Goto 0 1 0 00
101> 1 Variable 1 1 0 00 r[1]=parameter(1,)
REG[1] = i:-11
101> 2 Variable 2 2 0 00 r[2]=parameter(2,)
REG[2] = i:1
101> 3 Integer 0 3 0 00 r[3]=0
REG[3] = i:0
101> 4 Eq 2 7 3 00 if r[3]==r[2] goto 7
REG[2] = i:1
REG[3] = i:0
101> 5 Gt 5 7 1 (binary) 53 if r[1]>r[5] goto 7
REG[5] = i:0
REG[1] = i:-11
101> 6 Halt 270 2 0 T41 03
Could you add some markers indicating what is subprogram,
what is depth of currently executed program, where it ends etc.
It would be great to see at the start of program original expression
that is currently checked.
> +
> + /* Make continuous registers allocation. */
> + uint32_t reg_count = def->field_count + checks_ast->nExpr;
> + int new_tuple_reg = sqlite3GetTempRange(&parser, reg_count);
> + int checks_state_reg = def->field_count + 1;
> + /*
> + * Generate a prologe code to bind variables new_tuple_var
Nit: prologe -> prologue.
> + * and checks_state_var to new_tuple_reg and
> + * checks_state_reg respectively.
> + */
> + struct Expr bind = {.op = TK_VARIABLE, .u.zToken = "?"};
> + checks->new_tuple_var = parser.nVar + 1;
> + checks->checks_state_var = checks->new_tuple_var + def->field_count;
> + for (uint32_t i = 0; i < reg_count; i++) {
> + sqlite3ExprAssignVarNumber(&parser, &bind, 1);
> + sqlite3ExprCodeTarget(&parser, &bind, new_tuple_reg + i);
> + }
> + /* Generate Checks code. */
> + vdbe_emit_checks_test(&parser, checks_ast, def, new_tuple_reg,
> + checks_state_reg);
> + sql_finish_coding(&parser);
> + sql_parser_destroy(&parser);
> +
> + user_session->sql_flags = sql_flags;
> + checks->stmt = (struct sqlite3_stmt *)v;
> + return checks;
> +}
> +
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 028a15245..f2a6587da 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -68,6 +68,29 @@ struct Select;
> struct Table;
> struct sql_trigger;
>
> +/** SQL Checks object. */
Extend or remove this comment.
> +struct sql_checks {
> + /**
> + * The first in an array variables representing the
> + * new tuple to be inserted.
> + */
> + int new_tuple_var;
> + /**
> + * The first in array of variables representing the state
> + * (1 for ON and 0 for OFF) of corresponding Check. This
> + * is the UPDATE optimization - disabled checks would be
> + * skipped.
> + */
> + int checks_state_var;
> + /**
> + * Precompiled reusable VDBE program for SQL Checks
> + * raising error on Check constraint failure. VDBE
> + * program assumes new_tuple and checks_state actual
> + * parameters to be placed in VDBE memory before run.
> + */
> + struct sqlite3_stmt *stmt;
> +};
This looks like internal VDBE context.
I suggest to reconsider naming of this struct, place to
vdbe.c/vdbeaux.c and encapsulate it.
Moreover, as Konstantin already mentioned, it is not
clear from comments what these variables are used for.
> +
> /**
> * Perform parsing of provided expression. This is done by
> * surrounding the expression w/ 'SELECT ' prefix and perform
> @@ -411,6 +434,49 @@ void
> sqlite3SrcListDelete(struct sqlite3 *db, struct SrcList *list);
>
>
> +/**
> + * Create sql_checks object by Checks AST for specified space.
> + * @param db Database handler.
> + * @param checks_ast Checks AST expression list. Checks space def
> + * references may be modified during
> + * construction.
> + * @param def Space definition that checks_ast should refer to.
> + * @retval NULL on error.
> + * @retval not NULL SQL Checks object pointer on success.
> +*/
> +struct sql_checks *
> +sql_checks_create(struct sqlite3 *db, struct ExprList *checks_ast,
> + struct space_def *def);
> +
> +/**
> + * Destroy SQL Checks object, release related resources.
> + * @param checks SQL Checks object to destroy.
This is redundant description of argument.
> + */
> +void
> +sql_checks_destroy(struct sql_checks *checks);
> +
> +/**
> + * Run SQL Checks VDBE with parameters constructed by Checks AST,
> + * Space definition and alter tuples. Checks AST and Space
> + * definition must match be the same that were used to create the
Smth wrong with this sentence: you should choose one:
"must match” or "must be the same”.
> + * SQL Checks object.
> + * @param checks SQL Checks object containing VDBE to execute.
> + * @param checks_ast Checks AST object to determine Checks that
> + * may be omitted on execution (to prepare
> + * checks_state parameter).
> + * @param def Space definition describing alter tuples format.
> + * @param old_tuple_raw Old tuple to be replaced in UPDATE operation.
> + * (NULL for INSERT)
> + * @param new_tuple_raw New tuple to be inserted.
> + * @retval 0 on all required Checks were passed.
Nit: if all required check constraints are passed.
> + * @retval -1 on system error or Check constrints failure
Please, use spell checker.
Nit: constrints -> constraints.
> + * indicating an diag_msg to be raised.
> + */
> +int
> +sql_checks_run(const struct sql_checks *checks,
> + const struct ExprList *checks_ast, struct space_def *def,
> + const char *old_tuple_raw, const char *new_tuple_raw);
> +
> #if defined(__cplusplus)
> } /* extern "C" { */
> #endif
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 9c6b1b49a..40675fd32 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -873,7 +873,7 @@ struct ExprList *
> space_checks_expr_list(uint32_t space_id)
> {
> struct space *space;
> - space = space_by_id(space_id);
> + space = space_cache_find(space_id);
What is difference if right after that we are verifying that
space != NULL ?
> assert(space != NULL);
> assert(space->def != NULL);
> return space->def->opts.checks_ast;
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index f147f6a50..ab0e12def 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -813,51 +813,36 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> sqlite3DbFree(db, aRegIdx);
> }
>
> +void
> +vdbe_emit_checks_test(struct Parse *parser, struct ExprList *checks_ast,
To be honest, I don’t like name of this function.
I’d better call it “vdbe_emit_check_constraint(s)”.
> + struct space_def *def, int new_tuple_reg,
> + int checks_state_reg)
> {
> - if (pExpr->op == TK_COLUMN) {
> - assert(pExpr->iColumn >= 0 || pExpr->iColumn == -1);
> - if (pExpr->iColumn >= 0) {
> - if (pWalker->u.aiCol[pExpr->iColumn] >= 0) {
> - pWalker->eCode |= CKCNSTRNT_COLUMN;
> - }
> - }
> + struct Vdbe *v = sqlite3GetVdbe(parser);
> + assert(checks_ast != NULL);
> + int check_state_off = sqlite3GetTempReg(parser);
> + sqlite3VdbeAddOp2(v, OP_Integer, 0, check_state_off);
> + /*
> + * For CHECK constraint and for INSERT/UPDATE conflict
> + * action DEFAULT and ABORT in fact has the same meaning.
> + */
Why did you copy-paste this comment?
Now it is not related to code.
> + parser->ckBase = new_tuple_reg;
> + for (int i = 0; i < checks_ast->nExpr; i++) {
> + struct Expr *expr = checks_ast->a[i].pExpr;
> + int all_ok = sqlite3VdbeMakeLabel(v);
All_ok? At least all_is_ok according to our codestyle.
But I would give to this var more illustrative name.
For instance, “continue_label_addr" or simple “continue_label”.
> + /* Skip check when it turned off. */
Nit: “Skip check when it is turned off."
> + sqlite3VdbeAddOp3(v, OP_Eq, checks_state_reg + i, all_ok,
> + check_state_off);
> + sqlite3ExprIfTrue(parser, expr, all_ok, SQLITE_JUMPIFNULL);
> + char *name = checks_ast->a[i].zName;
> + if (name == NULL)
> + name = def->name;
> + sqlite3HaltConstraint(parser, SQLITE_CONSTRAINT_CHECK,
> + ON_CONFLICT_ACTION_ABORT, name,
> + P4_TRANSIENT, P5_ConstraintCheck);
> + sqlite3VdbeResolveLabel(v, all_ok);
> }
> - return WRC_Continue;
> -}
> -
>
> int
> sqlite3_exec(sqlite3 *, /* An open database */
> const char *sql, /* SQL to be evaluated */
> @@ -3869,6 +3882,25 @@ vdbe_emit_constraint_checks(struct Parse *parse_context,
> struct Table *tab, int new_tuple_reg,
> enum on_conflict_action on_conflict,
> int ignore_label, int *upd_cols);
> +/**
> + * This routine generates code to make Check constraints
> + * validations on tuple insertion during INSERT, REPLACE or
> + * UPDATE operation.
> + *
> + * @param parser Current parsing context.
> + * @param checks_ast Checks AST Expression list.
> + * @param def Destination space definition.
> + * @param new_tuple_reg The first register of group containing
> + * fields of tuple to be inserted.
> + * @param checks_state_reg The first register of group contining
Nit: contining -> containing.
> + * enabled state of corresponding checks:
> + * 0 for disabled check to be skipped,
> + * 1 - for enabled.
> + */
> +void
> +vdbe_emit_checks_test(struct Parse *parser, struct ExprList *checks_ast,
> + struct space_def *def, int new_tuple_reg,
> + int checks_state_reg);
>
> /**
> * This routine generates code to finish the INSERT or UPDATE
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 9f9d0eacf..74a2e366c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -992,7 +992,7 @@ case OP_HaltIfNull: { /* in3 */
> * automatically.
> *
> * P1 is the result code returned by sqlite3_exec(),
> - * sqlite3_reset(), or sqlite3_finalize(). For a normal halt,
> + * sql_stmt_reset(), or sqlite3_finalize(). For a normal halt,
> * this should be SQLITE_OK (0).
> * For errors, it can be some other value. If P1!=0 then P2 will
> * determine whether or not to rollback the current transaction.
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 9e57af051..b62de9d39 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -132,23 +132,15 @@ sqlite3_finalize(sqlite3_stmt * pStmt)
> return rc;
> }
>
> -/*
> - * Terminate the current execution of an SQL statement and reset it
> - * back to its starting state so that it can be reused. A success code from
> - * the prior execution is returned.
> - *
> - * This routine sets the error code and string returned by
> - * sqlite3_errcode(), sqlite3_errmsg() and sqlite3_errmsg16().
> - */
> int
> -sqlite3_reset(sqlite3_stmt * pStmt)
> +sql_stmt_reset(struct sqlite3_stmt *stmt)
Move refactoring of this function to a separate commit,
since it contains mostly code style fixes.
> {
> int rc;
> - if (pStmt == 0) {
> + if (stmt == NULL) {
> rc = SQLITE_OK;
> } else {
> - Vdbe *v = (Vdbe *) pStmt;
> - sqlite3 *db = v->db;
> + struct Vdbe *v = (struct Vdbe *)stmt;
> + struct sqlite3 *db = v->db;
> checkProfileCallback(db, v);
> rc = sqlite3VdbeReset(v);
If you start to refactor this function, fix return code discard:
rc = sqlite3VdbeReset(v);
…
rc = sqlite3ApiExit(db, rc);
Also, consider refactoring:
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index b62de9d39..8170e55e1 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -135,18 +135,15 @@ sqlite3_finalize(sqlite3_stmt * pStmt)
int
sql_stmt_reset(struct sqlite3_stmt *stmt)
{
- int rc;
- if (stmt == NULL) {
- rc = SQLITE_OK;
- } else {
- struct Vdbe *v = (struct Vdbe *)stmt;
- struct sqlite3 *db = v->db;
- checkProfileCallback(db, v);
- rc = sqlite3VdbeReset(v);
- sqlite3VdbeRewind(v);
- assert((rc & (db->errMask)) == rc);
- rc = sqlite3ApiExit(db, rc);
- }
+ if (stmt == NULL)
+ return 0;
+ struct Vdbe *v = (struct Vdbe *)stmt;
+ struct sqlite3 *db = v->db;
+ checkProfileCallback(db, v);
+ int rc = sqlite3VdbeReset(v);
+ sqlite3VdbeRewind(v);
+ assert((rc & (db->errMask)) == rc);
+ rc = sqlite3ApiExit(db, rc);
return rc;
}
> sqlite3VdbeRewind(v);
> @@ -509,30 +501,19 @@ sqlite3Step(Vdbe * p)
>
> assert(p);
> if (p->magic != VDBE_MAGIC_RUN) {
> - /* We used to require that sqlite3_reset() be called before retrying
> - * sqlite3_step() after any error or after SQLITE_DONE. But beginning
> - * with version 3.7.0, we changed this so that sqlite3_reset() would
> - * be called automatically instead of throwing the SQLITE_MISUSE error.
> - * This "automatic-reset" change is not technically an incompatibility,
> - * since any application that receives an SQLITE_MISUSE is broken by
> - * definition.
> - *
> - * Nevertheless, some published applications that were originally written
> - * for version 3.6.23 or earlier do in fact depend on SQLITE_MISUSE
> - * returns, and those were broken by the automatic-reset change. As a
> - * a work-around, the SQLITE_OMIT_AUTORESET compile-time restores the
> - * legacy behavior of returning SQLITE_MISUSE for cases where the
> - * previous sqlite3_step() returned something other than a SQLITE_LOCKED
> - * or SQLITE_BUSY error.
> + /*
> + * The sql_stmt_reset() routine would be called
> + * automatically instead of throwing the
> + * SQLITE_MISUSE error.
> */
> #ifdef SQLITE_OMIT_AUTORESET
> if ((rc = p->rc & 0xff) == SQLITE_BUSY || rc == SQLITE_LOCKED) {
> - sqlite3_reset((sqlite3_stmt *) p);
> + sql_stmt_reset((sqlite3_stmt *) p);
> } else {
> return SQLITE_MISUSE_BKPT;
> }
> #else
> - sqlite3_reset((sqlite3_stmt *) p);
> + sql_stmt_reset((sqlite3_stmt *) p);
> #endif
> }
>
> @@ -632,7 +613,7 @@ sqlite3_step(sqlite3_stmt * pStmt)
> rc2 = rc = sqlite3Reprepare(v);
> if (rc != SQLITE_OK)
> break;
> - sqlite3_reset(pStmt);
> + sql_stmt_reset(pStmt);
> if (savedPc >= 0)
> v->doingRerun = 1;
> assert(v->expired == 0);
> diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
> index 1176ad500..08b7edcc7 100755
> --- a/test/sql-tap/check.test.lua
> +++ b/test/sql-tap/check.test.lua
> @@ -55,7 +55,7 @@ test:do_catchsql_test(
> INSERT INTO t1 VALUES(6,7, 2);
> ]], {
> -- <check-1.3>
> - 1, "CHECK constraint failed: T1"
> + 1, "Failed to execute SQL statement: CHECK constraint failed: T1”
But check constraints are no longer SQL specific thing:
user can get this error without involving any SQL routines.
Hence, it would look quite strange, I guess.
>
> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index 0582bbb63..5fb6f65dc 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -60,5 +60,23 @@ format = {{name = 'X', type = 'unsigned'}}
> t = {513, 1, 'test', 'memtx', 0, opts, format}
> s = box.space._space:insert(t)
>
> +--
> +-- gh-3691: Do SQL checks on server side
> +--
> +box.sql.execute("CREATE TABLE t1(x INTEGER CONSTRAINT ONE CHECK( x<5 ), y REAL CONSTRAINT TWO CHECK( y>x ), z INTEGER PRIMARY KEY);")
> +box.space.T1:insert({7, 1, 1})
> +box.space.T1:insert({2, 1, 1})
> +box.space.T1:insert({2, 4, 1})
> +box.space.T1:update({1}, {{"=", 1, 7}})
> +box.sql.execute("DROP TABLE t1");
> +
> +opts = {checks = {{expr ='X > 5', name = 'ONE'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +_ = box.space.test:create_index('pk')
> +box.space.test:insert({1})
> +box.space.test:insert({6})
> +box.space.test:drop()
Such poor test on this significantly important issue?
Also, firstly please consider Konstantin’s remarks and comments
to this and previous patches.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-01-21 14:47 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-10 13:54 [tarantool-patches] [PATCH v1 0/4] sql: Checks on server side Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
2019-01-11 14:05 ` [tarantool-patches] " Konstantin Osipov
2019-01-18 18:00 ` Konstantin Osipov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
2019-01-11 14:06 ` [tarantool-patches] " Konstantin Osipov
2019-01-11 14:07 ` Konstantin Osipov
2019-01-18 18:04 ` Konstantin Osipov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 3/4] box: exported sql_bind structure and API Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
2019-01-11 14:12 ` [tarantool-patches] " Konstantin Osipov
2019-01-11 14:14 ` Konstantin Osipov
2019-01-18 18:11 ` Konstantin Osipov
2019-01-21 14:47 ` n.pettik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox