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

[PATCH] Sys_query_cache_limit initialization depends on initialization in other source files

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 10.0.2, 5.5.31
    • Fix Version/s: 10.0.4, 5.5.33
    • Component/s: None
    • Labels:
      None

      Description

      Sys_query_cache_limit variable has query_cache.query_cache_limit as a place to write its value to. But it's a member of a class that has constructor and so initialization of Sys_query_cache_limit (which assigns default value to the variable) is dependent on initialization of query_cache to happen earlier. As these variables are in different source files order of initialization between them is not defined.

      The attached patch fixes Sys_query_cache_limit to point to global variable instead of class member.

      Detected by new feature of AddressSanitizer – check_initialization_order.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            serg Sergei Golubchik added a comment -

            Sanja, please fix that in 5.5, if applicable.

            Show
            serg Sergei Golubchik added a comment - Sanja, please fix that in 5.5, if applicable.
            Hide
            sanja Oleksandr Byelkin added a comment -

            As I can see fix_query_cache_limit (as well other fix* functions) is not called on initialization with default value so I do not see how the patch can help except hiding the problem... maybe I am wrong, still checking...

            Show
            sanja Oleksandr Byelkin added a comment - As I can see fix_query_cache_limit (as well other fix* functions) is not called on initialization with default value so I do not see how the patch can help except hiding the problem... maybe I am wrong, still checking...
            Hide
            pivanof Pavel Ivanov added a comment -

            Actually if fix_query_cache_limit function will be called during initialization then it will cause the same problem again and patch won't fix anything. If fix_query_cache_limit functions is not called on initialization then the patch reaches its goal – query_cache and Sys_query_cach_limit are initialized independently from each other and don't touch each other's data during initialization.

            BTW, I noticed that query_cache initializes query_cache_limit to ULONG_MAX while Sys_query_cache_limit has maximum of UINT_MAX, so it may be worth checking what these values should be.

            Show
            pivanof Pavel Ivanov added a comment - Actually if fix_query_cache_limit function will be called during initialization then it will cause the same problem again and patch won't fix anything. If fix_query_cache_limit functions is not called on initialization then the patch reaches its goal – query_cache and Sys_query_cach_limit are initialized independently from each other and don't touch each other's data during initialization. BTW, I noticed that query_cache initializes query_cache_limit to ULONG_MAX while Sys_query_cache_limit has maximum of UINT_MAX, so it may be worth checking what these values should be.
            Hide
            sanja Oleksandr Byelkin added a comment -

            I think setting result size limit could be moved to QC initialization, and this (with above patch) will solve the problem.

            Show
            sanja Oleksandr Byelkin added a comment - I think setting result size limit could be moved to QC initialization, and this (with above patch) will solve the problem.
            Hide
            serg Sergei Golubchik added a comment - - edited

            Strictly speaking, there is no bug here. All command-line options (and query_cache_limit is a command-line option) are initialized run-time from handle_options() function of my_getopt.c. So whatever static initialization does and in what order is irrelevant.

            But okay, one can argue that it's a fragile assumption to rely on (if we'd decide to remove a possibility of setting query_cache_limit from the command line, it will make its initial value non-deterministic). Let's fix it.

            Oleksandr Byelkin — ok to push.

            Show
            serg Sergei Golubchik added a comment - - edited Strictly speaking, there is no bug here. All command-line options (and query_cache_limit is a command-line option) are initialized run-time from handle_options() function of my_getopt.c. So whatever static initialization does and in what order is irrelevant. But okay, one can argue that it's a fragile assumption to rely on (if we'd decide to remove a possibility of setting query_cache_limit from the command line, it will make its initial value non-deterministic). Let's fix it. Oleksandr Byelkin — ok to push.
            Hide
            sanja Oleksandr Byelkin added a comment -

            pushed to 10.0

            Show
            sanja Oleksandr Byelkin added a comment - pushed to 10.0

              People

              • Assignee:
                sanja Oleksandr Byelkin
                Reporter:
                pivanof Pavel Ivanov
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 20 minutes
                  20m