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 81A3725AE8 for ; Thu, 11 Jul 2019 04:22:40 -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 pHdEW-oXlM4H for ; Thu, 11 Jul 2019 04:22:40 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 41D0425AE6 for ; Thu, 11 Jul 2019 04:22:40 -0400 (EDT) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed Date: Thu, 11 Jul 2019 11:22:38 +0300 Message-Id: In-Reply-To: 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: korablev@tarantool.org Cc: tarantool-patches@freelists.org When the INSERT was executed as an INSERT OR IGNORE, it was possible to get the generated autoincrement identifiers, even if the insert 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); should return only two identifiers. After this patch, only in case of successful insertion, the generated identifiers will be returned. Also, row-count shows now number of successfull insertions in case of INSERT OR IGNORE. Follow-up #4188 --- src/box/sql/insert.c | 9 +++++++-- src/box/sql/sqlInt.h | 10 +++++++++- src/box/sql/update.c | 2 +- src/box/sql/vdbe.c | 14 +++++++++++--- test/sql/row-count.result | 26 ++++++++++++++++++++++++++ test/sql/row-count.test.lua | 7 +++++++ 6 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index db95a02..d162242 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -737,7 +737,9 @@ sqlInsert(Parse * pParse, /* Parser context */ fk_constraint_emit_check(pParse, space, 0, regIns, 0); vdbe_emit_insertion_completion(v, space, regIns + 1, space->def->field_count, - on_error); + on_error, + autoinc_fieldno < UINT32_MAX && + pParse->triggered_space == NULL); /* * Save the newly generated autoincrement value to * VDBE context. We don't need to do so for @@ -984,10 +986,13 @@ process_index: ; 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, + bool is_autoinc_present) { assert(v != NULL); u16 pik_flags = OPFLAG_NCHANGE; + if (is_autoinc_present) + pik_flags |= OPFLAG_AUTOINC; SET_CONFLICT_FLAG(pik_flags, on_conflict); sqlVdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len, raw_data_reg + tuple_len); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 73dc6e4..f668236 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2365,6 +2365,11 @@ struct Parse { #define OPFLAG_NCHANGE 0x01 /* OP_Insert: Set to update db->nChange */ /* Also used in P2 (not P5) of OP_Delete */ #define OPFLAG_EPHEM 0x01 /* OP_Column: Ephemeral output is ok */ +/* + * OP_IdxInsert, OP_IdxReplace: Inserted tuple can contain NULL in + * PK field, that will be replaced by integer using sequence. + */ +#define OPFLAG_AUTOINC 0x100 #define OPFLAG_OE_IGNORE 0x200 /* OP_IdxInsert: Ignore flag */ #define OPFLAG_OE_FAIL 0x400 /* OP_IdxInsert: Fail flag */ #define OPFLAG_OE_ROLLBACK 0x800 /* OP_IdxInsert: Rollback flag. */ @@ -3502,11 +3507,14 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr, * @param raw_data_reg Register with raw data to insert. * @param tuple_len Number of registers to hold the tuple. * @param on_conflict On conflict action. + * @param is_autoinc_present True if space has a field with + * autoincrement. */ 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, + bool is_autoinc_present); void sql_set_multi_write(Parse *, bool); diff --git a/src/box/sql/update.c b/src/box/sql/update.c index d77bee0..d249a55 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -450,7 +450,7 @@ sqlUpdate(Parse * pParse, /* The parser context */ if (on_error == ON_CONFLICT_ACTION_REPLACE) { vdbe_emit_insertion_completion(v, space, regNew, space->def->field_count, - on_error); + on_error, false); } else { int key_reg; diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 47ef0ab..884c660 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4204,8 +4204,6 @@ case OP_IdxReplace: case OP_IdxInsert: { pIn2 = &aMem[pOp->p1]; assert((pIn2->flags & MEM_Blob) != 0); - if (pOp->p5 & OPFLAG_NCHANGE) - p->nChange++; if (ExpandBlob(pIn2) != 0) goto abort_due_to_error; struct space *space; @@ -4228,8 +4226,18 @@ case OP_IdxInsert: { rc = tarantoolsqlEphemeralInsert(space, pIn2->z, pIn2->z + pIn2->n); } + if (rc == 0 && (pOp->p5 & OPFLAG_NCHANGE) != 0) + p->nChange++; - if (pOp->p5 & OPFLAG_OE_IGNORE) { + if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) { + /* + * If we ignore errors, we must skip the + * OP_SaveAutoincValue opcode, if present. This + * should be the following opcode if the space has + * a field with AUTOINCREMENT. + */ + if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0) + ++pOp; /* Ignore any kind of failes and do not raise error message */ rc = 0; /* If we are in trigger, increment ignore raised counter */ diff --git a/test/sql/row-count.result b/test/sql/row-count.result index e7841ca..522d2f1 100644 --- a/test/sql/row-count.result +++ b/test/sql/row-count.result @@ -335,3 +335,29 @@ box.execute("DROP TABLE t1;") --- - row_count: 1 ... +-- +-- gh-4188: check that generated IDs is not showed for failed +-- insertions in case of INSERT OR IGNORE. +-- +box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));") +--- +- row_count: 1 +... +box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);") +--- +- autoincrement_ids: + - 1 + - 3 + row_count: 2 +... +box.execute("SELECT * FROM t;") +--- +- metadata: + - name: I + type: integer + - name: A + type: integer + rows: + - [1, 1] + - [3, 2] +... diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua index 9f5215c..30de8b4 100644 --- a/test/sql/row-count.test.lua +++ b/test/sql/row-count.test.lua @@ -73,3 +73,10 @@ box.execute("DROP TABLE t2;") box.execute("DROP TABLE t3;") box.execute("DROP TABLE t1;") +-- +-- gh-4188: check that generated IDs is not showed for failed +-- insertions in case of INSERT OR IGNORE. +-- +box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));") +box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);") +box.execute("SELECT * FROM t;") -- 2.7.4