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 CEFC73163B for ; Wed, 26 Jun 2019 12:11:09 -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 vhia_MAaV6wO for ; Wed, 26 Jun 2019 12:11:09 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 8F17B312C7 for ; Wed, 26 Jun 2019 12:11:09 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: add OP_Error opcode in VDBE From: "n.pettik" In-Reply-To: <7cbb598d41f2c5751538d22d5ea8c90e07fc8ebd.1561476468.git.imeevma@gmail.com> Date: Wed, 26 Jun 2019 19:11:06 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <2CCDE513-7D85-41C7-A50D-3EDFA076EA8C@tarantool.org> References: <7cbb598d41f2c5751538d22d5ea8c90e07fc8ebd.1561476468.git.imeevma@gmail.com> 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: Imeev Mergen > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 292168f..c75e1d8 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1066,14 +1066,21 @@ vdbe_emit_ck_constraint_create(struct Parse = *parser, > sqlDbStrDup(db, ck_def->expr_str), P4_DYNAMIC); > sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 5, > ck_constraint_reg + 5); > - const char *error_msg =3D > + /* Check that there is no CHECK with such name. */ > + const char *err =3D > tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), > ck_def->name); > - if (vdbe_emit_halt_with_presence_test(parser, = BOX_CK_CONSTRAINT_ID, 0, > - ck_constraint_reg, 2, > - ER_CONSTRAINT_EXISTS, = error_msg, > - false, OP_NoConflict) !=3D = 0) > - return; > + int cursor =3D parser->nTab++; > + vdbe_emit_open_cursor(parser, cursor, 0, > + space_by_id(BOX_CK_CONSTRAINT_ID)); > + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); > + sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 3, > + ck_constraint_reg, 2); > + sqlVdbeAddOp4(v, OP_Error, ER_CONSTRAINT_EXISTS, v->nOp + 1, 0, = err, > + P4_STATIC); > + sqlVdbeAddOp1(v, OP_Halt, -1); > + sqlVdbeAddOp1(v, OP_Close, cursor); I don=E2=80=99t like code duplication: this snippet looks almost the = same as vdbe_emit_halt_with_presence_test(). Wouldn=E2=80=99t it be better if = OP_Error was integrated to that function? Also, why not move error setting to OP_Error from other OP_Halt usages? > + > sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0, > ck_constraint_reg + 5); > save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2, > @@ -1131,14 +1138,19 @@ vdbe_emit_fk_constraint_create(struct Parse = *parse_context, > * Lets check that constraint with this name hasn't > * been created before. > */ > - const char *error_msg =3D > + const char *err =3D > tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), = name_copy); > - if (vdbe_emit_halt_with_presence_test(parse_context, > - BOX_FK_CONSTRAINT_ID, 0, > - constr_tuple_reg, 2, > - ER_CONSTRAINT_EXISTS, = error_msg, > - false, OP_NoConflict) !=3D = 0) > - return; > + int cursor =3D parse_context->nTab++; > + vdbe_emit_open_cursor(parse_context, cursor, 0, > + space_by_id(BOX_FK_CONSTRAINT_ID)); > + sqlVdbeChangeP5(vdbe, OPFLAG_SYSTEMSP); > + sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 3, > + constr_tuple_reg, 2); > + sqlVdbeAddOp4(vdbe, OP_Error, ER_CONSTRAINT_EXISTS, vdbe->nOp + = 1, 0, > + err, P4_STATIC); > + sqlVdbeAddOp1(vdbe, OP_Halt, -1); > + sqlVdbeAddOp1(vdbe, OP_Close, cursor); > + > sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + = 3); > sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0, > fk_constraint_match_strs[fk->match], = P4_STATIC); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index c8887f9..abf881a 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -885,6 +885,18 @@ case OP_Goto: { /* jump */ > goto jump_to_p2; > } >=20 > +/* Opcode: Error P1 P2 * P4 * I=E2=80=99d call it OP_DiagError or OP_SetDiag. > + * > + * Setting an error and an unconditional jump to address P2. Nit: -> Set an error and process unconditional jump =E2=80=A6 Or -> Set an error and unconditionally jump to=20 > + * > + * The P1 parameter is error code of the erroe. The P4 parameter -> is an error code to be set. > + * is a description of the error. -> is a text description. > + */ > +case OP_Error: { /* jump */ > + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z); Why not simple diag_set() ? > + goto jump_to_p2; > +} > +