[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