From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger Date: Wed, 17 Jul 2019 19:58:43 +0300 [thread overview] Message-ID: <742F5B6B-D414-41DF-A0CF-342F13A4C5F0@tarantool.org> (raw) In-Reply-To: <c6ce7d0b81b5e93b87b2505ffbfda9e9c338b309.1563356272.git.imeevma@gmail.com> > 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 */ > } > } > > + assert(regData > 0); Pretty much useless assertion here. > + int autoinc_reg = 0; > + if (autoinc_fieldno < UINT32_MAX && > + pParse->triggered_space == NULL) > + autoinc_reg = 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; > } > > -int > +static int > vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id) > { > assert(vdbe != NULL); > @@ -4161,12 +4161,17 @@ case OP_SorterInsert: { /* in2 */ > break; > } > > -/* Opcode: IdxInsert P1 P2 * P4 P5 > +/* Opcode: IdxInsert P1 P2 P3 P4 P5 > * Synopsis: key=r[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 = tarantoolsqlEphemeralInsert(space, pIn2->z, > pIn2->z + pIn2->n); > } > + if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) { > + assert(space->sequence != NULL); > + int64_t value; > + if (sequence_get_value(space->sequence, &value) != 0) > + goto abort_due_to_error; > + vdbe_add_new_autoinc_id(p, value); > + } I’d 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 = tarantoolsqlEphemeralInsert(space, pIn2->z, pIn2->z + pIn2->n); } - if (rc == 0) { - if (pOp->p5 & OPFLAG_NCHANGE) - p->nChange++; - if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) { - assert(space->sequence != NULL); - int64_t value; - if (sequence_get_value(space->sequence, &value) != 0) - goto abort_due_to_error; - vdbe_add_new_autoinc_id(p, value); + if (rc != 0) { + if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) { + /* + * Ignore any kind of fails and do not + * raise error message. + */ + rc = 0; + /* + * If we are in trigger, increment ignore + * raised counter. + */ + if (p->pFrame) + p->ignoreRaised++; + break; } + if (pOp->p5 & OPFLAG_OE_FAIL) + p->errorAction = ON_CONFLICT_ACTION_FAIL; + else if (pOp->p5 & OPFLAG_OE_ROLLBACK) + p->errorAction = 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 = 0; - /* If we are in trigger, increment ignore raised counter */ - if (p->pFrame) - p->ignoreRaised++; - } else if (pOp->p5 & OPFLAG_OE_FAIL) { - p->errorAction = ON_CONFLICT_ACTION_FAIL; - } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) { - p->errorAction = ON_CONFLICT_ACTION_ROLLBACK; + if (pOp->p5 & OPFLAG_NCHANGE) + p->nChange++; + if (pOp->p3 > 0 && (aMem[pOp->p3].flags & MEM_Null) != 0) { + assert(space->sequence != NULL); + int64_t value; + if (sequence_get_value(space->sequence, &value) != 0) + goto abort_due_to_error; + vdbe_add_new_autoinc_id(p, value); } - if (rc != 0) - goto abort_due_to_error; break; } > > if (pOp->p5 & OPFLAG_OE_IGNORE) { > /* Ignore any kind of failes and do not raise error message */ > > 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 “Make sure that … doesn’t appear / is fixed” or sort of. It allows to avoid confusion while reading such comments.
next prev parent reply other threads:[~2019-07-17 16:58 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-17 9:54 [tarantool-patches] [PATCH v4 0/4] sql: do not show IDs generated by trigger imeevma 2019-07-17 9:54 ` [tarantool-patches] [PATCH v4 1/4] sql: remove unnecessary AUTOINCREMENT ID generation imeevma 2019-07-17 9:54 ` [tarantool-patches] [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger imeevma 2019-07-17 16:58 ` n.pettik [this message] 2019-07-19 9:33 ` [tarantool-patches] " Mergen Imeev 2019-07-17 9:54 ` [tarantool-patches] [PATCH v4 3/4] sql: remove VDBE from TXN imeevma 2019-07-17 9:54 ` [tarantool-patches] [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed imeevma 2019-07-17 16:57 ` [tarantool-patches] " n.pettik 2019-07-17 18:08 ` [tarantool-patches] " Мерген Имеев 2019-07-19 9:36 ` Mergen Imeev 2019-07-22 10:48 ` n.pettik 2019-07-22 11:26 ` Mergen Imeev 2019-07-24 13:55 ` [tarantool-patches] Re: [PATCH v4 0/4] sql: do not show IDs generated by trigger Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=742F5B6B-D414-41DF-A0CF-342F13A4C5F0@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox