* [tarantool-patches] [PATCH v3 2/4] sql: do not show IDs generated by trigger
2019-07-11 8:22 [tarantool-patches] [PATCH v3 0/4] sql: do not show IDs generated by trigger imeevma
2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 1/4] sql: remove unnecessary AUTOINCREMENT ID generation imeevma
@ 2019-07-11 8:22 ` imeevma
2019-07-15 17:50 ` [tarantool-patches] " n.pettik
2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 3/4] sql: remove VDBE from TXN imeevma
2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed imeevma
3 siblings, 1 reply; 9+ messages in thread
From: imeevma @ 2019-07-11 8:22 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
Thank you for review. My answers, new patch and diff below.
On 7/8/19 9:54 PM, n.pettik wrote:
>
>>>> 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.
>
Is it necessary? It is not used now, and I deleted it. I described
this in a commit-message. Does it really need any other reason?
>> 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?
>
Changed commit-message.
> 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.
>
Fixed in a new patch.
>> 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:
>
Thanks, fixed.
> @@ -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);
> }
>
Thanks, fixed.
>> + 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.
>
Fixed.
> 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.
> */
>
Thanks, fixed.
>> 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?
>
No, I think. Changed to assert.
>> 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.
>
>
Added.
Diff between the patches are slightly different from the real
changes. The difference is in the changes to the comment to the
OP_SaveAutoincValue.
Diff:
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 6d22b09..be8dab2 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -250,7 +250,6 @@ 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") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sequence.c b/src/box/sequence.c
index c666972..5996519 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -369,10 +369,7 @@ sequence_get_value(struct sequence *seq, int64_t *out_value)
uint32_t key = seq->def->id;
uint32_t hash = sequence_hash(key);
uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
- if (pos == light_sequence_end) {
- diag_set(ClientError, ER_SEQUENCE_NO_ELEMENTS, seq->def->name);
- return -1;
- }
+ assert(pos != light_sequence_end);
struct sequence_data data = light_sequence_get(&sequence_data_index,
pos);
*out_value = data.value;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c331cc5..db95a02 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -233,8 +233,7 @@ 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,
- bool is_triggered)
+ enum on_conflict_action on_error)
{
sql *db; /* The main database structure */
char *zTab; /* Name of the table into which we are inserting */
@@ -740,10 +739,16 @@ 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) {
- sqlVdbeAddOp2(v, OP_NextAutoincValue, space->def->id,
+ if (autoinc_fieldno < UINT32_MAX &&
+ pParse->triggered_space == NULL) {
+ sqlVdbeAddOp2(v, OP_SaveAutoincValue, space->def->id,
regData + autoinc_fieldno);
}
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 50ddd4b..010feff 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -826,7 +826,7 @@ cmd ::= with(W) insert_cmd(R) INTO fullname(X) idlist_opt(F) select(S). {
sqlSubProgramsRemaining = SQL_MAX_COMPILING_TRIGGERS;
/* Instruct SQL to initate Tarantool's transaction. */
pParse->initiateTTrans = true;
- sqlInsert(pParse, X, S, F, R, false);
+ sqlInsert(pParse, X, S, F, R);
}
cmd ::= with(W) insert_cmd(R) INTO fullname(X) idlist_opt(F) DEFAULT VALUES.
{
@@ -834,7 +834,7 @@ cmd ::= with(W) insert_cmd(R) INTO fullname(X) idlist_opt(F) DEFAULT VALUES.
sqlSubProgramsRemaining = SQL_MAX_COMPILING_TRIGGERS;
/* Instruct SQL to initate Tarantool's transaction. */
pParse->initiateTTrans = true;
- sqlInsert(pParse, X, 0, F, R, false);
+ sqlInsert(pParse, X, 0, F, R);
}
%type insert_cmd {int}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 5dcfd34..73dc6e4 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, bool);
+ enum on_conflict_action);
void *sqlArrayAllocate(sql *, void *, int, int *, int *);
/**
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 32a7052..d746ef8 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -631,10 +631,14 @@ codeTriggerProgram(Parse * pParse, /* The parser context */
break;
}
case TK_INSERT:{
- sqlInsert(pParse, targetSrcList(pParse, pStep),
- sqlSelectDup(db, pStep->pSelect, 0),
- sqlIdListDup(db, pStep->pIdList),
- pParse->eOrconf, true);
+ sqlInsert(pParse,
+ targetSrcList(pParse, pStep),
+ sqlSelectDup(db,
+ pStep->pSelect,
+ 0),
+ sqlIdListDup(db,
+ pStep->pIdList),
+ pParse->eOrconf);
break;
}
case TK_DELETE:{
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5ecfcf4..55a8a08 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1156,7 +1156,7 @@ case OP_String: { /* out2 */
* save this value in the VDBE. If the value in register in not
* NULL than this opcode is a no-op.
*/
-case OP_NextAutoincValue: {
+case OP_SaveAutoincValue: {
assert(pOp->p1 > 0);
assert(pOp->p2 > 0);
@@ -1169,8 +1169,8 @@ case OP_NextAutoincValue: {
goto abort_due_to_error;
int64_t value;
- struct sequence *sequence = space->sequence;
- if (sequence == NULL || sequence_get_value(sequence, &value) != 0)
+ assert(space->sequence != NULL);
+ if (sequence_get_value(space->sequence, &value) != 0)
goto abort_due_to_error;
vdbe_add_new_autoinc_id(p, value);
diff --git a/test/box/misc.result b/test/box/misc.result
index 0f8bcb1..dab8549 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -525,7 +525,6 @@ t;
195: box.error.CREATE_CK_CONSTRAINT
196: box.error.CK_CONSTRAINT_FAILED
197: box.error.SQL_COLUMN_COUNT
- 198: box.error.SEQUENCE_NO_ELEMENTS
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index cc52727..8ef9648 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -563,15 +563,19 @@ box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
---
- row_count: 1
...
-box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
---
- row_count: 1
...
-box.execute('INSERT INTO t2 VALUES (100);')
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
---
- row_count: 1
...
-box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+box.execute('INSERT INTO t1 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
---
- autoincrement_ids:
- 1
@@ -585,11 +589,34 @@ box.execute('SELECT * FROM t1;')
- name: I
type: integer
rows:
+ - [100]
+ - [101]
+ - [102]
+ - [103]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
- [1]
- [2]
- [3]
...
-box.execute('SELECT * FROM t2;')
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+ - 1
+ - 2
+ - 3
+ row_count: 3
+...
+box.execute('SELECT * FROM t1;')
---
- metadata:
- name: I
@@ -599,6 +626,32 @@ box.execute('SELECT * FROM t2;')
- [101]
- [102]
- [103]
+ - [104]
+ - [105]
+ - [106]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [1]
+ - [2]
+ - [3]
+ - [4]
+ - [5]
+ - [6]
+...
+box.execute('SELECT * FROM t3;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [1]
+ - [2]
+ - [3]
...
box.execute('DROP TABLE t1;')
---
@@ -608,3 +661,7 @@ box.execute('DROP TABLE t2;')
---
- row_count: 1
...
+box.execute('DROP TABLE t3;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index 3d3b393..ee77192 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -198,10 +198,20 @@ box.execute("DROP TABLE t1;")
--
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')
-box.execute('INSERT INTO t2 VALUES (100);')
-box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+box.execute('INSERT INTO t1 VALUES (100);')
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
box.execute('SELECT * FROM t1;')
box.execute('SELECT * FROM t2;')
+
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('SELECT * FROM t3;')
+
box.execute('DROP TABLE t1;')
box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t3;')
New patch:
From 4c36c8fa80439c4900c9916dd89bac2bd8cf77f8 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. This has now been fixed
by adding a new opcode and using it to retrieve and save the newly
generated identifiers.
For example:
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')
box.execute('INSERT INTO t2 VALUES (100);')
box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
Result should be:
---
- autoincrement_ids:
- 1
- 2
- 3
row_count: 3
...
Closes #4188
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 2bf38f9..5996519 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -216,9 +216,6 @@ sequence_next(struct sequence *seq, int64_t *result)
new_data) == light_sequence_end)
return -1;
*result = def->start;
- if (txn_vdbe() != NULL &&
- vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
- return -1;
return 0;
}
old_data = light_sequence_get(&sequence_data_index, pos);
@@ -253,9 +250,6 @@ done:
new_data, &old_data) == light_sequence_end)
unreachable();
*result = value;
- if (txn_vdbe() != NULL &&
- vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
- return -1;
return 0;
overflow:
if (!def->cycle) {
@@ -368,3 +362,16 @@ sequence_data_iterator_create(void)
light_sequence_iterator_freeze(&sequence_data_index, &iter->iter);
return &iter->base;
}
+
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value)
+{
+ uint32_t key = seq->def->id;
+ uint32_t hash = sequence_hash(key);
+ uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
+ assert(pos != light_sequence_end);
+ struct sequence_data data = light_sequence_get(&sequence_data_index,
+ pos);
+ *out_value = data.value;
+ return 0;
+}
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 9a745ad..b56236e 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -157,9 +157,6 @@ sequence_next(struct sequence *seq, int64_t *result);
int
access_check_sequence(struct sequence *seq);
-int
-vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
-
/**
* Create an iterator over sequence data.
*
@@ -170,6 +167,16 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
struct snapshot_iterator *
sequence_data_iterator_create(void);
+/**
+ * Get last element of given sequence.
+ *
+ * @param seq sequence to get value from.
+ * @param[out] out_value last element of sequence.
+ * @retval 0 on success, -1 on error.
+ */
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value);
+
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d2b4e17..db95a02 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -738,6 +738,19 @@ sqlInsert(Parse * pParse, /* Parser context */
vdbe_emit_insertion_completion(v, space, regIns + 1,
space->def->field_count,
on_error);
+ /*
+ * 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 (autoinc_fieldno < UINT32_MAX &&
+ pParse->triggered_space == NULL) {
+ sqlVdbeAddOp2(v, OP_SaveAutoincValue, space->def->id,
+ regData + autoinc_fieldno);
+ }
}
/* Update the count of rows that are inserted
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..d6b32f5 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);
@@ -1149,30 +1149,32 @@ case OP_String: { /* out2 */
break;
}
-/* Opcode: NextAutoincValue P1 P2 * * *
- * Synopsis: r[P2] = next value from space sequence, which pageno is r[P1]
+/* Opcode: SaveAutoincValue P1 P2 * * *
*
- * 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 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 is not NULL than this opcode is a no-op.
*/
-case OP_NextAutoincValue: {
+case OP_SaveAutoincValue: {
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)
+ assert(space->sequence != NULL);
+ if (sequence_get_value(space->sequence, &value) != 0)
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.result b/test/sql/triggers.result
index 307b390..8ef9648 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -551,3 +551,117 @@ box.execute("DROP TABLE t1;")
---
- row_count: 1
...
+--
+-- gh-4188: Additional generated identifiers when INSERT is
+-- executed by triggers.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t1 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+ - 1
+ - 2
+ - 3
+ row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [100]
+ - [101]
+ - [102]
+ - [103]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [1]
+ - [2]
+ - [3]
+...
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+ - 1
+ - 2
+ - 3
+ row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [100]
+ - [101]
+ - [102]
+ - [103]
+ - [104]
+ - [105]
+ - [106]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [1]
+ - [2]
+ - [3]
+ - [4]
+ - [5]
+ - [6]
+...
+box.execute('SELECT * FROM t3;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [1]
+ - [2]
+ - [3]
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t2;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t3;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index a056e79..ee77192 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -191,3 +191,27 @@ 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 TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+box.execute('INSERT INTO t1 VALUES (100);')
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('SELECT * FROM t3;')
+
+box.execute('DROP TABLE t1;')
+box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t3;')
^ permalink raw reply [flat|nested] 9+ messages in thread* [tarantool-patches] [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed
2019-07-11 8:22 [tarantool-patches] [PATCH v3 0/4] sql: do not show IDs generated by trigger imeevma
` (2 preceding siblings ...)
2019-07-11 8:22 ` [tarantool-patches] [PATCH v3 3/4] sql: remove VDBE from TXN imeevma
@ 2019-07-11 8:22 ` imeevma
2019-07-11 8:52 ` [tarantool-patches] " Mergen Imeev
2019-07-15 17:59 ` n.pettik
3 siblings, 2 replies; 9+ messages in thread
From: imeevma @ 2019-07-11 8:22 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread