[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>

[Nikita Pettik]

On Wed, Sep 04, 2019 at 05:14:10PM +0300, Roman Khabibov wrote:
> 
> commit 4f78ff7cf8b4a3f496d72388235f773ae51d6efc
> Author: Roman Khabibov <roman.habibov at tarantool.org>
> Date:   Mon Jul 29 18:00:34 2019 +0300
> 
>     sql: allow to create view as <WITH> clause
>     
>     Allow views to use CTEs in <WITH> clauses, 

Nit: all CTEs start from WITH clause; so your statement is a bit confusing.

>     which can be in any
>     (nested) select after <AS>. Before this patch, during view
>     creation all referenced spaces were fetched by name from SELECT
>     and their reference counters were incremented to avoid dangling
>     references. It occurred in update_view_references(). Obviously, CTE
>     tables weren't held in space cache, ergo error "space doesn’t
>     exist" was raised. Add check if a space from FROM is CTE. If it is,
>     don't increment its reference counter and don't raise the error.
>     
>     Closes #4149
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 4f2e34bf0..a625d8c39 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1759,6 +1759,13 @@ update_view_references(struct Select *select, int update_value,
>  		const char *space_name = sql_src_list_entry_name(list, i);
>  		if (space_name == NULL)
>  			continue;
> +		/*
> +		 * CTE tables isn't held in space cache and we

Nit: tables aren't held ...

> +		 * don't need to increment reference counter for
> +		 * CTEs.

I'd rephrase a bit:

Views are allowed to contain CTEs. CTE is a temporary object, created
and destroyed at SQL runtime (it is represented by an ephemeral table).
So, it is absent in space cache and as a consequence we can't increment
its reference counter. Skip iteration.

> +		 */
> +		if (sql_select_has_cte_with_name(select, space_name))
> +			continue;
>  		struct space *space = space_by_name(space_name);
>  		if (space == NULL) {
>  			if (! suppress_error) {
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 7644051b4..422c8dfc2 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -323,6 +323,17 @@ sql_select_delete(struct sql *db, struct Select *select);
>  struct SrcList *
>  sql_select_expand_from_tables(struct Select *select);
>  
> +/**
> + * Check if @a name matches with at least one of CTE names typed
> + * in <WITH> clauses within @a select.
> + *
> + * @param select Select which may contain CTE.

Nit: you forgot to describe name argument.

> + * @retval true Has CTE with @a name.
> + * @retval false Hasn't CTE with @a name.
> +*/
> +bool
> +sql_select_has_cte_with_name(struct Select *select, const char *name);

sql_select_contains_cte() is enough, I guess.

> +
>  /**
>   * Temporary getter in order to avoid including sqlInt.h
>   * in alter.cc.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index ddb5de87e..d7dab35cd 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -332,6 +332,29 @@ sql_select_expand_from_tables(struct Select *select)
>  	return walker.u.pSrcList;
>  }
>  
> +bool
> +sql_select_has_cte_with_name(struct Select *select, const char *name)
> +{
> +	assert(select != NULL && name != NULL);
> +	struct With *with = select->pWith;
> +	if (with != NULL) {
> +		for (int i = 0; i < with->nCte; i++) {
> +			const struct Cte *cte = &with->a[i];
> +			if (memcmp(name, cte->zName, strlen(name)) == 0)
> +				return true;
> +		}
> +	}

Please explain why we don't use recursion here (cte->pSelect->pWith).
I'd attach schema of SELECT for the sake of the clarity.

> +	struct SrcList *list = select->pSrc;
> +	int item_count = sql_src_list_entry_count(list);
> +	for (int i = 0; i < item_count; ++i) {
> +		if (list->a[i].pSelect != NULL)
> +			if (sql_select_has_cte_with_name(list->a[i].pSelect,
> +							 name))
> +				return true;
> +	

Nit: in case 'if' body contains more than one string it is enclosed in
brackets.