From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed Date: Mon, 15 Jul 2019 20:59:32 +0300 [thread overview] Message-ID: <D3C82E73-63E5-4438-9239-BF17B3143377@tarantool.org> (raw) In-Reply-To: <e916e024f1832a78777523b39725df812626592c.1562832978.git.imeevma@gmail.com> I’ve edited commit message: sql: omit auto-inc ID if INSERT OR IGNORE failed If INSERT statement is executed with IGNORE error action (i.e. INSERT OR IGNORE ...), it will return generated autoincrement identifiers. Even in case the insert has failed. For example: CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0)); INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2); In this case only two identifiers should be returned, but before this patch all three are appeared. So, let's account only identifiers which are really inserted to the space. Also, row-count shows now number of successful insertions in case of INSERT OR IGNORE. Follow-up #4188 TBO I dislike your approach for several reasons. Firstly, it introduces another one auxiliary flag, which enlarges branching, complicates code reading etc. Secondly, it changes program counter in OP_Insert: generally speaking, I’d not consider implicit incrementation of program counter to be a good practice. You even don’t check that the next instruction is OP_SaveAutoinc… Could you spend some more time trying come up with another solution? Also, I wonder why didn’t you split OP_SaveAutoinc into two opcode: OP_IsNull + OP_SaveAutoinc which would unconditionally save autroincremented value to the vdbe? Obvious diff: diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index d1622428c..e9a9c3afc 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -735,11 +735,11 @@ sqlInsert(Parse * pParse, /* Parser context */ vdbe_emit_constraint_checks(pParse, space, regIns + 1, on_error, endOfLoop, 0); fk_constraint_emit_check(pParse, space, 0, regIns, 0); + bool has_autoinc_field = autoinc_fieldno < UINT32_MAX && + pParse->triggered_space == NULL; vdbe_emit_insertion_completion(v, space, regIns + 1, space->def->field_count, - on_error, - autoinc_fieldno < UINT32_MAX && - pParse->triggered_space == NULL); + on_error, has_autoinc_field); /* * Save the newly generated autoincrement value to * VDBE context. We don't need to do so for @@ -748,8 +748,7 @@ sqlInsert(Parse * pParse, /* Parser context */ * autoincrement values is returned as a result of * query execution. */ - if (autoinc_fieldno < UINT32_MAX && - pParse->triggered_space == NULL) { + if (has_autoinc_field) { sqlVdbeAddOp2(v, OP_SaveAutoincValue, space->def->id, regData + autoinc_fieldno); } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 884c6605c..366f1f989 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4236,8 +4236,10 @@ case OP_IdxInsert: { * should be the following opcode if the space has * a field with AUTOINCREMENT. */ - if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0) + if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0) { ++pOp; + assert(pOp->opcode == OP_SaveAutoincValue); + } /* Ignore any kind of failes and do not raise error message */ rc = 0;
prev parent reply other threads:[~2019-07-15 17:59 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-11 8:22 [tarantool-patches] [PATCH v3 0/4] sql: do not show IDs generated by trigger imeevma 2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 1/4] sql: remove unnecessary AUTOINCREMENT ID generation imeevma 2019-07-15 17:50 ` [tarantool-patches] " n.pettik 2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 2/4] sql: do not show IDs generated by trigger imeevma 2019-07-15 17:50 ` [tarantool-patches] " n.pettik 2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 3/4] sql: remove VDBE from TXN imeevma 2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed imeevma 2019-07-11 8:52 ` [tarantool-patches] " Mergen Imeev 2019-07-15 17:59 ` n.pettik [this message]
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=D3C82E73-63E5-4438-9239-BF17B3143377@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed' \ /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