Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-26364

Add Spider server variable for disabling GROUP BY handler

Details

    Description

      Add a server variable, to the Spider storage engine, that enables/disables the use of GROUP BY handler. This provides users with a unified workaround for the bugs like MDEV-26013. In the Spider docs, an execution is called "direct" if it uses GROUP BY handler (e.g., direct aggregation, direct join, ...). So, the parameter name would be spider_direct_execution spider_enable_direct_select. Or, simply naming spider_enable_group_by_handler would be OK.

      The default value of the variable should be 1 (i.e., use GROUP BY handler) for compatibility.

      Attachments

        Issue Links

          Activity

            serg I would like to introduce the parameter to 10.3 and later. In principle, it might not be good to add a new parameter to the stable versions, but I expect this helps many users. I also think that the parameter should have been introduced at the same time as the GROUP BY handler.

            What do you think? I would also like to get your opinion on the parameter name.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - serg I would like to introduce the parameter to 10.3 and later. In principle, it might not be good to add a new parameter to the stable versions, but I expect this helps many users. I also think that the parameter should have been introduced at the same time as the GROUP BY handler. What do you think? I would also like to get your opinion on the parameter name.

            This parameter seems to be useful for testing purposes. One can specify the execution path that he want to test.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - This parameter seems to be useful for testing purposes. One can specify the execution path that he want to test.

            10.3 looks ok. spider_direct_execution sounds good too.

            serg Sergei Golubchik added a comment - 10.3 looks ok. spider_direct_execution sounds good too.

            Thank you for your confirmation.

            After I asked you a question, I noticed that there are status variables named spider_direct_update and spider_direct_delete. Thus, naming spider_direct_execution might be confusing. Maybe, spider_enable_direct_select is a better name, because it clarifies that it is a switch of direct execution, and the Spider's GROUP BY handler only supports SELECT statements.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - Thank you for your confirmation. After I asked you a question, I noticed that there are status variables named spider_direct_update and spider_direct_delete . Thus, naming spider_direct_execution might be confusing. Maybe, spider_enable_direct_select is a better name, because it clarifies that it is a switch of direct execution, and the Spider's GROUP BY handler only supports SELECT statements.

            This seems not to be "refactoring". So, I removed the epic "Spider Refactoring".

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - This seems not to be "refactoring". So, I removed the epic "Spider Refactoring".

            Please review https://github.com/MariaDB/server/commit/43bbe5286358d54b3f46227509969e2586e8fa4a.

            Finally, I decided to name the variable spider_enable_group_by_handler. I now think that a name like spider_enable_direct_XX will lead to confusion, because the word "direct" is used in many ways in the Spider storage engine.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - Please review https://github.com/MariaDB/server/commit/43bbe5286358d54b3f46227509969e2586e8fa4a . Finally, I decided to name the variable spider_enable_group_by_handler . I now think that a name like spider_enable_direct_XX will lead to confusion, because the word "direct" is used in many ways in the Spider storage engine.

            may be spider_direct_group_by ? to avoid the internal implementation name of class group_by_handler.

            Generally, I don't like this change, I think it's a step in the wrong direction, instead of us fixing bugs it encourages users (and us) to simply disable the feature. I believe a feature should either be enabled and used or removed.

            But as it's purely Spider patch, it's ultimately up to you to decide whether to push it or not.

            Code-wise it's, of course, ok.

            serg Sergei Golubchik added a comment - may be spider_direct_group_by ? to avoid the internal implementation name of class group_by_handler . Generally, I don't like this change, I think it's a step in the wrong direction, instead of us fixing bugs it encourages users (and us) to simply disable the feature. I believe a feature should either be enabled and used or removed. But as it's purely Spider patch, it's ultimately up to you to decide whether to push it or not. Code-wise it's, of course, ok.

            Thank you for your review. However, I changed my mind and will try to handle the problem in MDEV-27260. So, won't do.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - Thank you for your review. However, I changed my mind and will try to handle the problem in MDEV-27260 . So, won't do.

            People

              nayuta-yanagisawa Nayuta Yanagisawa (Inactive)
              nayuta-yanagisawa Nayuta Yanagisawa (Inactive)
              Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.