From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A48172445C for ; Mon, 21 Jan 2019 09:47:39 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FYmLINRm_g2S for ; Mon, 21 Jan 2019 09:47:39 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D53D222982 for ; Mon, 21 Jan 2019 09:47:38 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side From: "n.pettik" In-Reply-To: <2d7c73e83bcee024ffcc095854a31bcd46f6ccab.1547128310.git.kshcherbatov@tarantool.org> Date: Mon, 21 Jan 2019 17:47:36 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <2d7c73e83bcee024ffcc095854a31bcd46f6ccab.1547128310.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov You can=E2=80=99t =E2=80=98make=E2=80=99 check constraints.=20 > On 10 Jan 2019, at 16:54, Kirill Shcherbatov = wrote: >=20 > 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. >=20 > 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 > #include > #include > @@ -108,6 +109,33 @@ space_fill_index_map(struct space *space) > } > } >=20 > +/** > + * SQL-specific actions for space. > + */ Such comment looks like a joke (as well as function=E2=80=99s name). > +static void > +on_space_before_replace(struct trigger *trigger, void *event) > +{ > + uint32_t space_id =3D (uint32_t)(uintptr_t)trigger->data; > + struct space *space =3D space_cache_find(space_id); Why not simple space_by_id() if you anyway assert that space ptr is not null? > + assert(space !=3D NULL); > + struct txn *txn =3D (struct txn *) event; > + struct txn_stmt *stmt =3D txn_current_stmt(txn); > + if (stmt =3D=3D NULL) > + return; > + struct tuple *new_tuple =3D stmt->new_tuple; > + struct tuple *old_tuple =3D stmt->old_tuple; > + if (new_tuple !=3D NULL && space->sql_checks !=3D NULL) { > + const char *new_tuple_raw =3D tuple_data(new_tuple); > + const char *old_tuple_raw =3D old_tuple !=3D NULL ? > + tuple_data(old_tuple) : = NULL; > + if (sql_checks_run(space->sql_checks, I agree with Konstantin: we can't =E2=80=98run=E2=80=99 check = constraint. > + space->def->opts.checks_ast, = space->def, > + old_tuple_raw, new_tuple_raw) !=3D 0) = { > + diag_raise(); > + } Nit: don=E2=80=99t 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 !=3D NULL) { > + struct sql_checks *checks =3D 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 =3D=3D 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" >=20 > #include "index.h" > -#include > +#include =E2=80=9Cinfo.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 =E2=80=9Cck_constraint_can_be_skippep_cb=E2= =80=9D , =E2=80=9Cck_constraint_is_skippable=E2=80=9D 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 =3D 0x01; As a rule we move such constants to enums. =E2=80=9Cmarker" name is too general. > + if (expr->op =3D=3D TK_COLUMN) { > + assert(expr->iColumn >=3D 0 || expr->iColumn =3D=3D -1); > + if (expr->iColumn >=3D 0) { > + if (w->u.aiCol[expr->iColumn] >=3D 0) > + w->eCode |=3D 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] >=3D 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 =E2=80=98elsewhere=E2=80=99, but =E2=80=98otherwise=E2=80=99. > + */ > +static bool > +sql_check_is_enabled(struct Expr *check, int *update_mask) > +{ > + struct Walker w; > + memset(&w, 0, sizeof(w)); > + w.eCode =3D 0; > + w.xExprCallback =3D check_is_enabled_callback; > + w.u.aiCol =3D update_mask; > + sqlite3WalkExpr(&w, check); > + return w.eCode =3D=3D 0 ? 1 : 0; > +} > + > +struct sql_checks * > +sql_checks_create(struct sqlite3 *db, struct ExprList *checks_ast, > + struct space_def *def) You don=E2=80=99t need to pass db as an argument: you simply can call sql_get() inside this function. > +{ > + assert(checks_ast !=3D NULL); > + struct sql_checks *checks =3D malloc(sizeof(struct sql_checks)); > + if (unlikely(checks =3D=3D NULL)) { The same: lets remove =E2=80=98unlikely=E2=80=99. > + diag_set(OutOfMemory, sizeof(struct sql_checks), = "malloc", > + "checks"); > + return NULL; > + } > + > + struct Parse parser; > + sql_parser_create(&parser, db); > + struct Vdbe *v =3D sqlite3GetVdbe(&parser); > + if (unlikely(v =3D=3D NULL)) { > + diag_set(ClientError, ER_SQL_EXECUTE, = sqlite3_errmsg(db)); > + free(checks); > + return NULL; > + } > + /* Compile VDBE with default sql parameters. */ > + struct session *user_session =3D current_session(); > + uint32_t sql_flags =3D user_session->sql_flags; > + user_session->sql_flags =3D default_sql_flags; I=E2=80=99ve noticed that vdbe trace output now looks quite entangled: it=E2=80=99s 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=20 101> 10 Goto 0 1 0 00=20 101> 1 Null 0 1 0 00 r[1]=3DNULL REG[1] =3D NULL 101> 2 Integer -11 2 0 00 r[2]=3D-11 REG[2] =3D i:-11 101> 3 HaltIfNull 1294 2 2 T41.ID 01 if r[2]=3Dnull = halt REG[2] =3D i:-11 101> 4 Cast 2 68 0 00 affinity(r[2]) REG[2] =3D i:-11 101> 5 Affinity 2 1 0 D 00 affinity(r[2]) 101> 6 MakeRecord 2 1 3 00 = r[3]=3Dmkrec(r[2]) REG[3] =3D e2[91F5..](8) 101> 7 IdxInsert 3 0 0 space 01 key=3Dr[3] VDBE Program Listing: 101> 0 Init 1 8 0 00 Start at 8 101> 1 Variable 1 1 0 00 = r[1]=3Dparameter(1,) 101> 2 Variable 2 2 0 00 = r[2]=3Dparameter(2,) 101> 3 Integer 0 3 0 00 r[3]=3D0 101> 4 Eq 2 7 3 00 if r[3]=3D=3Dr[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=20 101> 7 Halt 0 0 0 00=20 101> 8 Integer 0 5 0 00 r[5]=3D0 101> 9 Goto 0 1 0 00=20 VDBE Trace: 101> 0 Init 1 8 0 00 Start at 8 101> 8 Integer 0 5 0 00 r[5]=3D0 REG[5] =3D i:0 101> 9 Goto 0 1 0 00=20 101> 1 Variable 1 1 0 00 = r[1]=3Dparameter(1,) REG[1] =3D i:-11 101> 2 Variable 2 2 0 00 = r[2]=3Dparameter(2,) REG[2] =3D i:1 101> 3 Integer 0 3 0 00 r[3]=3D0 REG[3] =3D i:0 101> 4 Eq 2 7 3 00 if r[3]=3D=3Dr[2]= goto 7 REG[2] =3D i:1 REG[3] =3D i:0 101> 5 Gt 5 7 1 (binary) 53 if r[1]>r[5] = goto 7 REG[5] =3D i:0 REG[1] =3D 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 =3D def->field_count + checks_ast->nExpr; > + int new_tuple_reg =3D sqlite3GetTempRange(&parser, reg_count); > + int checks_state_reg =3D 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 =3D {.op =3D TK_VARIABLE, .u.zToken =3D "?"}; > + checks->new_tuple_var =3D parser.nVar + 1; > + checks->checks_state_var =3D checks->new_tuple_var + = def->field_count; > + for (uint32_t i =3D 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 =3D sql_flags; > + checks->stmt =3D (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; >=20 > +/** 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); >=20 >=20 > +/** > + * 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=E2=80=9D or "must be the same=E2=80=9D.=20 > + * 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 =3D space_by_id(space_id); > + space =3D space_cache_find(space_id); What is difference if right after that we are verifying that space !=3D NULL ? > assert(space !=3D NULL); > assert(space->def !=3D 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); > } >=20 > +void > +vdbe_emit_checks_test(struct Parse *parser, struct ExprList = *checks_ast, To be honest, I don=E2=80=99t like name of this function. I=E2=80=99d better call it =E2=80=9Cvdbe_emit_check_constraint(s)=E2=80=9D= . > + struct space_def *def, int new_tuple_reg, > + int checks_state_reg) > { > - if (pExpr->op =3D=3D TK_COLUMN) { > - assert(pExpr->iColumn >=3D 0 || pExpr->iColumn =3D=3D = -1); > - if (pExpr->iColumn >=3D 0) { > - if (pWalker->u.aiCol[pExpr->iColumn] >=3D 0) { > - pWalker->eCode |=3D CKCNSTRNT_COLUMN; > - } > - } > + struct Vdbe *v =3D sqlite3GetVdbe(parser); > + assert(checks_ast !=3D NULL); > + int check_state_off =3D 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 =3D new_tuple_reg; > + for (int i =3D 0; i < checks_ast->nExpr; i++) { > + struct Expr *expr =3D checks_ast->a[i].pExpr; > + int all_ok =3D 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, =E2=80=9Ccontinue_label_addr" or simple = =E2=80=9Ccontinue_label=E2=80=9D. > + /* Skip check when it turned off. */ Nit: =E2=80=9CSkip 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 =3D checks_ast->a[i].zName; > + if (name =3D=3D NULL) > + name =3D def->name; > + sqlite3HaltConstraint(parser, SQLITE_CONSTRAINT_CHECK, > + ON_CONFLICT_ACTION_ABORT, name, > + P4_TRANSIENT, P5_ConstraintCheck); > + sqlite3VdbeResolveLabel(v, all_ok); > } > - return WRC_Continue; > -} > - >=20 > 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); >=20 > /** > * 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!=3D0 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; > } >=20 > -/* > - * 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 =3D=3D 0) { > + if (stmt =3D=3D NULL) { > rc =3D SQLITE_OK; > } else { > - Vdbe *v =3D (Vdbe *) pStmt; > - sqlite3 *db =3D v->db; > + struct Vdbe *v =3D (struct Vdbe *)stmt; > + struct sqlite3 *db =3D v->db; > checkProfileCallback(db, v); > rc =3D sqlite3VdbeReset(v); If you start to refactor this function, fix return code discard: rc =3D sqlite3VdbeReset(v); =E2=80=A6 rc =3D 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 =3D=3D NULL) { - rc =3D SQLITE_OK; - } else { - struct Vdbe *v =3D (struct Vdbe *)stmt; - struct sqlite3 *db =3D v->db; - checkProfileCallback(db, v); - rc =3D sqlite3VdbeReset(v); - sqlite3VdbeRewind(v); - assert((rc & (db->errMask)) =3D=3D rc); - rc =3D sqlite3ApiExit(db, rc); - } + if (stmt =3D=3D NULL) + return 0; + struct Vdbe *v =3D (struct Vdbe *)stmt; + struct sqlite3 *db =3D v->db; + checkProfileCallback(db, v); + int rc =3D sqlite3VdbeReset(v); + sqlite3VdbeRewind(v); + assert((rc & (db->errMask)) =3D=3D rc); + rc =3D sqlite3ApiExit(db, rc); return rc; } > sqlite3VdbeRewind(v); > @@ -509,30 +501,19 @@ sqlite3Step(Vdbe * p) >=20 > assert(p); > if (p->magic !=3D 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 =3D p->rc & 0xff) =3D=3D SQLITE_BUSY || rc =3D=3D = 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 > } >=20 > @@ -632,7 +613,7 @@ sqlite3_step(sqlite3_stmt * pStmt) > rc2 =3D rc =3D sqlite3Reprepare(v); > if (rc !=3D SQLITE_OK) > break; > - sqlite3_reset(pStmt); > + sql_stmt_reset(pStmt); > if (savedPc >=3D 0) > v->doingRerun =3D 1; > assert(v->expired =3D=3D 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); > ]], { > -- > - 1, "CHECK constraint failed: T1" > + 1, "Failed to execute SQL statement: CHECK constraint failed: = T1=E2=80=9D 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. >=20 > 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 =3D {{name =3D 'X', type =3D 'unsigned'}} > t =3D {513, 1, 'test', 'memtx', 0, opts, format} > s =3D box.space._space:insert(t) >=20 > +-- > +-- 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}, {{"=3D", 1, 7}}) > +box.sql.execute("DROP TABLE t1"); > + > +opts =3D {checks =3D {{expr =3D'X > 5', name =3D 'ONE'}}} > +format =3D {{name =3D 'X', type =3D 'unsigned'}} > +t =3D {513, 1, 'test', 'memtx', 0, opts, format} > +s =3D box.space._space:insert(t) > +_ =3D 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=E2=80=99s remarks and comments to this and previous patches.