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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the patch!
> Firstly, I don't like code duplication in view on_commit/on_rollback
> triggers. But I didn't manage to find any 'satisfactory' solution,
> except for adding auxilary function with enum/bool arg which in turn
> will determine action on view reference counter.

I think it is better than code duplication. Lets do special function (maybe
with another name)

sql_view_update_references(Select *, int update_value)
{
	expand_names()
	for (int i = 0; i < select->pSrc->nSrc; ++i) {
		const char *space_name = select->pSrc->a[i].zName;
		if (space_name == NULL)
			continue;
		uint32_t space_id = schema_find_id(BOX_SPACE_ID, 0, space_name,
						   strlen(space_name));
		assert(space_id != BOX_ID_NIL);
		space = space_by_id(space_id);
		space->def->view_ref_count += update_value;
	}
}

on_drop/create_commit just do update_references(+1/-1) + delete.

> Secondly, (I guess) including in alter.cc sqliteInt.h header seems to be
> total mess, but again - I failed to come up with kind of sinsible> resolution. Is it time for dividing SQL code base into source/header files?

Yes, sqliteInt.h in alter.cc looks ugly. Just implement temporary
getters/setters taking Select * and defined in sql.h. Look for example on
Expr * - it is used in alter.cc, but does not require include anything
except sql.h.

Also maybe the suggestion above can help you. You can declare
sql_view_update_references in sql.h, and do not access
select->pSrc->a[i].zName directly in alter.cc.

> At least, including "select.h" looks less annoying.
> Finally, I temporary changed system space in box_space_id_by_name()
> from _vspace to _space, in order to find space by name in on_dd_replace trigger.

_vspace is needed in box_space_id_by_name because it is called by external API
and hides from the user the spaces he has no access. So _space is not acceptable
here.

> I have noticed that Kirill Sh. has almost done the same thing, so this
> hack is going to be substituted with his patch.

In alter.cc you can use schema_find_id(). Kirill in his patch was not able to use
this function because it throws exceptions, but alter.cc is C++.

See my 22 comments and recommendations below.

1. Travis is failed on the branch on box/func_reload. It is very strange, but
on the previous commit on your branch it is not failed.

> 
>  src/box/alter.cc               |  84 ++++++++++++
>  src/box/box.cc                 |   4 +-
>  src/box/space_def.c            |   1 +
>  src/box/space_def.h            |   2 +
>  src/box/sql.h                  |  47 +++++++
>  src/box/sql/build.c            | 289 ++++++++++++-----------------------------
>  src/box/sql/delete.c           |   6 +-
>  src/box/sql/expr.c             |   5 +-
>  src/box/sql/fkey.c             |   4 +-
>  src/box/sql/insert.c           |   4 +-
>  src/box/sql/parse.y            |  19 +--
>  src/box/sql/select.c           |  61 +++++++--
>  src/box/sql/sqliteInt.h        |  31 ++++-
>  src/box/sql/tokenize.c         |  18 +++
>  src/box/sql/trigger.c          |   8 +-
>  src/box/sql/vdbe.c             |  24 ++++
>  test/sql-tap/colname.test.lua  |   4 +-
>  test/sql-tap/drop_all.test.lua |   4 +-
>  test/sql-tap/fkey2.test.lua    |   1 +
>  test/sql-tap/trigger1.test.lua |   1 +
>  test/sql-tap/triggerC.test.lua |   1 +
>  test/sql-tap/view.test.lua     | 137 ++++++++++++++-----
>  test/sql/view.result           |  23 +++-
>  test/sql/view.test.lua         |  11 +-
>  24 files changed, 502 insertions(+), 287 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index b62f8adea..1fbb54d1a 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -29,6 +29,7 @@
>   * SUCH DAMAGE.
>   */
>  #include "alter.h"
> +#include "box.h"

2. As I said above, you can remove box.h. It must not be included
anywhere in internal files when possible. So consider this diff,
please.

After this fix and after others I have not run tests. So please
check them before applying. If they fail after me, then sorry %)

@@ -29,7 +29,6 @@
   * SUCH DAMAGE.
   */
  #include "alter.h"
-#include "box.h"
  #include "schema.h"
@@ -1465,8 +1464,8 @@ on_create_view_commit(struct trigger *trigger, void *event)
                 const char *space_name = select->pSrc->a[i].zName;
                 if (space_name == NULL)
                         continue;
-               uint32_t space_id = box_space_id_by_name(space_name,
-                                                        strlen(space_name));
+               uint32_t space_id = schema_find_id(BOX_SPACE_ID, 2, space_name,
+                                                  strlen(space_name));
                 assert(space_id != BOX_ID_NIL);
@@ -1497,8 +1496,8 @@ on_drop_view_commit(struct trigger *trigger, void *event)
                 const char *space_name = select->pSrc->a[i].zName;
                 if (space_name == NULL)
                         continue;
-               uint32_t space_id = box_space_id_by_name(space_name,
-                                                        strlen(space_name));
+               uint32_t space_id = schema_find_id(BOX_SPACE_ID, 2, space_name,
+                                                  strlen(space_name));
                 assert(space_id != BOX_ID_NIL);


> @@ -1440,6 +1442,70 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
>  	}
>  }
>  
> +/**
> + * Trigger which is fired on creation of new SQL view.
> + * Its purpose is to increment view reference counters of
> + * dependent spaces.
> + */
> +static void
> +on_create_view_commit(struct trigger *trigger, void *event)
> +{
> +	(void) event;
> +	struct space *space = (struct space *)trigger->data;
> +	struct space_def *def = space->def;
> +	struct Select *select;
> +	if (sql_view_compile(sql_get(), def->opts.sql, &select) != 0) {
> +		diag_log();> +		return;

3. On commit trigger must never fail. So please, do compile in
on_replace_dd_space, and here just use it. I propose this.

- Compile sql in on_replace_dd_space;
- Put compiled Select * in trigger->data (as well as you pass space * now);
- In on_drop/create_view_commit you take Select * from trigger data. Space *
   as I can see is used in these functions to take def->sql and create
   Select * only so it is ok to replace it with already compiled Select *.

Then do not forget to add on_rollback triggers to delete Select *.

> @@ -1575,6 +1652,13 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
>  		struct trigger *on_commit =
>  			txn_alter_trigger_new(on_drop_space_commit, space);
>  		txn_on_commit(txn, on_commit);
> +		if (old_space->def->opts.is_view &&
> +		    old_space->def->opts.sql != NULL) {

4. How is it possible that is_view == true, but sql == NULL?

>               if (def->opts.is_view != old_space->def->opts.is_view)
> 			tnt_raise(ClientError, ER_ALTER_SPACE,
> 				  space_name(old_space),
> 				  "can not convert a space to "
> 				  "a view and vice versa");

5. Please, use {} for 'if' body consisting of multiple lines.

6. I do not see where do you set def->view_ref_count on space alter.
See this test:

     tarantool> box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY);")
     tarantool> box.sql.execute("CREATE VIEW v1 AS SELECT * FROM t1")
     tarantool> box.sql.execute("DROP TABLE t1")
     ---
     - error: 'Can''t drop table T1: other views depend on this space'
     ...

It is ok. But then I do this:

     tarantool> s = box.space._space:get{box.space.T1.id}
     tarantool> box.space._space:replace(s)

Nothing is really changed, only space * and space_def * are
recreated. But now I am able to do this:

     tarantool> box.sql.execute("DROP TABLE t1")
     ---
     ...

You need to update class ModifySpace to move view refs from the
old to the new def.

> diff --git a/src/box/sql.h b/src/box/sql.h
> index 23021e56b..9a006e5cb 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -83,6 +83,19 @@ int
>  sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len,
>  		 struct Expr **result);
>  
> +/**
> + * This routine executes parser on 'CREATE VIEW ...' statement
> + * and loads content of SELECT into internal structs as result.
> + *
> + * @param db Current SQL context.
> + * @param view_stmt String containing 'CREATE VIEW' statement.
> + * @param[out] select Fetched SELECT statement.
> + * @retval 0 on success, -1 otherwise.
> + */
> +int
> +sql_view_compile(struct sqlite3 *db, const char *view_stmt,
> +		 struct Select **select);

7. You need an out parameter only when returned value is occupied for
another output. Here it is enough to just return Select * on success
and NULL on error. I see that sql_expr_compile uses out parameter but
it is wrong too. Kirill in his patch fixes it.
> @@ -293,6 +306,40 @@ sql_parser_create(struct Parse *parser, struct sqlite3 *db);
> +/**
> + * Work the same as sqlite3SrcListAppend(), but before adding to
> + * list provide check on name duplicates: only values with unique
> + * names are appended.
> + *
> + * @param db Database handler.
> + * @param list List of entries.
> + * @param new_name Name of entity to be added.
> + * @retval @list with new element on success, old one otherwise.
> + */
> +struct SrcList *
> +sql_src_list_append_unique(struct sqlite3 *db, struct SrcList *list,
> +			   const char *new_name);

8. Do you really need this declaration in sql.h? It is used in select.c
only. Maybe it can be static inside this file?

> +
> +/**
> + * Expand all spaces names from 'FROM' clause, including
> + * ones from subqueries, and add those names to the original
> + * select.
> + *
> + * @param select Select to be expanded.
> + */
> +void
> +sql_select_expand_from_tables(struct Select *select);

9. If you agree with my suggestion to create sql_view_update_references(),
you can move this function into select.c together with the former and
make it static.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 28e4d7a4d..053cf83b7 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2009,72 +2007,74 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>  }
>  
>  #ifndef SQLITE_OMIT_VIEW

10. Maybe it is worth to delete SQLITE_OMIT_VIEW alongside?

> -/*
> - * The parser calls this routine in order to create a new VIEW
> - */
>  void
> -sqlite3CreateView(Parse * pParse,	/* The parsing context */
> -		  Token * pBegin,	/* The CREATE token that begins the statement */
> -		  Token * pName,	/* The token that holds the name of the view */
> -		  ExprList * pCNames,	/* Optional list of view column names */
> -		  Select * pSelect,	/* A SELECT statement that will become the new view */
> -		  int noErr	/* Suppress error messages if VIEW already exists */
> -    )
> +sql_create_view(struct Parse *parse_context, struct Token *begin,
> +		struct Token *name, struct ExprList *aliases,
> +		struct Select *select, bool if_exists)

11. 'if_exists' in build.c, but 'is_exists' in sqliteInt.h.

12. Please, consider this diff:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 053cf83b7..935054540 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2020,16 +2020,16 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
         }
         sqlite3StartTable(parse_context, name, if_exists);
         struct Table *p = parse_context->pNewTable;
-       if (p == NULL || parse_context->nErr)
+       if (p == NULL || parse_context->nErr != 0)
                 goto create_view_fail;
         struct Table *sel_tab = sqlite3ResultSetOfSelect(parse_context, select);
         if (sel_tab == NULL)
                 goto create_view_fail;
         if (aliases != NULL) {
                 if ((int)sel_tab->def->field_count != aliases->nExpr) {
-                       sqlite3ErrorMsg(parse_context,
-                                       "expected %d columns for '%s' but got %d",
-                                       aliases->nExpr, p->def->name,
+                       sqlite3ErrorMsg(parse_context, "expected %d columns "\
+                                       "for '%s' but got %d", aliases->nExpr,
+                                       p->def->name,
                                         sel_tab->def->field_count);
                         goto create_view_fail;
                 }
@@ -2053,21 +2053,17 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
          */
         struct Token end = parse_context->sLastToken;
         assert(end.z[0] != 0);
-       if (end.z[0] != ';') {
+       if (end.z[0] != ';')
                 end.z += end.n;
-       }
         end.n = 0;
-       int n = (int)(end.z - begin->z);
+       int n = end.z - begin->z;
         assert(n > 0);
         const char *z = begin->z;
-       while (sqlite3Isspace(z[n - 1])) {
+       while (sqlite3Isspace(z[n - 1]))
                 n--;
-       }
         end.z = &z[n - 1];
         end.n = 1;
-       p->def->opts.sql = malloc(n + 1);
-       memcpy(p->def->opts.sql, begin->z, n);
-       p->def->opts.sql[n] = '\0';
+       p->def->opts.sql = strndup(begin->z, n);

> +	p->def->opts.sql = malloc(n + 1);

13. No error checking. Here you should to set diag OOM, increment
parser.nErr etc.

> @@ -2084,155 +2084,22 @@ sql_view_column_names(struct Parse *parse, struct Table *table)
>  {
>  	assert(table != NULL);
>  	assert(space_is_view(table));
> -	/* A positive nCol means the columns names for this view
> -	 * are already known.
> -	 */
> -	if (table->def->field_count > 0)
> -		return 0;
> -
> -	/* A negative nCol is a special marker meaning that we are
> -	 * currently trying to compute the column names.  If we
> -	 * enter this routine with a negative nCol, it means two
> -	 * or more views form a loop, like this:
> -	 *
> -	 *     CREATE VIEW one AS SELECT * FROM two;
> -	 *     CREATE VIEW two AS SELECT * FROM one;
> -	 *
> -	 * Actually, the error above is now caught prior to
> -	 * reaching this point. But the following test is still
> -	 * important as it does come up in the following:
> -	 *
> -	 *     CREATE TABLE main.ex1(a);
> -	 *     CREATE TEMP VIEW ex1 AS SELECT a FROM ex1;
> -	 *     SELECT * FROM temp.ex1;
> -	 */
> -	if ((int)table->def->field_count < 0) {
> -		sqlite3ErrorMsg(parse, "view %s is circularly defined",
> -				table->def->name);
> +	struct sqlite3 *db = parse->db;
> +	struct Select *select;
> +	if (sql_view_compile(db, table->def->opts.sql, &select) != 0) {
> +		diag_log();

14. You should not log each error. sql_view_column_names can be called
by the parser only, so user will receive the error anyway. Log is needed
when 1) the error is not logged itself, see LoggedError, 2) when it
emerges with no possibility to be raised up to a user.

>  /**
>   * Remove entries from the _sql_stat1 and _sql_stat4
> @@ -2283,7 +2150,6 @@ static void
>  sql_code_drop_table(struct Parse *parse_context, struct space *space,
>  		    bool is_view)
>  {
> -	struct sqlite3 *db = parse_context->db;
>  	struct Vdbe *v = sqlite3GetVdbe(parse_context);
>  	assert(v != NULL);
>  	/*
> @@ -2313,6 +2179,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>  	int space_id_reg = ++parse_context->nMem;
>  	int space_id = space->def->id;
>  	sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
> +	sqlite3VdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);

15. Why do you need separate opcode here? As far as I can understand
you can rely on Tarantool core. SQL can just delete the view from
_space, and in alter.cc the refs will be checked.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 2aa35a114..0b64aee74 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -228,6 +225,43 @@ findRightmost(Select * p)
>  	return p;
>  }
>  
> +/**
> + * This function is an inner call of recursive traverse through
> + * select AST starting from interface function
> + * sql_select_expand_from_tables().
> + *
> + * @param top_select The root of AST.
> + * @param sub_select sub-select of current level recursion.
> + */
> +static void
> +expand_names_sub_select(struct Select *top_select,
> +			struct Select *sub_select)
> +{

16. Out of 80 below, and declaration fits in one line. Please, consider this
diff:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 0b64aee74..7cec4baa2 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -234,20 +234,19 @@ findRightmost(Select * p)
   * @param sub_select sub-select of current level recursion.
   */
  static void
-expand_names_sub_select(struct Select *top_select,
-                       struct Select *sub_select)
+expand_names_sub_select(struct Select *top_select, struct Select *sub_select)
  {
         assert(top_select != NULL);
         assert(sub_select != NULL);
-       for (int i = 0; i < sub_select->pSrc->nSrc; ++i) {
-               if (sub_select->pSrc->a[i].zName == NULL) {
-                       expand_names_sub_select(top_select,
-                                               sub_select->pSrc->a[i].pSelect);
+       struct SrcList_item *sub_src = sub_select->pSrc->a;
+       for (int i = 0; i < sub_select->pSrc->nSrc; ++i, ++sub_src) {
+               if (sub_src->zName == NULL) {
+                       expand_names_sub_select(top_select, sub_src->pSelect);
                 } else {
                         top_select->pSrc =
                                 sql_src_list_append_unique(sql_get(),
                                                            top_select->pSrc,
-                                                          sub_select->pSrc->a[i].zName);
+                                                          sub_src->zName);
                 }
         }
  }

> +void
> +sql_select_expand_from_tables(struct Select *select)
> +{
> +	assert(select != NULL);
> +	for (int i = 0; i < select->pSrc->nSrc; ++i)
> +		if (select->pSrc->a[i].zName == NULL)
> +			expand_names_sub_select(select,
> +						select->pSrc->a[i].pSelect);
> +}

17. Multi-line 'if' and 'for' must have {}:

@@ -256,10 +255,11 @@ void
  sql_select_expand_from_tables(struct Select *select)
  {
         assert(select != NULL);
-       for (int i = 0; i < select->pSrc->nSrc; ++i)
-               if (select->pSrc->a[i].zName == NULL)
-                       expand_names_sub_select(select,
-                                               select->pSrc->a[i].pSelect);
+       struct SrcList_item *src = select->pSrc->a;
+       for (int i = 0; i < select->pSrc->nSrc; ++i, ++src) {
+               if (src->zName == NULL)
+                       expand_names_sub_select(select, src->pSelect);
+       }
  }

> @@ -4738,13 +4772,16 @@ selectExpander(Walker * pWalker, Select * p)
>  			}
>  #if !defined(SQLITE_OMIT_VIEW)
>  			if (space_is_view(pTab)) {
> -				if (sql_view_column_names(pParse, pTab) != 0)
> +				struct Select *select;
> +				if (sql_view_compile(db, pTab->def->opts.sql,
> +						     &select) != 0) {
> +					diag_log();
>  					return WRC_Abort;
> +				}
> +				sqlite3SrcListAssignCursors(pParse,
> +							    select->pSrc);

18. Looks exactly like sql_view_column_names(). Why have not you used it?

>  				assert(pFrom->pSelect == 0);
> -				assert(pTab->def->opts.is_view ==
> -				       (pTab->pSelect != NULL));
> -				pFrom->pSelect =
> -				    sqlite3SelectDup(db, pTab->pSelect, 0);
> +				pFrom->pSelect = select;
>  				sqlite3SelectSetName(pFrom->pSelect,
>  						     pTab->def->name);
>  				int columns = pTab->def->field_count;
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 01351a183..781f5d0a0 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2962,6 +2959,8 @@ struct Parse {
>  	bool parse_only;
>  	/** If parse_only is set to true, store parsed expression. */
>  	struct Expr *parsed_expr;
> +	/** If parse_only is set to true, store parsed SELECT. */
> +	struct Select *parsed_select;

19. Is it possible to make union of parsed_expr, parsed_select?
Like this:

     union {
         struct Expr *expr;
         struct Select *select;
     } ast;

> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 42c70a255..25d8dc59b 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -566,3 +566,21 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
>  	sql_parser_destroy(&parser);
>  	return 0;
>  }
> +
> +int
> +sql_view_compile(struct sqlite3 *db, const char *view_stmt,
> +		 struct Select **select)
> +{
> +	assert(select != NULL);
> +	struct Parse parser;
> +	sql_parser_create(&parser, db);
> +	parser.parse_only = true;
> +	char *unused;
> +	if (sqlite3RunParser(&parser, view_stmt, &unused) != SQLITE_OK) {
> +		diag_set(ClientError, ER_SQL_EXECUTE, view_stmt);

20. ER_SQL_EXECUTE expects parser error message in the argument, it is not?
SQL statement in error looks useless. I see the fact of an error, but can not
understand what is wrong.

> +		return -1;

21. Must not you destroy the parser regardless of result?

> +	}
> +	*select = parser.parsed_select;
> +	sql_parser_destroy(&parser);
> +	return 0;
> +}
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index ea3521133..32f40044f 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -758,7 +758,7 @@ codeTriggerProgram(Parse * pParse,	/* The parser context */
>  				    sqlite3SelectDup(db, pStep->pSelect, 0);
>  				sqlite3SelectDestInit(&sDest, SRT_Discard, 0);
>  				sqlite3Select(pParse, pSelect, &sDest);
> -				sqlite3SelectDelete(db, pSelect);
> +			sql_select_delete(db, pSelect);

22. Something is wrong with indentation.

>  				break;
>  			}
>  		}
>