Hi, Sanja! _some_ comments are below. The main thing, I still don't understand your changes in sql_select.cc. Why did you create separate counters and code paths with if() for CYCLE? I'd think it could be just a generalization of DISTINCT On Feb 20, Oleksandr Byelkin wrote:

revision-id: 4c263b6d30b (mariadb-10.5.0-92-g4c263b6d30b) parent(s): 6f2e2285291 author: Oleksandr Byelkin <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> timestamp: 2020-01-27 21:56:02 +0100 message:

MDEV-20632: Recursive CTE cycle detection using CYCLE clause

Added CYCLE clause to recursive CTE.

diff --git a/sql/item.h b/sql/item.h index c1f68a4f942..2cbaf880e00 100644 --- a/sql/item.h +++ b/sql/item.h @@ -622,6 +622,13 @@ class st_select_lex_unit; class Item_func_not; class Item_splocal;

+/* Item::common_flags */ +/* Indicates that name of this Item autogenerated or set by user */ +#define IS_AUTO_GENETATED_NAME 1

typo ^^^

+/* Inticates that this item is in CYCLE clause of WITH */

typo ^^^

+#define IS_IN_WITH_CYCLE 2 + + /** String_copier that sends Item specific warnings. */ diff --git a/sql/sql_cte.cc b/sql/sql_cte.cc index 5b3d3108da5..971844120bc 100644 --- a/sql/sql_cte.cc +++ b/sql/sql_cte.cc @@ -982,6 +982,38 @@ With_element::rename_columns_of_derived_unit(THD *thd,

are you sure what you're doing below can still be called "rename_columns_of_derived_unit" ?

else make_valid_column_names(thd, select->item_list);

+ if (cycle_list) + { + List_iterator_fast<Item> it(select->item_list); + List_iterator_fast<LEX_CSTRING> nm(*cycle_list); + List_iterator_fast<LEX_CSTRING> nm_check(*cycle_list); + DBUG_ASSERT(cycle_list->elements != 0); + while (LEX_CSTRING *name= nm++) + { + Item *item; + // unique check + LEX_CSTRING *check; + nm_check.rewind(); + while ((check= nm_check++) && check != name) + { + if (check->length == name->length && + strncmp(check->str, name->str, name->length) == 0) + { + my_error(ER_DUP_FIELDNAME, MYF(0), check->str); + return true; + } + } + while ((item= it++) && + (item->name.length != name->length || + strncmp(item->name.str, name->str, name->length) != 0)); + if (item == NULL) + { + my_error(ER_BAD_FIELD_ERROR, MYF(0), name->str, "CYCLE clause"); + return true; + }

this pattern is used so often that we might want to have a helper, like, List_iterator_fast<>::find() not in this commit, though.

+ item->common_flags|= IS_IN_WITH_CYCLE; + } + } unit->columns_are_renamed= true;

return false; @@ -1425,6 +1457,21 @@ void With_clause::print(String *str, enum_query_type query_type) }

+static void list_strlex_print(String *str, List<LEX_CSTRING> *list) +{ + List_iterator_fast<LEX_CSTRING> li(*list); + bool first= TRUE; + while(LEX_CSTRING *col_name= li++) + { + if (first) + first= FALSE; + else + str->append(','); + str->append(col_name);

should be "append_identifier", shouldn't it? add a test where a column name is a reserved word.

+ } +} + + /** @brief Print this with element @@ -1444,22 +1491,20 @@ void With_element::print(String *str, enum_query_type query_type) { List_iterator_fast<LEX_CSTRING> li(column_list); str->append('('); - for (LEX_CSTRING *col_name= li++; ; ) - { - str->append(col_name); - col_name= li++; - if (!col_name) - { - str->append(')'); - break; - } - str->append(','); - } + list_strlex_print(str, &column_list);

any other code that could make use of your list_strlex_print ?

+ str->append(')'); } - str->append(STRING_WITH_LEN(" as ")); - str->append('('); + str->append(STRING_WITH_LEN(" as (")); spec->print(str, query_type); str->append(')'); + + if (cycle_list) + { + DBUG_ASSERT(cycle_list->elements != 0); + str->append(STRING_WITH_LEN(" CYCLE (")); + list_strlex_print(str, cycle_list); + str->append(") "); + } }

diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index c115d9352aa..0d60ab8579f 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -14704,9 +14704,30 @@ with_list_element: if (elem->set_unparsed_spec(thd, spec_start, $6.pos(), spec_start - query_start)) MYSQL_YYABORT; + if ($7) + { + elem->set_cycle_list($7); + } } ;

+opt_cycle: + /* empty */ + { $$= NULL; } + | + CYCLE_SYM + { + if (!Lex->curr_with_clause->with_recursive) + { + thd->parse_error(ER_SYNTAX_ERROR, $1.pos()); + } + } + '(' with_column_list ')'

Where did you see that the standard requires parentheses here?

+ { + $$= $4; + } + ; +

opt_with_column_list: /* empty */

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org