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

Documentation on spider_casual_read is insufficient/non-existing

Details

    Description

      It is not clear from this document:

      https://mariadb.com/docs/reference/mdb/cli/mariadbd/spider-casual-read/

      and KB:

      https://mariadb.com/kb/en/spider-server-system-variables/#spider_casual_read

      what is the "connection channel" and what settings should be used and at what level for typical use cases:

      1. The overall data node is 4 nodes and there are two partitions that point to the same data node.

      CREATE TABLE spider (
      :
      ) ENGINE SPIDER PARTITION BY xxx (
      pt1 ... ENGINE SPIDER COMMENT 'srv datanode1' ,
      pt2 ... ENGINE SPIDER COMMENT 'srv datanode1' ,
      pt3 ... ENGINE SPIDER COMMENT 'srv datanode2' ,
      pt4 ... ENGINE SPIDER COMMENT 'srv datanode3' ,
      ptmax ... ENGINE SPIDER COMMENT 'srv datanode4'

      )

      2. Case with four overall data nodes and three partitions pointing to the same data node


      CREATE TABLE spider (
      :
      ) ENGINE SPIDER PARTITION BY xxx (
      pt1 ... ENGINE SPIDER COMMENT 'srv datanode1' ,
      pt2 ... ENGINE SPIDER COMMENT 'srv datanode1' ,
      pt3 ... ENGINE SPIDER COMMENT 'srv datanode1' ,
      pt4 ... ENGINE SPIDER COMMENT 'srv datanode2' ,
      pt5 ... ENGINE SPIDER COMMENT 'srv datanode3' ,
      ptmax ... ENGINE SPIDER COMMENT 'srv datanode4'

      )

      3. Case where the overall data node is 4 nodes and every partition points to a different data node


      CREATE TABLE spider (
      :
      ) ENGINE SPIDER PARTITION BY xxx (
      pt1 ... ENGINE SPIDER COMMENT 'srv datanode1',
      pt2 ... ENGINE SPIDER COMMENT 'srv datanode2',
      pt3 ... ENGINE SPIDER COMMENT 'srv datanode3',
      pt4 ... ENGINE SPIDER COMMENT 'srv datanode4'
      )

      4. Case where all partitions point to a local table


      CREATE TABLE spider (
      :
      ) ENGINE SPIDER PARTITION BY xxx (
      pt1 ... ENGINE InnoDB,
      pt2 ... ENGINE InnoDB,
      pt3 ... ENGINE InnoDB,
      pt4 ... ENGINE InnoDB
      )

      So, can you, please, clarify in the documentation, Enterprise or KB, or both, what value to set for spider_casual_read, and on what level, table or global, for each of these 4 cases?

      Attachments

        Issue Links

          Activity

            The word "a connection channel" refers to a database connection to a data node (the Spider SE may have multiple channels to a single data node). With more partitions on a Spider table, you would have more chances to leverage parallelism. As the documentation said, this is true even if multiple shards are stored on the same physical server. Please note that it quite depends on how much the response time is reduced by parallel search, so it is best to have the actual measurement done in the user's environment.

            Setting spider_casual_read=X (X in [2, 3..., 63]) is to let the Spider storage engine use the specific channel of No. X to each data node. This setting seems to be for fine-tuning and sometimes results in an excessive tweak. So, if one wants to benefit from a parallel read, I recommend setting spider_casual_read = 1 in most cases.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - The word "a connection channel" refers to a database connection to a data node (the Spider SE may have multiple channels to a single data node). With more partitions on a Spider table, you would have more chances to leverage parallelism. As the documentation said, this is true even if multiple shards are stored on the same physical server. Please note that it quite depends on how much the response time is reduced by parallel search, so it is best to have the actual measurement done in the user's environment. Setting spider_casual_read=X (X in [2, 3..., 63]) is to let the Spider storage engine use the specific channel of No. X to each data node. This setting seems to be for fine-tuning and sometimes results in an excessive tweak. So, if one wants to benefit from a parallel read, I recommend setting spider_casual_read = 1 in most cases.
            ycp Yuchen Pei added a comment -

            The best way to know for sure the behaviour of this parameter is to find or create a test covering it. I will be working on this.

            ycp Yuchen Pei added a comment - The best way to know for sure the behaviour of this parameter is to find or create a test covering it. I will be working on this.
            ycp Yuchen Pei added a comment -

            As I suspected, none of the tests cover the case where
            spider_check_and_get_casual_read_conn() is called and
            result_list->casual_read[link_idx] is nonzero. So we have to add a
            test.

            To make this task more "interesting",
            spider_check_and_get_casual_read_conn() is only called when
            bgs_phase is nonzero, which implies bgs_mode is nonzero. This
            is why only one test outside of the spider/bg subsuite goes there.
            And it is "interesting" because bgs stands for "background search",
            yet another undocumented feature in spider.

            ycp Yuchen Pei added a comment - As I suspected, none of the tests cover the case where spider_check_and_get_casual_read_conn() is called and result_list->casual_read [link_idx] is nonzero. So we have to add a test. To make this task more "interesting", spider_check_and_get_casual_read_conn() is only called when bgs_phase is nonzero, which implies bgs_mode is nonzero. This is why only one test outside of the spider/bg subsuite goes there. And it is "interesting" because bgs stands for "background search", yet another undocumented feature in spider.
            ycp Yuchen Pei added a comment - - edited

            After looking into the code, and creating, running and inspecting
            code paths tests, I am convinced that the casual read is unnecessary
            and therefore should always be left alone (i.e. default value 0 i.e.
            disabled), because

            1. it only takes effect when bgs mode is on (bgs_mode > 0), and
            direct_order_limit or direct_aggregate is on
            2. when it is enabled and takes effect,
            spider_check_and_get_casual_read_conn() creates at least one
            background thread for each link-query combination, where link is a
            data node. I doubt the function
            spider_check_and_get_casual_read_conn() is ever called more than
            once for the same link-query combination*, in which case it creates
            exactly one extra background thread. Furthermore, the case where
            the connection key is supposed to be incremented due to casual read
            value being 1 is likely not gonna happen, because the latter gets
            overwritten and I doubt it gets reset when the query_id is not
            changed** (it gets reset, for example, at the end of a query,
            through ha_spider::reset):

            // in spider_check_and_get_casual_read_conn()
              if (spider->result_list.casual_read[link_idx] == 1)
              {
                spider->result_list.casual_read[link_idx] = conn->casual_read_current_id;
                ++conn->casual_read_current_id;
            // in spider_check_and_init_casual_read(), the caller of spider_check_and_get_casual_read_conn()
                if (!result_list->casual_read[link_idx])
                {
                  result_list->casual_read[link_idx] =
                    spider_param_casual_read(thd, share->casual_read);
                }
                if ((error_num = spider_check_and_get_casual_read_conn(thd, spider,
                  link_idx)))
               

            3. Whenever casual read take effect, a bg thread has been created for
            the same purpose already:

            // before every call site of spider_check_and_init_casual_read()
              if ((error_num = spider_set_conn_bg_param(this)))
                DBUG_RETURN(error_num);
               

            While working on this ticket, I also did some code cleanup, which I
            will finish up and send for review - that is MDEV-31787.

            I will also open a ticket to deprecate and remove this sysvar /
            table param - that is MDEV-31789.

            Disclaimer: this is not a mathematical proof, and rests on the two
            assumptions marked with * and ** above.

            Update on [2023-07-31 Mon]: Regarding the assumption * above, I did
            some experiments, and here's what I got:

            1. A relaxed assumption does not hold: we have that the same
            conn-query combination can be encountered in
            spider_check_and_init_casual_read() (not different function
            name) more than once, see commit [1] where a bunch of spider/bg
            tests fail on the abort()
            2. A less relaxed assumption does hold for all tests: the same
            conn-query combination is never encountered in
            spider_check_and_get_casual_read() more than noce, see commit
            [2] - none of the tests break.
            3. Even if the same conn-query combination is encountered more than
            once in spider_check_and_get_casual_read() in a specific
            query, the existing implementation of casual_read == 1 simply
            will create only one extra thread, effectively making it useless.
            To see this, note that spider_get_conn() overwrites
            spider->conn_keys[link_idx] with the new connection, which
            has the initial query_id of 0, so the check against
            thd->query_id will fail and reset the conn_key to 2.
            4. To fix 3., we need to make assumptions about the real intention
            of casual read. For example, if we assume casual_read == 1 means
            creating a new connection/thread for the same link-query, perhaps
            we could do something like [3]. But fixing casual read is out of
            the scope of this ticket, which has the presmise that the feature
            is already implemented properly, nor is it as trivial as [3],
            because we don't have testcases that exercise this assumption
            (too many conditions required: bgs mode, direct order/aggregate,
            entering a specific function at least twice with the same
            link-query combination), and the code paths as they are now are
            too complicated to understand without such testcases.

            So what do we do? I think there are two possible approaches:

            1. We take casual read at face value, in which case it is pretty
            useless and should be disabled, deprecated and deleted as I
            recommended above.
            2. We redo the implementation of casual read, before coming back to
            this ticket, which is a lot more work.

            I still think 1 is much better, as spider code is too legacy and
            complex and we should delete code whenever possible, and there does
            not seem to be a usecase afaik for adding extra background threads
            for each link when there's already one.

            [1] https://github.com/MariaDB/server/commit/fe5ca4a3f58
            [2] https://github.com/MariaDB/server/commit/5d45e7a0bd6
            [3] https://github.com/MariaDB/server/commit/a7921996536

            Also holyfoot, feel free to chime in if you have thoughts about
            this

            ycp Yuchen Pei added a comment - - edited After looking into the code, and creating, running and inspecting code paths tests, I am convinced that the casual read is unnecessary and therefore should always be left alone (i.e. default value 0 i.e. disabled), because 1. it only takes effect when bgs mode is on (bgs_mode > 0), and direct_order_limit or direct_aggregate is on 2. when it is enabled and takes effect, spider_check_and_get_casual_read_conn() creates at least one background thread for each link-query combination, where link is a data node. I doubt the function spider_check_and_get_casual_read_conn() is ever called more than once for the same link-query combination*, in which case it creates exactly one extra background thread. Furthermore, the case where the connection key is supposed to be incremented due to casual read value being 1 is likely not gonna happen, because the latter gets overwritten and I doubt it gets reset when the query_id is not changed** (it gets reset, for example, at the end of a query, through ha_spider::reset): // in spider_check_and_get_casual_read_conn() if (spider->result_list.casual_read[link_idx] == 1) { spider->result_list.casual_read[link_idx] = conn->casual_read_current_id; ++conn->casual_read_current_id; // in spider_check_and_init_casual_read(), the caller of spider_check_and_get_casual_read_conn() if (!result_list->casual_read[link_idx]) { result_list->casual_read[link_idx] = spider_param_casual_read(thd, share->casual_read); } if ((error_num = spider_check_and_get_casual_read_conn(thd, spider, link_idx))) 3. Whenever casual read take effect, a bg thread has been created for the same purpose already: // before every call site of spider_check_and_init_casual_read() if ((error_num = spider_set_conn_bg_param( this ))) DBUG_RETURN(error_num); While working on this ticket, I also did some code cleanup, which I will finish up and send for review - that is MDEV-31787 . I will also open a ticket to deprecate and remove this sysvar / table param - that is MDEV-31789 . Disclaimer: this is not a mathematical proof, and rests on the two assumptions marked with * and ** above. Update on [2023-07-31 Mon] : Regarding the assumption * above, I did some experiments, and here's what I got: 1. A relaxed assumption does not hold: we have that the same conn-query combination can be encountered in spider_check_and_init_casual_read() (not different function name) more than once, see commit [1] where a bunch of spider/bg tests fail on the abort() 2. A less relaxed assumption does hold for all tests: the same conn-query combination is never encountered in spider_check_and_get_casual_read() more than noce, see commit [2] - none of the tests break. 3. Even if the same conn-query combination is encountered more than once in spider_check_and_get_casual_read() in a specific query, the existing implementation of casual_read == 1 simply will create only one extra thread, effectively making it useless. To see this, note that spider_get_conn() overwrites spider->conn_keys [link_idx] with the new connection, which has the initial query_id of 0, so the check against thd->query_id will fail and reset the conn_key to 2. 4. To fix 3., we need to make assumptions about the real intention of casual read. For example, if we assume casual_read == 1 means creating a new connection/thread for the same link-query, perhaps we could do something like [3] . But fixing casual read is out of the scope of this ticket, which has the presmise that the feature is already implemented properly, nor is it as trivial as [3] , because we don't have testcases that exercise this assumption (too many conditions required: bgs mode, direct order/aggregate, entering a specific function at least twice with the same link-query combination), and the code paths as they are now are too complicated to understand without such testcases. So what do we do? I think there are two possible approaches: 1. We take casual read at face value, in which case it is pretty useless and should be disabled, deprecated and deleted as I recommended above. 2. We redo the implementation of casual read, before coming back to this ticket, which is a lot more work. I still think 1 is much better, as spider code is too legacy and complex and we should delete code whenever possible, and there does not seem to be a usecase afaik for adding extra background threads for each link when there's already one. [1] https://github.com/MariaDB/server/commit/fe5ca4a3f58 [2] https://github.com/MariaDB/server/commit/5d45e7a0bd6 [3] https://github.com/MariaDB/server/commit/a7921996536 Also holyfoot , feel free to chime in if you have thoughts about this
            ycp Yuchen Pei added a comment -

            Following up on discussions with valerii, let me wrap up this
            ticket.

            Basically, my understanding after testing and analysing the code is
            that spider_casual_read does not have any positive impact on the
            query performance.

            Whenever the casual read value is used, a background thread has
            already been created per partition. Whether the value is 0 or 1 or 23
            does not matter.

            Take the test spider/bg.direct_aggregate_part[1] for example. This
            is an example of select count(*) from T where T is partitioned spider
            table referring to two different servers. The original test has casual
            read 0, and it already creates a background thread for each partition.
            I tested it with spider_casual_read to 1, as well as 23[2]. Again it
            confirms my understanding, i.e. the these nonzero casual read values,
            when in effect, create an extra bg thread to run the query on. The
            previously created bg thread for the same link/partition just stays
            idle for the rest of the query. So no gains whatsoever.

            [1] https://github.com/MariaDB/server/blob/11.2/storage/spider/mysql-test/spider/bg/t/direct_aggregate_part.test
            [2] https://github.com/MariaDB/server/commit/5f5a24eb53f

            For more detailed analysis, see my previous comments in this ticket.
            Closing.

            ycp Yuchen Pei added a comment - Following up on discussions with valerii , let me wrap up this ticket. Basically, my understanding after testing and analysing the code is that spider_casual_read does not have any positive impact on the query performance. Whenever the casual read value is used, a background thread has already been created per partition. Whether the value is 0 or 1 or 23 does not matter. Take the test spider/bg.direct_aggregate_part [1] for example. This is an example of select count(*) from T where T is partitioned spider table referring to two different servers. The original test has casual read 0, and it already creates a background thread for each partition. I tested it with spider_casual_read to 1, as well as 23 [2] . Again it confirms my understanding, i.e. the these nonzero casual read values, when in effect, create an extra bg thread to run the query on. The previously created bg thread for the same link/partition just stays idle for the rest of the query. So no gains whatsoever. [1] https://github.com/MariaDB/server/blob/11.2/storage/spider/mysql-test/spider/bg/t/direct_aggregate_part.test [2] https://github.com/MariaDB/server/commit/5f5a24eb53f For more detailed analysis, see my previous comments in this ticket. Closing.

            Should I create a new MDEV/task to change KB (https://mariadb.com/kb/en/spider-server-system-variables/#spider_casual_read) according to the results presented in this MDEV? Current text there still seems misleading.

            valerii Valerii Kravchuk added a comment - Should I create a new MDEV/task to change KB ( https://mariadb.com/kb/en/spider-server-system-variables/#spider_casual_read ) according to the results presented in this MDEV? Current text there still seems misleading.
            ycp Yuchen Pei added a comment -

            > Should I create a new MDEV/task to change KB
            > (https://mariadb.com/kb/en/spider-server-system-variables/#spider_casual_read)
            > according to the results presented in this MDEV? Current text there
            > still seems misleading.

            That's fine by me.

            ycp Yuchen Pei added a comment - > Should I create a new MDEV/task to change KB > ( https://mariadb.com/kb/en/spider-server-system-variables/#spider_casual_read ) > according to the results presented in this MDEV? Current text there > still seems misleading. That's fine by me.

            People

              AnneStrasser Anne Strasser (Inactive)
              valerii Valerii Kravchuk
              Votes:
              0 Vote for this issue
              Watchers:
              8 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.