[tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side

["n.pettik" <korablev@tarantool.org>]

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.