[tarantool-patches] Re: [PATCH v2 2/3] sql: do not show IDs generated by trigger
n.pettik
korablev at tarantool.org
Mon Jul 8 21:54:37 MSK 2019
>>> 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.
More information about the Tarantool-patches
mailing list