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 ADBAA24F26 for ; Thu, 11 Jul 2019 04:52:56 -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 zKYsB83WdBYX for ; Thu, 11 Jul 2019 04:52:56 -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 1173324E2D for ; Thu, 11 Jul 2019 04:52:55 -0400 (EDT) Date: Thu, 11 Jul 2019 11:52:52 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed Message-ID: <20190711085252.GA8085@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 Sorry, forgot to drop table in test. Fixed here and on branch. Diff and new patch below. Diff: diff --git a/test/sql/row-count.result b/test/sql/row-count.result index 522d2f1..69bfb78 100644 --- a/test/sql/row-count.result +++ b/test/sql/row-count.result @@ -361,3 +361,7 @@ box.execute("SELECT * FROM t;") - [1, 1] - [3, 2] ... +box.execute("DROP TABLE t;") +--- +- row_count: 1 +... diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua index 30de8b4..965408e 100644 --- a/test/sql/row-count.test.lua +++ b/test/sql/row-count.test.lua @@ -80,3 +80,4 @@ box.execute("DROP TABLE t1;") 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;") +box.execute("DROP TABLE t;") New patch: >From 988b155477f12ed27cf20edf14a6a88d6f995a2b Mon Sep 17 00:00:00 2001 From: Mergen Imeev Date: Wed, 10 Jul 2019 12:56:16 +0300 Subject: [PATCH] sql: do not show generated IDs if INSERT failed 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 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..69bfb78 100644 --- a/test/sql/row-count.result +++ b/test/sql/row-count.result @@ -335,3 +335,33 @@ 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] +... +box.execute("DROP TABLE t;") +--- +- row_count: 1 +... diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua index 9f5215c..965408e 100644 --- a/test/sql/row-count.test.lua +++ b/test/sql/row-count.test.lua @@ -73,3 +73,11 @@ 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;") +box.execute("DROP TABLE t;")