[tarantool-patches] [PATCH v2 2/3] sql: do not show IDs generated by trigger
imeevma at tarantool.org
imeevma at tarantool.org
Thu Jul 4 14:42:11 MSK 2019
Hi! Thank you for review. My answers and new patch below.
On 7/3/19 3:25 AM, n.pettik wrote:
>
>
>> On 25 Jun 2019, at 17:09, Imeev Mergen <imeevma at tarantool.org> wrote:
>> Hi! Thank you for review! My answers below.
>> On 6/7/19 3:38 AM, n.pettik wrote:
>>>> On 29 May 2019, at 20:09, imeevma at tarantool.org wrote:
>>>>
>>>> Currently, if INSERT is executed by a trigger, the new generated
>>>> identifiers will be stored in the VDBE. This is wrong, and this
>>>> patch fixes it.
>>> Why is it so?
>> Currently, we are not able to divide the received IDS into those
>> that belong to the mentioned table and those that do not belong.
>
> Ok, now it is clear. Add explanation to the commit message.
>
Added.
>>>> Only identifiers generated during INSERT run by
>>>> the user will be saved.
>>> Firstly, I truly do not understand why this behaviour
>>> is considered to be buggy. I don’t see discussion or
>>> bug description.
>> Now this discussion can be viewed on the dev mailing list.
>>> Secondly, I do not understand what happens in this patch.
>>> Could you please explain these changes and argue why
>>> they are needed?
>> Changes in this patch:
>> 1) At first I removed changes made in patch #2981 since it isn't
>> necessary now. Due to this, all the generation of new identifiers
>> is now performed in BOX.
>> 2) I removed VDBE from TXN. So now sequence_next() does not save
>> the generated IDs in VDBE.
>> 3) I changed the way OP_NextAutoincValue works. Now it is executed
>> when there is an insert in the field with AUTOINCREMENT. If NULL
>> was inserted it receives the last generated ID using the new
>> function sequence_get_value(), and stores it in VDBE. Otherwise it
>> is a no-op.
>
> Please, split this patch into patches according to your points,
> and re-send new patch-set.
>
Done.
>> 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.
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.
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/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") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sequence.c b/src/box/sequence.c
index c9828c0..ee8a8d1 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -195,9 +195,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);
@@ -232,9 +229,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) {
@@ -347,3 +341,19 @@ 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);
+ if (pos == light_sequence_end) {
+ diag_set(ClientError, ER_SEQUENCE_NO_ELEMENTS, seq->def->name);
+ return -1;
+ }
+ 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 4419427..2acb6da 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -140,9 +140,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.
*
@@ -153,6 +150,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" */
#endif /* defined(__cplusplus) */
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.
+ */
+ if (!is_triggered && autoinc_fieldno < UINT32_MAX) {
+ sqlVdbeAddOp2(v, OP_NextAutoincValue, space->def->id,
+ regData + autoinc_fieldno);
+ }
}
/* Update the count of rows that are inserted
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 010feff..50ddd4b 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);
+ sqlInsert(pParse, X, S, F, R, false);
}
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);
+ sqlInsert(pParse, X, 0, F, R, false);
}
%type insert_cmd {int}
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/trigger.c b/src/box/sql/trigger.c
index d746ef8..32a7052 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -631,14 +631,10 @@ 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);
+ sqlInsert(pParse, targetSrcList(pParse, pStep),
+ sqlSelectDup(db, pStep->pSelect, 0),
+ sqlIdListDup(db, pStep->pIdList),
+ pParse->eOrconf, true);
break;
}
case TK_DELETE:{
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: {
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)
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/box/misc.result b/test/box/misc.result
index dab8549..0f8bcb1 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -525,6 +525,7 @@ 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/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..cc52727 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -551,3 +551,60 @@ 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 TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+ - 1
+ - 2
+ - 3
+ row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [1]
+ - [2]
+ - [3]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+ - name: I
+ type: integer
+ rows:
+ - [100]
+ - [101]
+ - [102]
+ - [103]
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t2;')
+---
+- row_count: 1
+...
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')
+box.execute('INSERT INTO t2 VALUES (100);')
+box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('DROP TABLE t1;')
+box.execute('DROP TABLE t2;')
More information about the Tarantool-patches
mailing list