[tarantool-patches] Re: [PATCH] sql: rework VIEW internals

[Vladislav Shpilevoy]

Thanks for the fixes!

I have force pushed another portion of minor ones (see below).
Now the patchset LGTM.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 8440aff64..762b99dbc 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1443,7 +1443,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
  
  /**
   * Walk through all spaces from 'FROM' clause of given select,
- * and update their view references counters.
+ * and update their view reference counters.
   *
   * @param select Tables from this select to be updated.
   * @param update_value +1 on view creation, -1 on drop.
diff --git a/src/box/sql.h b/src/box/sql.h
index fc068abbf..834f951c4 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -65,7 +65,6 @@ sql_get();
  struct Expr;
  struct Parse;
  struct Select;
-struct SrcList;
  struct Table;
  
  /**
diff --git a/test/sql/view_delayed_wal.result b/test/sql/view_delayed_wal.result
index 3ed7694ef..50efbee37 100644
--- a/test/sql/view_delayed_wal.result
+++ b/test/sql/view_delayed_wal.result
@@ -49,9 +49,10 @@ box.space.V1
  ---
  - null
  ...
+--
  -- In the same way, we have to drop all referenced spaces before
  -- dropping view, since view reference counter of space to be
--- dropped is checked before firing on_commit trigger.
+-- dropped is checked before firing on_commit trigger.
  --
  box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY)')
  ---
diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
index b8412a666..630c52baf 100644
--- a/test/sql/view_delayed_wal.test.lua
+++ b/test/sql/view_delayed_wal.test.lua
@@ -19,9 +19,10 @@ fiber.sleep(0.1)
  box.space.T1
  box.space.V1
  
+--
  -- In the same way, we have to drop all referenced spaces before
  -- dropping view, since view reference counter of space to be
--- dropped is checked before firing on_commit trigger.
+-- dropped is checked before firing on_commit trigger.
  --
  box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY)')
  box.sql.execute('CREATE VIEW view2 AS SELECT * FROM t2')


On 08/06/2018 16:17, n.pettik wrote:
> 
>> On 7 Jun 2018, at 23:06, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>>
>> Thanks for the fixes! See 1 comment below and a commit with my
>> fixes on the branch right after your. Most likely, my fixes are
>> not passing tests, so please, finish them. I fixed Select * leak on
>> exception during trigger_new_xc, and error message. + some style
>> fixes.
> 
> Thanks for fixes, I have squashed them and tests seem to pass
> (Except for 1 run: box/sql.test.lua once failed on timeout, but I’m not sure
> that it is my fault: locally on my machine it works fine both as single run and within suite).
> The only change I made - added fiber.sleep(0.1) in tests after
> box.error.injection.set("ERRINJ_WAL_DELAY", false)
> Without it test turns out to be flaky.
> 
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index 28e4d7a4d..53bb53ab8 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -2009,230 +2007,99 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>>> int
>>> -sql_view_column_names(struct Parse *parse, struct Table *table)
>>> +sql_view_assign_cursors(struct Parse *parse, const char *view_stmt)
>>> {
>>> +	assert(view_stmt != NULL);
>>> +	struct sqlite3 *db = parse->db;
>>> +	struct Select *select = sql_view_compile(db, view_stmt);
>>> 	if (select == NULL)
>>> 		return -1;
>>> -	int n = parse->nTab;
>>> 	sqlite3SrcListAssignCursors(parse, select->pSrc);
>>
>> Where do you free the struct Select?
> 
> Nowhere, shame on me..
> 
> +++ b/src/box/sql/build.c
> @@ -2090,6 +2090,7 @@ sql_view_assign_cursors(struct Parse *parse, const char *view_stmt)
>          if (select == NULL)
>                  return -1;
>          sqlite3SrcListAssignCursors(parse, select->pSrc);
> +       sql_select_delete(db, select);
> 
>