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 D5832216C9 for ; Mon, 8 Jul 2019 14:54:39 -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 XtYR5Aor6HfL for ; Mon, 8 Jul 2019 14:54:39 -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 7550B20976 for ; Mon, 8 Jul 2019 14:54:39 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v2 2/3] sql: do not show IDs generated by trigger From: "n.pettik" In-Reply-To: Date: Mon, 8 Jul 2019 21:54:37 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8C468A4E-BF2C-4820-8BBE-6E229CA5ED0D@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 >>> 4) I added an extra argument to the sqlInsert() function, which >>> makes it clear that the insertion was done in a trigger. >>>=20 >>> My point about these changes: >>> 1 - I think it should be done anyway. >>> 2 - It doesn't look like VDBE should be stored in TXN. >>=20 >> Please, support such statements with arguments. >>=20 > 1 - it worked only for cases like: > CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, = CHECK (s1 <> 19)); > INSERT INTO t1 VALUES (18, NULL); > INSERT INTO t1 (s2) VALUES (NULL); >=20 > But it doesn't worked for these cases: > CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, = CHECK (s1 <> 19)); > INSERT INTO t1 VALUES (18, NULL); > INSERT INTO t1 VALUES (NULL, NULL); >=20 > CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, = CHECK (s1 <> 19)); > INSERT INTO t1 VALUES (18, NULL); > INSERT INTO t1 SELECT NULL, NULL FROM t1; >=20 > In addition, after the CHECK has been moved to the BOX, it works > correctly for all cases. So now it is not necessary. >=20 > However, I can actually leave it at that. Then I need to create a > new opcode if I want to solve #4188 using my solution. >=20 > 2 - sorry i can't say exactly why. But Gosha told me about it when > I asked him verbally, and Kostja O. and Gosha mentioned it in the > server chat. I think this can be found in messages for April or > March. Please investigate subj and add motivation to the commit message. > New patch: >=20 > =46rom d275de15fb14cf8ca9c8361602ed62d6d68e8aaf Mon Sep 17 00:00:00 = 2001 > Date: Tue, 28 May 2019 17:41:15 +0300 > Subject: [PATCH] sql: do not show IDs generated by trigger >=20 > Currently, if an INSERT was executed by a trigger during the > execution of a statement, if there were any generated identifiers, > they can be displayed as a result of the statement. This is > incorrect, since we are not able to divide the IDs obtained into > those that belong to the table mentioned in the statement and > those that do not belong to this table. So, how did you fix it? I=E2=80=99ve noticed following behaviour: tarantool> create table t1(id int primary key autoincrement, a int check = (a > 0)) --- - row_count: 1 ... tarantool> insert or ignore into t1 values(null, -1) --- - autoincrement_ids: - 1 row_count: 1 =E2=80=A6 tarantool> select * from t1 --- - metadata: - name: ID type: integer - name: A type: integer rows: [] =E2=80=A6 As you can see, insertion failed, but due to the =E2=80=9Cignore=E2=80=9D = clause, execution wasn=E2=80=99t stopped and OP_NextAutoincValue was processed. Please, consider this case in your patch-set. > diff --git a/src/box/errcode.h b/src/box/errcode.h > index be8dab2..6d22b09 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -250,6 +250,7 @@ struct errcode_record { > /*195 */_(ER_CREATE_CK_CONSTRAINT, "Failed to create check = constraint '%s': %s") \ > /*196 */_(ER_CK_CONSTRAINT_FAILED, "Check constraint failed = '%s': %s") \ > /*197 */_(ER_SQL_COLUMN_COUNT, "Unequal number of = entries in row expression: left side has %u, but right side - %u") \ > + /*198 */_(ER_SEQUENCE_NO_ELEMENTS, "Sequence '%s' has no = elements") \ Could you please explain what does it mean? I see that this error is not tested at all. =20 > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index d2b4e17..c331cc5 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -233,7 +233,8 @@ sqlInsert(Parse * pParse, /* Parser context */ > SrcList * pTabList, /* Name of table into which we = are inserting */ > Select * pSelect, /* A SELECT statement to use as the data = source */ > IdList * pColumn, /* Column names corresponding to IDLIST. = */ > - enum on_conflict_action on_error) > + enum on_conflict_action on_error, > + bool is_triggered) > { > sql *db; /* The main database structure */ > char *zTab; /* Name of the table into which we are = inserting */ > @@ -738,6 +739,13 @@ sqlInsert(Parse * pParse, /* Parser = context */ > vdbe_emit_insertion_completion(v, space, regIns + 1, > space->def->field_count, > on_error); > + /* > + * Save the newly autogenerated value to VDBE. > + */ Extended comment: @@ -740,9 +739,14 @@ sqlInsert(Parse * pParse, /* Parser context */ space->def->field_count, on_error); /* - * Save the newly autogenerated value to VDBE. + * Save the newly generated autoincrement value to + * VDBE context. We don't need to do so for triggers + * in order to avoid mess of incremented ids. After + * VDBE execution is finished, list of autoincrement + * values is returned as a result of query execution. */ > + if (!is_triggered && autoinc_fieldno < UINT32_MAX) { It is likely that you don=E2=80=99t need this flag: you can check = triggered_space field in parsing context: @@ -742,7 +741,8 @@ sqlInsert(Parse * pParse, /* Parser context */ /* * Save the newly autogenerated value to VDBE. */ - if (!is_triggered && autoinc_fieldno < UINT32_MAX) { + if (pParse->triggered_space =3D=3D NULL && + autoinc_fieldno < UINT32_MAX) { sqlVdbeAddOp2(v, OP_NextAutoincValue, = space->def->id, regData + autoinc_fieldno); } > + sqlVdbeAddOp2(v, OP_NextAutoincValue, = space->def->id, > + regData + autoinc_fieldno); > + } > } >=20 > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 73dc6e4..5dcfd34 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3003,7 +3003,7 @@ sql_store_select(struct Parse *parse_context, = struct Select *select); > void > sql_drop_table(struct Parse *); > void sqlInsert(Parse *, SrcList *, Select *, IdList *, > - enum on_conflict_action); > + enum on_conflict_action, bool); > void *sqlArrayAllocate(sql *, void *, int, int *, int *); >=20 > /** > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index c8887f9..5ecfcf4 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); > @@ -1150,29 +1150,30 @@ case OP_String: { /* out2 */ > } >=20 > /* Opcode: NextAutoincValue P1 P2 * * * > - * Synopsis: r[P2] =3D next value from space sequence, which pageno = is r[P1] > * > - * Get next value from space sequence, which pageno is written into = register > - * P1, write this value into register P2. If space doesn't exists = (invalid > - * space_id or something else), raise an error. If space with > - * specified space_id doesn't have attached sequence, also raise an = error. > + * If the value in the register P2 is NULL, get the last value > + * from the sequence that belongs to the space with id P1, and > + * save this value in the VDBE. If the value in register in not > + * NULL than this opcode is a no-op. > */ > case OP_NextAutoincValue: { Let=E2=80=99s give this opcode more suitable name: SaveAutoincValue. Also, I=E2=80=99ve extended comment: @@ -1152,11 +1152,12 @@ case OP_String: { /* out2 */ /* Opcode: NextAutoincValue P1 P2 * * * * * If the value in the register P2 is NULL, get the last value - * from the sequence that belongs to the space with id P1, and - * save this value in the VDBE. If the value in register in not - * NULL than this opcode is a no-op. + * from the sequence that attached to the space with id P1, and + * save this value to the VDBE. At the end of execution, mentioned + * list is returned as a result of query execution. If the value in + * register in not NULL than this opcode is a no-op. */ > assert(pOp->p1 > 0); > assert(pOp->p2 > 0); >=20 > + pIn2 =3D &p->aMem[pOp->p2]; > + if ((pIn2->flags & MEM_Null) =3D=3D 0) > + break; > + > struct space *space =3D space_by_id(pOp->p1); > if (space =3D=3D NULL) > goto abort_due_to_error; >=20 > int64_t value; > struct sequence *sequence =3D space->sequence; > - if (sequence =3D=3D NULL || sequence_next(sequence, &value) !=3D = 0) > + if (sequence =3D=3D NULL || sequence_get_value(sequence, &value) = !=3D 0) Is it possible that we added this opcode but sequence disappeared? > goto abort_due_to_error; >=20 > - pOut =3D out2Prerelease(p, pOp); > - pOut->flags =3D MEM_Int; > - pOut->u.i =3D value; > + vdbe_add_new_autoinc_id(p, value); >=20 > break; > } > --- > diff --git a/test/sql/iproto.result b/test/sql/iproto.result > index 9639ba7..3580d6b 100644 > --- a/test/sql/iproto.result > +++ b/test/sql/iproto.result > @@ -552,8 +552,6 @@ cn:execute('insert into test values (null, 1)') > --- > - autoincrement_ids: > - 127 > - - 1 > - - 2 > row_count: 1 > ... > box.execute('create table test3 (id int primary key autoincrement)') >=20 > diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua > index a056e79..3d3b393 100644 > --- a/test/sql/triggers.test.lua > +++ b/test/sql/triggers.test.lua > @@ -191,3 +191,17 @@ 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. > +-- > +box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);') > +box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);') > +box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN = INSERT INTO t2 VALUES (null); END=E2=80=99) Please, add example with chained triggers.