Details

    Description

      This is for plugin userstat:

      Add "Queries" to INDEX_STATISTICS that is incremented for each query
      the index is part of.

      This allows one to see how effective the index is.

      Attachments

        Issue Links

          Activity

            to test this feature separately please use bb-11.5-MDEV-33152-index-statistics

            serg Sergei Golubchik added a comment - to test this feature separately please use bb-11.5- MDEV-33152 -index-statistics

            Testing results:

            1. Two bugs are filed as tentative blockers for the feature:

            • MDEV-33881 INDEX_STATISTICS skip system tables inconsistently
            • MDEV-33901 INDEX_STATISTICS.QUERIES is incremented additionally for subqueries

            They should be looked at before the feature is released with RC. However, if as a result of the analysis they are determined to be not bugs or not critical enough to prevent the feature release, I won't dispute it. For "not bugs", please redirect the JIRA items to documentation. For "unimportant", please downgrade the issues in JIRA.

            2. Two bugs are filed as not blocking the feature release:

            • MDEV-33903 For stored functions INDEX_STATISTICS.QUERIES only counts the top-most query
            • MDEV-33904 For triggers INDEX_STATISTICS.QUERIES only counts the triggering query

            It would be nice if they were looked at before the release and, similarly to those in (1), redirected to documentation if they are not bugs.

            3. A number of legacy bugs were filed as a by-product of the feature testing:
            MDEV-33883 MDEV-33885 MDEV-33886 MDEV-33890 MDEV-33900 MDEV-33902 MDEV-33905
            They are not directly related to the feature implemented in the scope of the task, but concern the old baseline functionality which this feature extends. As such, they limited the testing to some extent.
            It is possible that most of them would also be ruled out as intended behavior, but I don't expect that anyone will look at them any time soon.

            4. MTR coverage flagged 2 lines of the feature-related changes as not covered, even after running Galera suites with --big (line numbers as of 802e89b17):

            ===File sql/wsrep_mysqld.cc:
               1472 : +        (!tbl && !is_stat_table(&tables->db, &tables->alias)))
               1492 : +                        !strcmp(tbl->s->db.str, "information_schema"));
            

            The first line appears to be a false-positive, probably related to the way GCOV sometimes handles branches. There is however a question about validity of the line itself. The line is a part of the following fragment:

                if ((tbl && tbl->s->table_category != TABLE_CATEGORY_STATISTICS) ||
                    (!tbl && !is_stat_table(&tables->db, &tables->alias)))
                {
                  if (tbl->s->primary_key == MAX_KEY &&
            

            So,if (!tbl && !is_stat_table(&tables->db, &tables->alias))) ever turns out to be true, there will be a big problem with the next line. I haven't hit this situation though, so hopefully it's just an excessive condition which is always false.

            The second GCOV miss, line 1492, is real. Something like this in the context of Galera tests should cover it:

            delete from mysql.wsrep_cluster;
            

            or maybe this as it looks safer:

            delete from mysql.wsrep_streaming_log;
            

            5. main.userstat test fails on bb-11.5-MDEV-33152-index-statistics deterministically. The reason for the failure is the above-mentioned MDEV-33881. In other trees, e.g. bb-11.5-monty or bb-11.5-MDEV-33151-userstat, to avoid the failure, the test and result files were amended like this: change in main.userstat. So, if it's decided that MDEV-33881 is not to be fixed, then the test needs to be modified before the feature is merged into main. If MDEV-33881 is fixed to allow system tables again, hopefully it won't be necessary.


            If the points above lead to non-trivial code changes, then another (short) round of testing may be needed. Otherwise I don't object releasing the feature with 11.5 RC.

            elenst Elena Stepanova added a comment - Testing results: 1. Two bugs are filed as tentative blockers for the feature: MDEV-33881 INDEX_STATISTICS skip system tables inconsistently MDEV-33901 INDEX_STATISTICS.QUERIES is incremented additionally for subqueries They should be looked at before the feature is released with RC. However, if as a result of the analysis they are determined to be not bugs or not critical enough to prevent the feature release, I won't dispute it. For "not bugs", please redirect the JIRA items to documentation. For "unimportant", please downgrade the issues in JIRA. 2. Two bugs are filed as not blocking the feature release: MDEV-33903 For stored functions INDEX_STATISTICS.QUERIES only counts the top-most query MDEV-33904 For triggers INDEX_STATISTICS.QUERIES only counts the triggering query It would be nice if they were looked at before the release and, similarly to those in (1), redirected to documentation if they are not bugs. 3. A number of legacy bugs were filed as a by-product of the feature testing: MDEV-33883 MDEV-33885 MDEV-33886 MDEV-33890 MDEV-33900 MDEV-33902 MDEV-33905 They are not directly related to the feature implemented in the scope of the task, but concern the old baseline functionality which this feature extends. As such, they limited the testing to some extent. It is possible that most of them would also be ruled out as intended behavior, but I don't expect that anyone will look at them any time soon. 4. MTR coverage flagged 2 lines of the feature-related changes as not covered, even after running Galera suites with --big (line numbers as of 802e89b17 ): ===File sql/wsrep_mysqld.cc: 1472 : + (!tbl && !is_stat_table(&tables->db, &tables->alias))) 1492 : + !strcmp(tbl->s->db.str, "information_schema")); The first line appears to be a false-positive, probably related to the way GCOV sometimes handles branches. There is however a question about validity of the line itself. The line is a part of the following fragment: if ((tbl && tbl->s->table_category != TABLE_CATEGORY_STATISTICS) || (!tbl && !is_stat_table(&tables->db, &tables->alias))) { if (tbl->s->primary_key == MAX_KEY && So,if (!tbl && !is_stat_table(&tables->db, &tables->alias))) ever turns out to be true, there will be a big problem with the next line. I haven't hit this situation though, so hopefully it's just an excessive condition which is always false. The second GCOV miss, line 1492, is real. Something like this in the context of Galera tests should cover it: delete from mysql.wsrep_cluster; or maybe this as it looks safer: delete from mysql.wsrep_streaming_log; 5. main.userstat test fails on bb-11.5- MDEV-33152 -index-statistics deterministically. The reason for the failure is the above-mentioned MDEV-33881 . In other trees, e.g. bb-11.5-monty or bb-11.5- MDEV-33151 -userstat, to avoid the failure, the test and result files were amended like this: change in main.userstat . So, if it's decided that MDEV-33881 is not to be fixed, then the test needs to be modified before the feature is merged into main. If MDEV-33881 is fixed to allow system tables again, hopefully it won't be necessary. If the points above lead to non-trivial code changes, then another (short) round of testing may be needed. Otherwise I don't object releasing the feature with 11.5 RC.

            Regarding the code:

            if ((tbl && tbl->s->table_category != TABLE_CATEGORY_STATISTICS) ||
            (!tbl && !is_stat_table(&tables->db, &tables->alias)))

            This is an exact rework of the original code:

            if (!is_stat_table(&tables->db, &tables->alias))

            The original code would also crash if tbl is 0.

            I am not sure if there could be a case where tbl is 0 and the table is a stat table. I tried to ask on Galera channel but did not get an answer, which is why I kept the logic as as the original code.

            monty Michael Widenius added a comment - Regarding the code: if ((tbl && tbl->s->table_category != TABLE_CATEGORY_STATISTICS) || (!tbl && !is_stat_table(&tables->db, &tables->alias))) This is an exact rework of the original code: if (!is_stat_table(&tables->db, &tables->alias)) The original code would also crash if tbl is 0. I am not sure if there could be a case where tbl is 0 and the table is a stat table. I tried to ask on Galera channel but did not get an answer, which is why I kept the logic as as the original code.

            Fixes for MDEV-33881 and MDEV-33901 were pushed into bb-11.5-monty and cherry-picked into bb-11.5-MDEV-33152-index-statistics.

            Note:
            The fix for MDEV-33881 was pushed into bb-11.5-monty as "squash 8372d62a863008ea6faa1547db5c275a5863f167", that is, as a postfix for MDEV-33151 rather than MDEV-33152.
            The bug MDEV-33881 was filed in the scope of MDEV-33152 testing; the fix semantically applies to both/either of MDEV-33151 and MDEV-33152; but the patch was causing conflicts when I was trying to apply it to bb-11.5-MDEV-33151-userstat, while it applied smoothly to bb-11.5-MDEV-33152-index-statistics, so I pushed it there.

            I took a quick look without performing full re-testing, the patches seem to be doing what they are claimed to do, I think they can and should be incorporated.

            The resulting commit is, as of now, bb-11.5-MDEV-33152-index-statistics 15930619.

            However, it cannot be merged into 11.5 as is. The point (5) from my previous comment still applies: even though MDEV-33881 was fixed, it wasn't in the direction that would make the test failure go away, so main.userstat still fails deterministically. Either it needs to be pushed together with MDEV-33151 where the test was adjusted, or the adjustment should be applied to bb-11.5-MDEV-33152-index-statistics.

            elenst Elena Stepanova added a comment - Fixes for MDEV-33881 and MDEV-33901 were pushed into bb-11.5-monty and cherry-picked into bb-11.5- MDEV-33152 -index-statistics. Note: The fix for MDEV-33881 was pushed into bb-11.5-monty as "squash 8372d62a863008ea6faa1547db5c275a5863f167", that is, as a postfix for MDEV-33151 rather than MDEV-33152 . The bug MDEV-33881 was filed in the scope of MDEV-33152 testing; the fix semantically applies to both/either of MDEV-33151 and MDEV-33152 ; but the patch was causing conflicts when I was trying to apply it to bb-11.5- MDEV-33151 -userstat, while it applied smoothly to bb-11.5- MDEV-33152 -index-statistics, so I pushed it there. I took a quick look without performing full re-testing, the patches seem to be doing what they are claimed to do, I think they can and should be incorporated. The resulting commit is, as of now, bb-11.5-MDEV-33152-index-statistics 15930619 . However, it cannot be merged into 11.5 as is . The point (5) from my previous comment still applies: even though MDEV-33881 was fixed, it wasn't in the direction that would make the test failure go away, so main.userstat still fails deterministically. Either it needs to be pushed together with MDEV-33151 where the test was adjusted, or the adjustment should be applied to bb-11.5- MDEV-33152 -index-statistics.

            People

              serg Sergei Golubchik
              monty Michael Widenius
              Votes:
              0 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.