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

Nikita Pettik korablev at tarantool.org
Mon Sep 16 22:48:34 MSK 2019


Thx, finally LGTM. Pushed to master.

On 13 Sep 17:57, Roman Khabibov wrote:
> 
> > On Sep 11, 2019, at 16:32, Nikita Pettik <korablev at tarantool.org> wrote:
> > 
> > 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.
>     sql: allow to create view as <WITH> clause
>     
>     Allow views to use CTEs, 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
> 
> >>    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.
> Ok.
> 
> @@ -1759,6 +1759,16 @@ 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;
> +		/*
> +		 * 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_constains_cte(select, space_name))
> +			continue;
>  		struct space *space = space_by_name(space_name);
>  		if (space == NULL) {
>  			if (! suppress_error) {
> 
> >> +		 */
> >> +		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.
> +/**
> + * Check if @a name matches with at least one of CTE names typed
> + * in <WITH> clauses within @a select except <WITH>s that are
> + * nested within other <WITH>s.
> + *
> + * @param select Select which may contain CTE.
> + * @param name The name of CTE, that may contained.
> + * @retval true Has CTE with @a name.
> + * @retval false Hasn't CTE with @a name.
> +*/
> +bool
> +sql_select_constains_cte(struct Select *select, const char *name);
> +
> 
> >> +
> >> /**
> >>  * 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.
> +bool
> +sql_select_constains_cte(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];
> +			/*
> +			 * Don't use recursive call for
> +			 * cte->pSelect, because this function is
> +			 * used during view creation. Consider
> +			 * the nested <WITH>s query schema:
> +			 * CREATE VIEW v AS
> +			 *     WITH w AS (
> +			 *         WITH w_nested AS
> +			 *             (...)
> +			 *         SELECT ...)
> +			 *     SELECT ... FROM ...;
> +			 * The use of CTE "w_nested" after the
> +			 * external select's <FROM> is disallowed.
> +			 * So, it is pointless to check <WITH>,
> +			 * which is nested to other <WITH>.
> +			 */
> +			if (memcmp(name, cte->zName, strlen(name)) == 0)
> +				return true;
> +		}
> +	}
> +	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_constains_cte(list->a[i].pSelect,
> +							 name))
> +				return true;
> +		}
> +	}
> +	return false;
> +}
> +
> 
> commit 32b5b1b2f355f57caf8e84e78bb3154890d7d82e
> 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, 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 c0780d6da..516d6abac 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1759,6 +1759,16 @@ 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;
> +		/*
> +		 * 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_constains_cte(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..0fa52fc0b 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -323,6 +323,19 @@ 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 except <WITH>s that are
> + * nested within other <WITH>s.
> + *
> + * @param select Select which may contain CTE.
> + * @param name The name of CTE, that may contained.
> + * @retval true Has CTE with @a name.
> + * @retval false Hasn't CTE with @a name.
> +*/
> +bool
> +sql_select_constains_cte(struct Select *select, const char *name);
> +
>  /**
>   * 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 cdcdbaf2b..8f93edd16 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -332,6 +332,46 @@ sql_select_expand_from_tables(struct Select *select)
>  	return walker.u.pSrcList;
>  }
>  
> +bool
> +sql_select_constains_cte(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];
> +			/*
> +			 * Don't use recursive call for
> +			 * cte->pSelect, because this function is
> +			 * used during view creation. Consider
> +			 * the nested <WITH>s query schema:
> +			 * CREATE VIEW v AS
> +			 *     WITH w AS (
> +			 *         WITH w_nested AS
> +			 *             (...)
> +			 *         SELECT ...)
> +			 *     SELECT ... FROM ...;
> +			 * The use of CTE "w_nested" after the
> +			 * external select's <FROM> is disallowed.
> +			 * So, it is pointless to check <WITH>,
> +			 * which is nested to other <WITH>.
> +			 */
> +			if (memcmp(name, cte->zName, strlen(name)) == 0)
> +				return true;
> +		}
> +	}
> +	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_constains_cte(list->a[i].pSelect,
> +							 name))
> +				return true;
> +		}
> +	}
> +	return false;
> +}
> +
>  /*
>   * Given 1 to 3 identifiers preceding the JOIN keyword, determine the
>   * type of join.  Return an integer constant that expresses that type
> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
> index 101f4c3e7..6234f863e 100755
> --- a/test/sql-tap/view.test.lua
> +++ b/test/sql-tap/view.test.lua
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env tarantool
>  test = require("sqltester")
> -test:plan(73)
> +test:plan(78)
>  
>  --!./tcltestrunner.lua
>  -- 2002 February 26
> @@ -1233,4 +1233,96 @@ test:do_catchsql_test(
>          -- </view-23.8>
>      })
>  
> +-- gh-4149: Make sure that VIEW can be created as CTE.
> +test:do_execsql_test(
> +    "view-24.1",
> +    [[
> +        CREATE TABLE ts (s1 INT PRIMARY KEY);
> +        INSERT INTO ts VALUES (1);
> +    ]], {
> +        -- <view-24.1>
> +        -- </view-24.1>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.2",
> +    [[
> +        CREATE VIEW v AS WITH w(id) AS (
> +            SELECT 1)
> +          SELECT * FROM ts, w;
> +        SELECT * FROM v;
> +    ]], {
> +        -- <view-24.2>
> +        1, 1
> +        -- </view-24.2>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.3",
> +    [[
> +        DROP VIEW v;
> +        CREATE VIEW v AS WITH RECURSIVE w AS (
> +            SELECT s1 FROM ts
> +            UNION ALL
> +            SELECT s1+1 FROM w WHERE s1 < 4)
> +          SELECT * FROM w;
> +        SELECT * FROM v;
> +    ]], {
> +        -- <view-24.3>
> +        1,2,3,4
> +        -- </view-24.3>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.4",
> +    [[
> +        DROP VIEW v;
> +        CREATE VIEW v AS SELECT * FROM
> +            (SELECT 1),
> +            (SELECT 2) JOIN
> +            (WITH RECURSIVE w AS (
> +                SELECT s1 FROM ts
> +                UNION ALL
> +                SELECT s1+1 FROM w WHERE s1 < 4)
> +              SELECT * FROM w);
> +          SELECT * FROM v;
> +    ]], {
> +        -- <view-24.4>
> +        1,2,1,1,2,2,1,2,3,1,2,4
> +        -- </view-24.4>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.5",
> +    [[
> +        DROP VIEW v;
> +        DROP TABLE ts;
> +        CREATE VIEW v AS WITH RECURSIVE
> +            xaxis(x) AS (
> +                VALUES(-2.0)
> +                UNION ALL
> +                SELECT x+0.5 FROM xaxis WHERE x<1.2),
> +            yaxis(y) AS (
> +                VALUES(-1.0)
> +                UNION ALL
> +                SELECT y+0.5 FROM yaxis WHERE y<1.0),
> +            m(iter, cx, cy, x, y) AS (
> +                SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis
> +                UNION ALL
> +                SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m WHERE (x*x + y*y) < 4.0 AND iter<1
> +            ),
> +            m2(iter, cx, cy) AS (
> +            SELECT max(iter), cx, cy FROM m GROUP BY cx, cy
> +            ),
> +            a(t) AS (
> +                SELECT group_concat( substr('a', 1+least(iter/7,4), 1), '') FROM m2 GROUP BY cy
> +            )
> +          SELECT group_concat(trim(t),x'0a') FROM a;
> +        SELECT * FROM v;
> +    ]], {
> +        -- <view-24.5>
> +        "aaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa"
> +        -- </view-24.5>
> +    })
> +
>  test:finish_test()
> 
> 
> 




More information about the Tarantool-patches mailing list