Re: f3aa4d0a215: MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func
Hi, Yuchen, On Jun 20, Yuchen Pei wrote:
revision-id: f3aa4d0a215 (mariadb-10.4.30-4-gf3aa4d0a215) parent(s): f5dceafd0ba author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-06-14 15:02:38 +1000 message:
MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func
spider_db_mbase_util::open_item_func() is a monster function. It is difficult to maintain while it is expected that we need to modify it when a new SQL function or a new func_type is added.
We split the function into two distinct functions: one handles the case of str != NULL and the other handles the case of str == NULL.
This refactoring was done in a conservative way because we do not have comprehensive tests on the function.
It also fixes MDEV-29447 and MDEV-31338 where field items that are arguments of a func item may be used before created / initialised.
Note this commit is a port of 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 (MDEV-26285) to current versions 10.3+.
As I wrote in the first review, please, don't mention transient commit hashes in a permanent commit comment. This commit will, supposedly, be pushed into the main branch. While 3836098c29ef1 will, on the other hand, disappear, as it's not in any of the main branches. You'll have a dangling pointer
Signed-off-by: Yuchen Pei
also, it looks strange to sign-off your own commits
diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc index a51c1497b02..1ed733105c6 100644 --- a/storage/spider/spd_db_mysql.cc +++ b/storage/spider/spd_db_mysql.cc @@ -5848,94 +5992,12 @@ int spider_db_mbase_util::open_item_func( { if (!strncasecmp("utc_timestamp", func_name, func_name_length)) { - if (str) str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); DBUG_RETURN(spider_db_open_item_string(item_func, NULL, spider, str, alias, alias_length, dbton_id, use_fields, fields)); } else if (!strncasecmp("timestampdiff", func_name, func_name_length)) { -#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC
Why did you removed this and not the #else part?
- Item_func_timestamp_diff *item_func_timestamp_diff = - (Item_func_timestamp_diff *) item_func; - if (str) - { - const char *interval_str; - uint interval_len; - switch (item_func_timestamp_diff->int_type) - { - case INTERVAL_YEAR: - interval_str = SPIDER_SQL_YEAR_STR; - interval_len = SPIDER_SQL_YEAR_LEN; - break; - case INTERVAL_QUARTER: - interval_str = SPIDER_SQL_QUARTER_STR; - interval_len = SPIDER_SQL_QUARTER_LEN; - break; - case INTERVAL_MONTH: - interval_str = SPIDER_SQL_MONTH_STR; - interval_len = SPIDER_SQL_MONTH_LEN; - break; - case INTERVAL_WEEK: - interval_str = SPIDER_SQL_WEEK_STR; - interval_len = SPIDER_SQL_WEEK_LEN; - break; - case INTERVAL_DAY: - interval_str = SPIDER_SQL_DAY_STR; - interval_len = SPIDER_SQL_DAY_LEN; - break; - case INTERVAL_HOUR: - interval_str = SPIDER_SQL_HOUR_STR; - interval_len = SPIDER_SQL_HOUR_LEN; - break; - case INTERVAL_MINUTE: - interval_str = SPIDER_SQL_MINUTE_STR; - interval_len = SPIDER_SQL_MINUTE_LEN; - break; - case INTERVAL_SECOND: - interval_str = SPIDER_SQL_SECOND_STR; - interval_len = SPIDER_SQL_SECOND_LEN; - break; - case INTERVAL_MICROSECOND: - interval_str = SPIDER_SQL_MICROSECOND_STR; - interval_len = SPIDER_SQL_MICROSECOND_LEN; - break; - default: - interval_str = ""; - interval_len = 0; - break; - } - str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); - if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN + - interval_len + SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(func_name, func_name_length); - str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); - str->q_append(interval_str, interval_len); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[0], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[1], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_CLOSE_PAREN_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, - SPIDER_SQL_CLOSE_PAREN_LEN); - } - DBUG_RETURN(0); -#else DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); -#endif } } else if (func_name_length == 14) {
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, Thanks for the comments. See below see my response and I have posted the url to the updated patch in the ticket. On Tue 2023-06-20 17:04:50 +0200, Sergei Golubchik wrote:
Hi, Yuchen,
On Jun 20, Yuchen Pei wrote:
revision-id: f3aa4d0a215 (mariadb-10.4.30-4-gf3aa4d0a215) parent(s): f5dceafd0ba author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-06-14 15:02:38 +1000 message:
MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func
spider_db_mbase_util::open_item_func() is a monster function. It is difficult to maintain while it is expected that we need to modify it when a new SQL function or a new func_type is added.
We split the function into two distinct functions: one handles the case of str != NULL and the other handles the case of str == NULL.
This refactoring was done in a conservative way because we do not have comprehensive tests on the function.
It also fixes MDEV-29447 and MDEV-31338 where field items that are arguments of a func item may be used before created / initialised.
Note this commit is a port of 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 (MDEV-26285) to current versions 10.3+.
As I wrote in the first review, please, don't mention transient commit hashes in a permanent commit comment. This commit will, supposedly, be pushed into the main branch. While 3836098c29ef1 will, on the other hand, disappear, as it's not in any of the main branches. You'll have a dangling pointer
Done.
Signed-off-by: Yuchen Pei
also, it looks strange to sign-off your own commits
I thought I needed this for gpg sign, but just learned they are two different things. So I have removed the sign-off. By the way why is it strange to sign-off own commits?
diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc index a51c1497b02..1ed733105c6 100644 --- a/storage/spider/spd_db_mysql.cc +++ b/storage/spider/spd_db_mysql.cc @@ -5848,94 +5992,12 @@ int spider_db_mbase_util::open_item_func( { if (!strncasecmp("utc_timestamp", func_name, func_name_length)) { - if (str) str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); DBUG_RETURN(spider_db_open_item_string(item_func, NULL, spider, str, alias, alias_length, dbton_id, use_fields, fields)); } else if (!strncasecmp("timestampdiff", func_name, func_name_length)) { -#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC
Why did you removed this and not the #else part?
This is part of MDEV-29269. It was a done task with a fixversion 11.0+, but there's no reason the same change shouldn't be in earlier versions. This is one of the redundant #ifdefs, where the flag is deterministically 0 or 1 (see MDEV-26178 for more context). In this case ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC is always undefined, so we remove the #if part and keep the #else part.
- Item_func_timestamp_diff *item_func_timestamp_diff = - (Item_func_timestamp_diff *) item_func; - if (str) - { - const char *interval_str; - uint interval_len; - switch (item_func_timestamp_diff->int_type) - { - case INTERVAL_YEAR: - interval_str = SPIDER_SQL_YEAR_STR; - interval_len = SPIDER_SQL_YEAR_LEN; - break; - case INTERVAL_QUARTER: - interval_str = SPIDER_SQL_QUARTER_STR; - interval_len = SPIDER_SQL_QUARTER_LEN; - break; - case INTERVAL_MONTH: - interval_str = SPIDER_SQL_MONTH_STR; - interval_len = SPIDER_SQL_MONTH_LEN; - break; - case INTERVAL_WEEK: - interval_str = SPIDER_SQL_WEEK_STR; - interval_len = SPIDER_SQL_WEEK_LEN; - break; - case INTERVAL_DAY: - interval_str = SPIDER_SQL_DAY_STR; - interval_len = SPIDER_SQL_DAY_LEN; - break; - case INTERVAL_HOUR: - interval_str = SPIDER_SQL_HOUR_STR; - interval_len = SPIDER_SQL_HOUR_LEN; - break; - case INTERVAL_MINUTE: - interval_str = SPIDER_SQL_MINUTE_STR; - interval_len = SPIDER_SQL_MINUTE_LEN; - break; - case INTERVAL_SECOND: - interval_str = SPIDER_SQL_SECOND_STR; - interval_len = SPIDER_SQL_SECOND_LEN; - break; - case INTERVAL_MICROSECOND: - interval_str = SPIDER_SQL_MICROSECOND_STR; - interval_len = SPIDER_SQL_MICROSECOND_LEN; - break; - default: - interval_str = ""; - interval_len = 0; - break; - } - str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); - if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN + - interval_len + SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(func_name, func_name_length); - str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); - str->q_append(interval_str, interval_len); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[0], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[1], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_CLOSE_PAREN_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, - SPIDER_SQL_CLOSE_PAREN_LEN); - } - DBUG_RETURN(0); -#else DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); -#endif } } else if (func_name_length == 14) {
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Best, Yuchen
Hi, Yuchen, On Jun 21, Yuchen Pei wrote:
Signed-off-by: Yuchen Pei
also, it looks strange to sign-off your own commits
I thought I needed this for gpg sign, but just learned they are two different things. So I have removed the sign-off. By the way why is it strange to sign-off own commits?
I somehow percieved it as someone prepares the commit and someone _else_ signs it off. But ok, whatever you prefer, we don't rely on Signed-off having any particular value, so you can use it as you want.
diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc index a51c1497b02..1ed733105c6 100644 --- a/storage/spider/spd_db_mysql.cc +++ b/storage/spider/spd_db_mysql.cc @@ -5848,94 +5992,12 @@ int spider_db_mbase_util::open_item_func( { if (!strncasecmp("utc_timestamp", func_name, func_name_length)) { - if (str) str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); DBUG_RETURN(spider_db_open_item_string(item_func, NULL, spider, str, alias, alias_length, dbton_id, use_fields, fields)); } else if (!strncasecmp("timestampdiff", func_name, func_name_length)) { -#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC
Why did you removed this and not the #else part?
This is part of MDEV-29269. It was a done task with a fixversion 11.0+, but there's no reason the same change shouldn't be in earlier versions. This is one of the redundant #ifdefs, where the flag is deterministically 0 or 1 (see MDEV-26178 for more context). In this case ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC is always undefined, so we remove the #if part and keep the #else part.
But it was *incorrectly* undefined. It was waiting for spider to have access to item_func_timestamp_diff->int_type, which was private at some point. It is private still, but there's an accessor and spider can perfectly use switch (item_func_timestamp_diff->get_int_type()) there's no reason to remove all that code neither in 10.4 nor in 11.0
- Item_func_timestamp_diff *item_func_timestamp_diff = - (Item_func_timestamp_diff *) item_func; - if (str) - { - const char *interval_str; - uint interval_len; - switch (item_func_timestamp_diff->int_type) - { - case INTERVAL_YEAR: - interval_str = SPIDER_SQL_YEAR_STR; - interval_len = SPIDER_SQL_YEAR_LEN; - break; - case INTERVAL_QUARTER: - interval_str = SPIDER_SQL_QUARTER_STR; - interval_len = SPIDER_SQL_QUARTER_LEN; - break; - case INTERVAL_MONTH: - interval_str = SPIDER_SQL_MONTH_STR; - interval_len = SPIDER_SQL_MONTH_LEN; - break; - case INTERVAL_WEEK: - interval_str = SPIDER_SQL_WEEK_STR; - interval_len = SPIDER_SQL_WEEK_LEN; - break; - case INTERVAL_DAY: - interval_str = SPIDER_SQL_DAY_STR; - interval_len = SPIDER_SQL_DAY_LEN; - break; - case INTERVAL_HOUR: - interval_str = SPIDER_SQL_HOUR_STR; - interval_len = SPIDER_SQL_HOUR_LEN; - break; - case INTERVAL_MINUTE: - interval_str = SPIDER_SQL_MINUTE_STR; - interval_len = SPIDER_SQL_MINUTE_LEN; - break; - case INTERVAL_SECOND: - interval_str = SPIDER_SQL_SECOND_STR; - interval_len = SPIDER_SQL_SECOND_LEN; - break; - case INTERVAL_MICROSECOND: - interval_str = SPIDER_SQL_MICROSECOND_STR; - interval_len = SPIDER_SQL_MICROSECOND_LEN; - break; - default: - interval_str = ""; - interval_len = 0; - break; - } - str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); - if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN + - interval_len + SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(func_name, func_name_length); - str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); - str->q_append(interval_str, interval_len); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[0], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[1], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_CLOSE_PAREN_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, - SPIDER_SQL_CLOSE_PAREN_LEN); - } - DBUG_RETURN(0); -#else DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); -#endif } } else if (func_name_length == 14) {
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, Thanks for the comment, see the new comment in the ticket for the new patch url. On Wed 2023-06-21 15:11:01 +0200, Sergei Golubchik wrote:
-#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC
Why did you removed this and not the #else part?
This is part of MDEV-29269. It was a done task with a fixversion 11.0+, but there's no reason the same change shouldn't be in earlier versions. This is one of the redundant #ifdefs, where the flag is deterministically 0 or 1 (see MDEV-26178 for more context). In this case ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC is always undefined, so we remove the #if part and keep the #else part.
But it was *incorrectly* undefined. It was waiting for spider to have access to item_func_timestamp_diff->int_type, which was private at some point. It is private still, but there's an accessor and spider can perfectly use
switch (item_func_timestamp_diff->get_int_type())
there's no reason to remove all that code neither in 10.4 nor in 11.0
I see, I have reinstated the ifdef and updated the check_item_func() accordingly. I will update the patch for other server versions after the review is done. Since this patch should focus on the conservative refactoring while fixing MDEV-29447 and MDEV-31338, I will do the removal of the flag and keeping the ifdef branch etc. in a separate patch, probably for MDEV-30504. Best, Yuchen
participants (2)
-
Sergei Golubchik -
Yuchen Pei