DanielYe133 I tested this and received the following;

10.7.0-dbg>CREATE TABLE test_table (c1 int(11) NOT NULL,c2 TEXT DEFAULT NULL,PRIMARY KEY (c1)) ENGINE=SPIDER DEFAULT CHARSET=utf8mb4 PARTITION BY LIST (CRC32(c1) MOD 2) (PARTITION pt0 VALUES IN (0) ENGINE = SPIDER, PARTITION pt1 VALUES IN (1) ENGINE=SPIDER);

ERROR 1564 (HY000): This partition function is not allowed

Can you check?

@Roel Van de Paar Removing the CRC32 should be okay. This appears in my example because we additionally added CRC32 support for partitioning in our fork. The partitioning strategy is not an influencer in this issue.

DanielYe133 Thank you for your report. I confirm that the bug reproduces in all the supported versions.

I did slight a debug concerning the following query on 10.2 HEAD(0d3de06e).

select * from test_table where c1=plusone(99);

The WHERE condition is appended by the second call of spider_mysql_handler::append_condition_part(). In more detail, the UDF stored function part is printed by spider_db_mysql_util::open_item_func().

There seem to be several problems here.

• The func_type of the UDF stored function is Item_func::FUNC_SP and it is not covered by the giant switch statement of spider_db_mysql_util::open_item_func}(). As a result, the UDF stored function is handled by the default case of the switch statement.
• The item_func->func_name() is empty. This would be a problem if we pushed down UDFs stored functions to data nodes. Otherwise, no problem.

Just skipping UDFs fixes the bug at least.

diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc

index 1a24d80a95f..b1db6aeb981 100644

--- a/storage/spider/spd_db_mysql.cc

+++ b/storage/spider/spd_db_mysql.cc

@@ -4222,6 +4222,8 @@ int spider_db_mysql_util::open_item_func(

          }

       }

       break;

+    case Item_func::FUNC_SP:

+    case Item_func::UDF_FUNC:

+      DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM);

     default:

       THD *thd = spider->trx->thd;

       SPIDER_SHARE *share = spider->share;

DanielYe133 What is your expectation on the UDF pushdown? Do you think an UDF should be pushed down to a data node?

A workaround for the bug is to set spider_skip_default_condition to 1.

set session spider_skip_default_condition=1;

I noticed that the above fix and the workaround work on 10.2 but not on 10.3 or higher. So, we need to do further debugging on 10.3.

More precisely, even on 10.3, SELECTs work as expected with the above fix but Update and DELETE do not.

Why the fix does work for 10.2? That is possibly because 10.2 does not have direct update and direct delete functionalities:

• handler::ha_direct_update_rows()
• handler::ha_direct_delete_rows()

I debugged the execution of the following query on 10.3 HEAD (a6383a1).

update test_table set c2="qq.com" where c1=plusone(99);

In a direct update, a WHERE clause is appended by spider_db_append_key_where_internal(). I do not yet know why but null search keys (start_key and end_key) are passed to the function. As a result, the function append no search condition. This is the mechanism of how the bug occurs.

I'm not yet so sure, but if Spider intentionally pass NULL search keys to spider_db_append_key_where_internal(), I think that a proper search condition should be appended by spider_mbase_handler::append_condition_part(). However, this is impossible because spider_db_mysql_util::open_item_func(), which is used in append_condition_part(), cannot properly handle the item_func corresponding to the UDF.

An easy fix would be not to perform a direct UPDATE when the WHERE condition includes UDFs.

The above would also apply to the case of DELETE.

> An easy fix would be not to perform a direct UPDATE when the WHERE condition includes UDFs.

I should have said that "If we do not push down UDFs to data nodes, an easy fix would be not to perform a direct UPDATE when the WHERE condition includes UDFs." If we decided to push down UDFs to data nodes, we might be able to just push it down without giving up direct UPDATE and DELETE.

The function spider_mbase_handler::append_condition() ignores the error ER_SPIDER_COND_SKIP_NUM. This is OK if the condition pushdown is just for optimization. That is because, even if some conditions are ignored, just more rows are returned by data nodes. However, for direct UPDATE and direct DELETE cases, every condition must be pushed down to data nodes.

int spider_mbase_handler::append_condition(

  spider_string *str,

  const char *alias,

  uint alias_length,

  bool start_where,

  ulong sql_type

) {

  ...

    if ((error_num = spider_db_print_item_type(

      (Item *) tmp_cond->cond, NULL, spider, str, alias, alias_length,

      dbton_id, FALSE, NULL)))

    {

      if (str && error_num == ER_SPIDER_COND_SKIP_NUM)

      {

        DBUG_PRINT("info",("spider COND skip"));

        str->length(restart_pos);

        start_where = (restart_pos == start_where_pos);

      } else

        DBUG_RETURN(error_num);

    }

    tmp_cond = tmp_cond->next;

  }

  DBUG_RETURN(0);

}

@nayuta-yanagisawa I personally prefer skipping UDFs conditions in the case of SELECT, and disabling direct update/delete in the case of UPDATE/DELETE, even though it means it could potentially bring the server a lot more records to process.

This is because, as far as I know, there is a blurry line between UDFs and SPs, and in our case the server identifies our functions as Item_func::FUNC_SP, even they do not have the characteristics of a SP (e.g. has DML queries, or takes OUT parameters). So, correct me if I am wrong, I doubt that the server is able to distinguish between a UDF and an SP. Therefore, if we decide to push down conditions containing usage of UDFs, it will likely also pushdown conditions containing SPs. This could become a problem because an SP could contain DML operations on several tables, and even if a remote has the same definition of the SP, it can fail to execute the SP due to non-existence of some of the tables. In this case it could be really error-prone.

So, I would suggest pushing down UDF conditions only if we are able to make sure the remotes can execute them successfully and do not do any unexpected damage.

DanielYe133 Thank you for your input. I think too that we should not push down UDFs, stored functions, and stored procedures. That is because inefficient and accurate query execution is far better than efficient and faulty query execution. Spider has a tendency to over-optimize at times, which I believe is not good.

In my previous comments, I wrote "UDF" where I should have written "stored function" (already corrected). I apologize if I caused any confusion.

DanielYe133 By the way, you can write like [~nayuta-yanagisawa] to mention me.

DanielYe133 I may not have fully understood your statement. SP here means "stored procedure", right? This seems to be no problem because stored procedures do not appear in WHERE clauses.

This is because, as far as I know, there is a blurry line between UDFs and SPs, and in our case the server identifies our functions as Item_func::FUNC_SP, even they do not have the characteristics of a SP (e.g. has DML queries, or takes OUT parameters). So, correct me if I am wrong, I doubt that the server is able to distinguish between a UDF and an SP.

nayuta-yanagisawa (thank you for telling me how to do it , the user mention option in the edit box really confuses me)

You are right. I was referring to stored procedures and indeed they do not appear in WHERE clauses. I did not fully comprehend what Item_func::FUNC_SP represents and thought it means stored procedures (judging by the name), and after looking into the code I now realize it is the type of Item_func_sp which represents stored functions.

I may not have fully understood your statement. SP here means "stored procedure", right? This seems to be no problem because stored procedures do not appear in WHERE clauses.

DanielYe133 OK. BTW, are you interested in fixing the present bug by yourself? If you want me to do it, that's fine too.

> after looking into the code I now realize it is the type of Item_func_sp which represents stored functions.

In my understanding, Item is something that returns a value, an expression. So, I think stored procedures aren't represented by Item_func_sp.

nayuta-yanagisawa Sure, I'd love to. And if you don't mind, please let me know if there is anything I need to pay attention to before doing it.

And great point on this!

In my understanding, Item is something that returns a value, an expression. So, I think stored procedures aren't represented by Item_func_sp.

DanielYe133 Great! Because of JIRA permission, I cannot assign the issue to you, but you are now in charge of this.

Here is some advice:

• Please read the contribution guide first. https://mariadb.com/kb/en/contributing-code/
• You can use your predecessor's PRs as a guide to creating your own PR.
• In general, the base branch should be the oldest supported major version on which the bug occurs.
• However, for this bug, the server behaves differently based on the major versions (see my comments above). So, I suggest you create different PRs based on 10.2 and 10.3, while the test can be shared. More precisely, you can create a PR including fix and test based on 10.2. Once I merge it to 10.2 and 10.3, you can create another PR, based on 10.3, including only a fix for 10.3 or later.

DanielYe133 If you have any questions, please feel free to ask.

nayuta-yanagisawa Thank you so much for your detailed guidance! I'll inform you once I have any progress on any question.