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 5601B24E4D for ; Wed, 17 Jul 2019 12:58:46 -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 rDqjJz8SFssz for ; Wed, 17 Jul 2019 12:58:46 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 D0E0C24DB0 for ; Wed, 17 Jul 2019 12:58:45 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger From: "n.pettik" In-Reply-To: Date: Wed, 17 Jul 2019 19:58:43 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <742F5B6B-D414-41DF-A0CF-342F13A4C5F0@tarantool.org> References: 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/insert.c b/src/box/sql/insert.c > index d2b4e17..6a2a742 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -728,6 +728,11 @@ sqlInsert(Parse * pParse, /* Parser = context */ > } > } >=20 > + assert(regData > 0); Pretty much useless assertion here. > + int autoinc_reg =3D 0; > + if (autoinc_fieldno < UINT32_MAX && > + pParse->triggered_space =3D=3D NULL) > + autoinc_reg =3D regData + autoinc_fieldno; > /* > * Generate code to check constraints and process > * final insertion. > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 3b8c82d..654eebd 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3506,7 +3506,8 @@ vdbe_emit_ck_constraint(struct Parse *parser, = struct Expr *expr, > void > vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space, > int raw_data_reg, uint32_t tuple_len, > - enum on_conflict_action on_conflict); > + enum on_conflict_action on_conflict, > + int autoinc_reg); You forgot to update comment to this function. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 6bb7bc3..951303c 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe) > return &vdbe->autoinc_id_list; > } >=20 > -int > +static int > vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id) > { > assert(vdbe !=3D NULL); > @@ -4161,12 +4161,17 @@ case OP_SorterInsert: { /* in2 */ > break; > } >=20 > -/* Opcode: IdxInsert P1 P2 * P4 P5 > +/* Opcode: IdxInsert P1 P2 P3 P4 P5 > * Synopsis: key=3Dr[P1] > * > * @param P1 Index of a register with MessagePack data to insert. > * @param P2 If P4 is not set, then P2 is register containing pointer > * to space to insert into. > + * @param P3 If not 0, than it is an index of a register that > + * contains value that will be inserted into field with > + * AUTOINCREMENT. If the value is NULL, than the newly > + * generated autoincrement value will be saved to VDBE > + * context. > * @param P4 Pointer to the struct space to insert to. > * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE > * accounts the change in a case of successful insertion in > @@ -4208,6 +4213,13 @@ case OP_IdxInsert: { > rc =3D tarantoolsqlEphemeralInsert(space, pIn2->z, > pIn2->z + pIn2->n); > } > + if (rc =3D=3D 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & = MEM_Null) !=3D 0) { > + assert(space->sequence !=3D NULL); > + int64_t value; > + if (sequence_get_value(space->sequence, &value) !=3D 0) > + goto abort_due_to_error; > + vdbe_add_new_autoinc_id(p, value); > + } I=E2=80=99d refactor this way (placed it at the top of last commit): diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 02f35c406..f3d70a982 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4171,7 +4171,8 @@ case OP_SorterInsert: { /* in2 */ * contains value that will be inserted into field with * AUTOINCREMENT. If the value is NULL, than the newly * generated autoincrement value will be saved to VDBE - * context. + * context. At the end of execution, mentioned list of + * ids is returned as a result of query execution. * @param P4 Pointer to the struct space to insert to. * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE * accounts the change in a case of successful insertion in @@ -4211,31 +4212,36 @@ case OP_IdxInsert: { rc =3D tarantoolsqlEphemeralInsert(space, pIn2->z, pIn2->z + pIn2->n); } - if (rc =3D=3D 0) { - if (pOp->p5 & OPFLAG_NCHANGE) - p->nChange++; - if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) !=3D= 0) { - assert(space->sequence !=3D NULL); - int64_t value; - if (sequence_get_value(space->sequence, &value) = !=3D 0) - goto abort_due_to_error; - vdbe_add_new_autoinc_id(p, value); + if (rc !=3D 0) { + if ((pOp->p5 & OPFLAG_OE_IGNORE) !=3D 0) { + /* + * Ignore any kind of fails and do not + * raise error message. + */ + rc =3D 0; + /* + * If we are in trigger, increment ignore + * raised counter. + */ + if (p->pFrame) + p->ignoreRaised++; + break; } + if (pOp->p5 & OPFLAG_OE_FAIL) + p->errorAction =3D ON_CONFLICT_ACTION_FAIL; + else if (pOp->p5 & OPFLAG_OE_ROLLBACK) + p->errorAction =3D ON_CONFLICT_ACTION_ROLLBACK; + goto abort_due_to_error; } - - if (pOp->p5 & OPFLAG_OE_IGNORE) { - /* Ignore any kind of failes and do not raise error = message */ - rc =3D 0; - /* If we are in trigger, increment ignore raised counter = */ - if (p->pFrame) - p->ignoreRaised++; - } else if (pOp->p5 & OPFLAG_OE_FAIL) { - p->errorAction =3D ON_CONFLICT_ACTION_FAIL; - } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) { - p->errorAction =3D ON_CONFLICT_ACTION_ROLLBACK; + if (pOp->p5 & OPFLAG_NCHANGE) + p->nChange++; + if (pOp->p3 > 0 && (aMem[pOp->p3].flags & MEM_Null) !=3D 0) { + assert(space->sequence !=3D NULL); + int64_t value; + if (sequence_get_value(space->sequence, &value) !=3D 0) + goto abort_due_to_error; + vdbe_add_new_autoinc_id(p, value); } - if (rc !=3D 0) - goto abort_due_to_error; break; } >=20 > if (pOp->p5 & OPFLAG_OE_IGNORE) { > /* Ignore any kind of failes and do not raise error = message */ >=20 > diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua > index a056e79..ee77192 100644 > --- a/test/sql/triggers.test.lua > +++ b/test/sql/triggers.test.lua > @@ -191,3 +191,27 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON = t1 FOR EACH ROW BEGIN SELECT 1; > box.session.su('admin') > box.schema.user.drop('tester') > box.execute("DROP TABLE t1;") > + > +-- > +-- gh-4188: Additional generated identifiers when INSERT is > +-- executed by triggers. Please, leave comments in form =E2=80=9CMake sure that =E2=80=A6 doesn=E2=80=99t appear / is fixed=E2=80=9D or sort of. It allows to = avoid confusion while reading such comments.