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 315FA24CC0 for ; Sun, 24 Jun 2018 11:25:37 -0400 (EDT) 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 Q8othlsC2AQh for ; Sun, 24 Jun 2018 11:25:37 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 BFD8024C93 for ; Sun, 24 Jun 2018 11:25:36 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence From: "n.pettik" In-Reply-To: <8bb49455a1fa0aa86ca338deffb25d4d447acb7e.1529490955.git.kshcherbatov@tarantool.org> Date: Sun, 24 Jun 2018 18:25:34 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <0CD65924-D11C-401F-A9D5-4EC06F5310A8@tarantool.org> References: <8bb49455a1fa0aa86ca338deffb25d4d447acb7e.1529490955.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 > On 20 Jun 2018, at 13:46, Kirill Shcherbatov = wrote: >=20 > Trigger presence in system should be Why? If you didn=E2=80=99t check it, you simply would get =E2=80=98duplica= te key error=E2=80=99. So, you did it to display more informative error..? > tested on each VDBE > execution attempt, not on Parser iteration. And? After this sentence you should explain changes which you patch = provides. >=20 > Part of #3435, #3273 > --- > src/box/sql/build.c | 42 = ++++++++++++++++++++++++++++++++++++++++++ > src/box/sql/main.c | 11 ++++++++--- > src/box/sql/sqliteInt.h | 24 ++++++++++++++++++++++++ > src/box/sql/trigger.c | 25 +++++++++++++------------ > src/box/sql/vdbe.c | 18 +++++++++++------- > test/sql-tap/view.test.lua | 8 ++++---- > test/sql/view.result | 4 ++-- > 7 files changed, 104 insertions(+), 28 deletions(-) >=20 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index c8bfad7..fff7c19 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -3971,4 +3971,46 @@ sqlite3WithDelete(sqlite3 * db, With * pWith) > sqlite3DbFree(db, pWith); > } > } > + > +int > +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int = space_id, I would call it vdbe_emit_halt_if_exists(). Or vdbe_emit_halt_on_duplication()... > + int index_id, const char *name_src, > + int tarantool_error_code, > + const char *error_src, bool no_error) > +{ > + struct Vdbe *v =3D sqlite3GetVdbe(parser); > + assert(v !=3D NULL); > + > + struct sqlite3 *db =3D parser->db; > + char *name =3D sqlite3DbStrDup(db, name_src); > + if (name =3D=3D NULL) { > + size_t size =3D strlen(name_src) + 1; > + diag_set(OutOfMemory, size, "sqlite3DbStrDup", = "name=E2=80=9D); In previous patch, you used sqlite3DbMalloc() function without using = diag_set(), since in case of fail it would call sqlite3OomFault(db); Lets handle all usages of sqlite3DbMalloc() in the same way. > + return -1; > + } > + char *error =3D sqlite3DbStrDup(db, error_src); > + if (error =3D=3D NULL) { > + sqlite3DbFree(db, name); > + size_t size =3D strlen(error_src) + 1; > + diag_set(OutOfMemory, size, "sqlite3DbStrDup", = "error=E2=80=9D); The same is here. > + return -1; > + } > + > + int cursor =3D parser->nTab++; > + vdbe_emit_open_cursor(parser, cursor, index_id, = space_by_id(space_id)); > + > + int name_reg =3D parser->nMem++; > + int label =3D sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, = name, > + P4_DYNAMIC); > + sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, = name_reg, 1); > + if (no_error) { > + sqlite3VdbeAddOp0(v, OP_Halt); > + } else { > + sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, > + ON_CONFLICT_ACTION_FAIL, 0, error, = P4_DYNAMIC); Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with = 0. > + sqlite3VdbeChangeP5(v, tarantool_error_code); > + } You may refactor code this way (if you like it): +++ b/src/box/sql/build.c @@ -4003,11 +4003,10 @@ vdbe_emit_execution_halt_on_exists(struct Parse = *parser, int space_id, int label =3D sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, = name, P4_DYNAMIC); sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, = name_reg, 1); - if (no_error) { - sqlite3VdbeAddOp0(v, OP_Halt); - } else { - sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, - ON_CONFLICT_ACTION_FAIL, 0, error, = P4_DYNAMIC); + int op_halt_addr =3D sqlite3VdbeAddOp0(v, OP_Halt); + if (!no_error) { + sqlite3VdbeChangeP1(v, op_halt_addr, = SQL_TARANTOOL_ERROR); + sqlite3VdbeChangeP4(v, op_halt_addr, error, P4_DYNAMIC); sqlite3VdbeChangeP5(v, tarantool_error_code); } > + sqlite3VdbeAddOp1(v, OP_Close, cursor); > + return 0; > +} > #endif /* !defined(SQLITE_OMIT_CTE) */ Why this func is under SQLITE_OMIT_CTE guard? > diff --git a/src/box/sql/main.c b/src/box/sql/main.c > index 0acf7bc..e435c01 100644 > --- a/src/box/sql/main.c > +++ b/src/box/sql/main.c > @@ -1454,11 +1454,16 @@ sqlite3_errmsg(sqlite3 * db) > z =3D sqlite3ErrStr(SQLITE_NOMEM_BKPT); > } else { > testcase(db->pErr =3D=3D 0); > - z =3D (char *)sqlite3_value_text(db->pErr); > assert(!db->mallocFailed); > - if (z =3D=3D 0) { > - z =3D sqlite3ErrStr(db->errCode); > + if (db->errCode !=3D SQL_TARANTOOL_ERROR) { > + assert(!db->mallocFailed); Why have you duplicated this assert? It occurs 2 lines above. > + z =3D (char *)sqlite3_value_text(db->pErr); > + if (z =3D=3D NULL) > + z =3D sqlite3ErrStr(db->errCode); > + } else { > + z =3D diag_last_error(diag_get())->errmsg; > } > + assert(z !=3D NULL); > } > return z; > } > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 72803fa..acda23d 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -4627,4 +4627,28 @@ extern int sqlite3InitDatabase(sqlite3 * db); > enum on_conflict_action > table_column_nullable_action(struct Table *tab, uint32_t column); >=20 > +/** > + * Generate VDBE code to halt execution with correct error if > + * the object with specified name is already present in > + * specified space. > + * The function does not begin to hold the passed error pointer > + * to memory. Rephrase last sentence - I can=E2=80=99t parse it.. > + * > + * @param parser Parsing context. > + * @param space_id Space to lookup identifier. > + * @param index_id Index identifier containing string primary key. How can index id contain =E2=80=99string primary key=E2=80=99? > + * @param name_src Of object to test existence. Name of object to test on existence. > + * @param tarantool_error_code to set on halt. > + * @param error_src string to notify on halt. Error message to display on VDBE halt. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index c1df880..487b026 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -960,7 +960,9 @@ case OP_HaltIfNull: { /* in3 */ > * > * If P4 is not null then it is an error message string. > * > - * P5 is a value between 0 and 4, inclusive, that modifies the P4 = string. > + * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and > + * P4 is error message to set. Else P5 is a value between 0 and 4, > + * inclusive, that modifies the P4 string. > * > * 0: (no change) > * 1: NOT NULL contraint failed: P4 > @@ -968,8 +970,8 @@ case OP_HaltIfNull: { /* in3 */ > * 3: CHECK constraint failed: P4 > * 4: FOREIGN KEY constraint failed: P4 > * > - * If P5 is not zero and P4 is NULL, then everything after the ":" is > - * omitted. > + * If P5 is not zero and P4 is NULL, then everything after the > + * ":" is omitted. Looks like redundant diff. If you wanted to make comments fit into 66 = chars, you would better do it for the whole comment. > * > * There is an implied "Halt 0 0 0" instruction inserted at the very = end of > * every program. So a jump past the last instruction of the program > @@ -1005,9 +1007,11 @@ case OP_Halt: { > p->rc =3D pOp->p1; > p->errorAction =3D (u8)pOp->p2; > p->pc =3D pcx; > - assert(pOp->p5<=3D4); > if (p->rc) { > - if (pOp->p5) { > + if (p->rc =3D=3D SQL_TARANTOOL_ERROR) { > + assert(pOp->p4.z !=3D NULL); > + box_error_set(__FILE__, __LINE__, pOp->p5, = pOp->p4.z); > + } else if (pOp->p5 !=3D 0) { > static const char * const azType[] =3D { "NOT = NULL", "UNIQUE", "CHECK", > "FOREIGN = KEY" }; > testcase( pOp->p5=3D=3D1); > @@ -2999,8 +3003,8 @@ case OP_CheckViewReferences: { > struct space *space =3D space_by_id(space_id); > assert(space !=3D NULL); > if (space->def->view_ref_count > 0) { > - sqlite3VdbeError(p,"Can't drop table %s: other views = depend " > - "on this space", space->def->name); > + diag_set(ClientError, ER_DROP_SPACE, space->def->name, > + "other views depend on this space=E2=80=9D); Why did you provide this diff? AFAIK in this particular case these two = calls are *almost* equivalent.