From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v2 2/3] sql: do not show IDs generated by trigger Date: Mon, 8 Jul 2019 21:54:37 +0300 [thread overview] Message-ID: <8C468A4E-BF2C-4820-8BBE-6E229CA5ED0D@tarantool.org> (raw) In-Reply-To: <d275de15fb14cf8ca9c8361602ed62d6d68e8aaf.1562239051.git.imeevma@gmail.com> >>> 4) I added an extra argument to the sqlInsert() function, which >>> makes it clear that the insertion was done in a trigger. >>> >>> 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. >> >> Please, support such statements with arguments. >> > 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); > > 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); > > 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; > > In addition, after the CHECK has been moved to the BOX, it works > correctly for all cases. So now it is not necessary. > > 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. > > 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: > > From 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 > > 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’ve 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 … tarantool> select * from t1 --- - metadata: - name: ID type: integer - name: A type: integer rows: [] … As you can see, insertion failed, but due to the “ignore” clause, execution wasn’t 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. > 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’t 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 == 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); > + } > } > > 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 *); > > /** > 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; > } > > -int > +static int > vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id) > { > assert(vdbe != NULL); > @@ -1150,29 +1150,30 @@ case OP_String: { /* out2 */ > } > > /* Opcode: NextAutoincValue P1 P2 * * * > - * Synopsis: r[P2] = 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’s give this opcode more suitable name: SaveAutoincValue. Also, I’ve 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); > > + pIn2 = &p->aMem[pOp->p2]; > + if ((pIn2->flags & MEM_Null) == 0) > + break; > + > struct space *space = space_by_id(pOp->p1); > if (space == NULL) > goto abort_due_to_error; > > int64_t value; > struct sequence *sequence = space->sequence; > - if (sequence == NULL || sequence_next(sequence, &value) != 0) > + if (sequence == NULL || sequence_get_value(sequence, &value) != 0) Is it possible that we added this opcode but sequence disappeared? > goto abort_due_to_error; > > - pOut = out2Prerelease(p, pOp); > - pOut->flags = MEM_Int; > - pOut->u.i = value; > + vdbe_add_new_autoinc_id(p, value); > > 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)') > > 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’) Please, add example with chained triggers.
next prev parent reply other threads:[~2019-07-08 18:54 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-04 11:42 [tarantool-patches] [PATCH v2 0/3] " imeevma 2019-07-04 11:42 ` [tarantool-patches] [PATCH v2 1/3] sql: remove unnecessary AUTOINCREMENT ID generation imeevma 2019-07-08 18:54 ` [tarantool-patches] " n.pettik 2019-07-04 11:42 ` [tarantool-patches] [PATCH v2 2/3] sql: do not show IDs generated by trigger imeevma 2019-07-08 18:54 ` n.pettik [this message] 2019-07-04 11:42 ` [tarantool-patches] [PATCH v2 3/3] sql: remove VDBE from TXN imeevma
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=8C468A4E-BF2C-4820-8BBE-6E229CA5ED0D@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v2 2/3] sql: do not show IDs generated by 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