[Tarantool-patches] [PATCH v1 1/1] sql: fix INSTEAD OF DELETE trigger for VIEW

[Tarantool-patches] [PATCH v1 1/1] sql: fix INSTEAD OF DELETE trigger for VIEW

[imeevma]

This patch makes the INSTEAD OF DELETE trigger work for every row
in VIEW. Prior to this patch, it worked only once for each group
of non-unique rows.

Also, this patch adds tests to check that the INSTEAD OF UPDATE
trigger work for every row in VIEW.

Closes #4740
---
https://github.com/tarantool/tarantool/issues/4740
https://github.com/tarantool/tarantool/tree/imeevma/gh-4740-fix-instead-of-delete-trigger

 src/box/sql/delete.c   |  2 +-
 test/sql/view.result   | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/view.test.lua | 18 ++++++++++++
 3 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 8d9ae4f..e9edbcc 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -231,7 +231,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		int eph_cursor = parse->nTab++;
 		int addr_eph_open = sqlVdbeCurrentAddr(v);
 		if (is_view) {
-			pk_len = space->def->field_count;
+			pk_len = space->def->field_count + 1;
 			parse->nMem += pk_len;
 			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
 					  pk_len);
diff --git a/test/sql/view.result b/test/sql/view.result
index f3b4da1..1e73ff6 100644
--- a/test/sql/view.result
+++ b/test/sql/view.result
@@ -321,3 +321,82 @@ box.execute("DROP TABLE t1;");
 ---
 - row_count: 1
 ...
+--
+-- gh-4740: make sure INSTEAD OF DELETE and INSTEAD OF UPDATE
+-- triggers work for each row of view.
+--
+box.cfg{}
+---
+...
+box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT, a INT);')
+---
+- row_count: 1
+...
+box.execute('CREATE VIEW v AS SELECT a FROM t;')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r1 INSTEAD OF DELETE ON v FOR EACH ROW BEGIN INSERT INTO t1 VALUES (NULL, 1); END;')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r2 INSTEAD OF UPDATE ON v FOR EACH ROW BEGIN INSERT INTO t1 VALUES (NULL, 2); END;')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t VALUES (NULL, 1), (NULL, 1), (NULL, 1), (NULL, 2), (NULL, 3), (NULL, 3);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  - 4
+  - 5
+  - 6
+  row_count: 6
+...
+box.execute('DELETE FROM v;')
+---
+- row_count: 0
+...
+box.execute('UPDATE v SET a = 10;')
+---
+- row_count: 0
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  - name: A
+    type: integer
+  rows:
+  - [1, 1]
+  - [2, 1]
+  - [3, 1]
+  - [4, 1]
+  - [5, 1]
+  - [6, 1]
+  - [7, 2]
+  - [8, 2]
+  - [9, 2]
+  - [10, 2]
+  - [11, 2]
+  - [12, 2]
+...
+box.execute('DROP VIEW v;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/view.test.lua b/test/sql/view.test.lua
index 76ea303..47ca726 100644
--- a/test/sql/view.test.lua
+++ b/test/sql/view.test.lua
@@ -120,3 +120,21 @@ box.space.T2:drop()
 -- Cleanup
 box.execute("DROP VIEW v1;");
 box.execute("DROP TABLE t1;");
+
+--
+-- gh-4740: make sure INSTEAD OF DELETE and INSTEAD OF UPDATE
+-- triggers work for each row of view.
+--
+box.cfg{}
+box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT);')
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT, a INT);')
+box.execute('CREATE VIEW v AS SELECT a FROM t;')
+box.execute('CREATE TRIGGER r1 INSTEAD OF DELETE ON v FOR EACH ROW BEGIN INSERT INTO t1 VALUES (NULL, 1); END;')
+box.execute('CREATE TRIGGER r2 INSTEAD OF UPDATE ON v FOR EACH ROW BEGIN INSERT INTO t1 VALUES (NULL, 2); END;')
+box.execute('INSERT INTO t VALUES (NULL, 1), (NULL, 1), (NULL, 1), (NULL, 2), (NULL, 3), (NULL, 3);')
+box.execute('DELETE FROM v;')
+box.execute('UPDATE v SET a = 10;')
+box.execute('SELECT * FROM t1;')
+box.execute('DROP VIEW v;')
+box.execute('DROP TABLE t;')
+box.execute('DROP TABLE t1;')
-- 
2.7.4

Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix INSTEAD OF DELETE trigger for VIEW

[Nikita Pettik]

On 22 Jan 16:54, imeevma@tarantool.org wrote:
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 8d9ae4f..e9edbcc 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -231,7 +231,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>  		int eph_cursor = parse->nTab++;
>  		int addr_eph_open = sqlVdbeCurrentAddr(v);
>  		if (is_view) {
> -			pk_len = space->def->field_count;
> +			pk_len = space->def->field_count + 1;
>  			parse->nMem += pk_len;
>  			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,

Please, add comments explaining what's going on here. Despite the seeming
simplicity, this change is way far from being obvious. This time I've added
comment myself:

+                       /*
+                        * At this stage SELECT is already materialized
+                        * into ephemeral table, which has one additional
+                        * tail field (except for ones specified in view
+                        * format) which contains sequential ids. These ids
+                        * are required since selected values may turn out to
+                        * be non-unique. For instance:
+                        * CREATE TABLE t (id INT PRIMARY KEY, a INT);
+                        * INSERT INTO t VALUES (1, 1), (2, 1), (3, 1);
+                        * CREATE VIEW v AS SELECT a FROM t;
+                        * Meanwhile ephemeral tables feature PK covering ALL
+                        * fields of format, so without id materialization
+                        * processing is impossible. Then, results of
+                        * materialization are transferred to the table being
+                        * created below. So to fit tuples in it we must
+                        * account that id field as well. That's why pk_len
+                        * has one field more than view format.
+                        */

Next time I'll reject such patches.

Pushed to master, 2.3 and 2.2.