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

Spider: remove pattern str->reserve() then str->q_append()

Details

    Description

      Remove the following pattern that can be seen anywhere in the Spider codebase.

      if (str->reserve(SPIDER_SQL_WHERE_LEN))
        DBUG_RETURN(HA_ERR_OUT_OF_MEM);
      str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
      

      This makes the code less readable and is error-prone (e.g., MDEV-27184).

      Attachments

        Issue Links

          Activity

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) created issue -
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Field Original Value New Value
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Description We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the member functions of {{spider_string}}, which need the manual memory allocation, by the functions that automatically allocate memory.
            We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by the functions that automatically allocate memory. Then, we can gradually replace the replaced ones with their smart counterpart.
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Summary Spider: stop using spider_string::reserve(uint32 space_needed) Deprecate spider_string member functions assuming manual memory allocation
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Fix Version/s 10.3 [ 22126 ]
            Fix Version/s 10.4 [ 22408 ]
            Fix Version/s 10.5 [ 23123 ]
            Fix Version/s 10.6 [ 24028 ]
            Fix Version/s 10.7 [ 24805 ]
            Fix Version/s 10.8 [ 26121 ]
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Description We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by the functions that automatically allocate memory. Then, we can gradually replace the replaced ones with their smart counterpart.
            We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by their corresponding functions that automatically allocate memory. Then, we can gradually replace the replaced ones with their smart correspondences.
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Description We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by their corresponding functions that automatically allocate memory. Then, we can gradually replace the replaced ones with their smart correspondences.
            We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by their corresponding functions that automatically allocate memory. Then, we can gradually replace the old functions with their smart correspondences.
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Description We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by their corresponding functions that automatically allocate memory. Then, we can gradually replace the old functions with their smart correspondences.
            We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by their corresponding smart functions that automatically allocate memory. Then, we can gradually replace the old functions with their smart correspondences.

            that'd be str->append(). q_append() means "quick append" that doesn't check if there's enough space and assumes the caller knows what he's doing. Invoking reserve() explicitly every time before q_append() makes little sense, one can just as well use append().

            serg Sergei Golubchik added a comment - that'd be str->append() . q_append() means "quick append" that doesn't check if there's enough space and assumes the caller knows what he's doing. Invoking reserve() explicitly every time before q_append() makes little sense, one can just as well use append() .

            Thank you for your comment.

            I would like to replace all q_appends with append, but that would be too big of a change. So, I will make spider_string::q_append() behave exactly same as spider_string::append(). Then, I'd gradually remove q_apppend(). I will do this on 10.3+.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - - edited Thank you for your comment. I would like to replace all q_appends with append, but that would be too big of a change. So, I will make spider_string::q_append() behave exactly same as spider_string::append() . Then, I'd gradually remove q_apppend() . I will do this on 10.3+.
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Status Open [ 1 ] In Progress [ 3 ]

            I noticed that we cannot simply replace spider_string::q_append() with spider_string::append() because Spider relies on the fact that q_append() never call String::realloc_with_efraxtra_if_needed(). realloc_with_efraxtra_if_needed() might wipes the data, in String::Ptr, which is placed beyond String::str_length.

            For example, the test spider.ha fails, if we do the simple replacement. That is because spider_mbase_share::append_table_name_with_adjusting() behaves wrongly.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - - edited I noticed that we cannot simply replace spider_string::q_append() with spider_string::append() because Spider relies on the fact that q_append() never call String::realloc_with_efraxtra_if_needed() . realloc_with_efraxtra_if_needed() might wipes the data, in String::Ptr , which is placed beyond String::str_length . For example, the test spider.ha fails, if we do the simple replacement. That is because spider_mbase_share::append_table_name_with_adjusting() behaves wrongly.

            I think that we can avoid relying on the implicit assumption by using spider_string::replace() instead of spider_string::length() + spider_string::q_append(). However, I wouldn't like to make major change in GA older versions. So, I will limit the scope of the issue to 10.9+.

            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - - edited I think that we can avoid relying on the implicit assumption by using spider_string::replace() instead of spider_string::length() + spider_string::q_append() . However, I wouldn't like to make major change in GA older versions. So, I will limit the scope of the issue to 10.9+.
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Status In Progress [ 3 ] Stalled [ 10000 ]
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Summary Deprecate spider_string member functions assuming manual memory allocation Spider: remove pattern str->reserve() then str->q_append()
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Description We do see the following anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).

            Replace the implementation of the {{spider_string}} member functions, which need the manual memory allocation, by their corresponding smart functions that automatically allocate memory. Then, we can gradually replace the old functions with their smart correspondences.
            Remove the following pattern that can be seen anywhere in the Spider codebase.

            {code:cpp}
            if (str->reserve(SPIDER_SQL_WHERE_LEN))
              DBUG_RETURN(HA_ERR_OUT_OF_MEM);
            str->q_append(SPIDER_SQL_WHERE_STR, SPIDER_SQL_WHERE_LEN);
            {code}

            This makes the code less readable and is error-prone (e.g., MDEV-27184).
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Fix Version/s 10.9 [ 26905 ]
            Fix Version/s 10.3 [ 22126 ]
            Fix Version/s 10.4 [ 22408 ]
            Fix Version/s 10.5 [ 23123 ]
            Fix Version/s 10.6 [ 24028 ]
            Fix Version/s 10.7 [ 24805 ]
            Fix Version/s 10.8 [ 26121 ]

            spider_string::q_append and other methods are modelled after String methods in sql/sql_string.h.

            I think it'd be very confusing if methods with identical names (e.g. q_append) will start behaving differently in spider_string and String)

            serg Sergei Golubchik added a comment - spider_string::q_append and other methods are modelled after String methods in sql/sql_string.h . I think it'd be very confusing if methods with identical names (e.g. q_append ) will start behaving differently in spider_string and String )
            serg Sergei Golubchik made changes -
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Fix Version/s 10.10 [ 27530 ]
            Fix Version/s 10.9 [ 26905 ]
            nayuta-yanagisawa Nayuta Yanagisawa (Inactive) made changes -
            Fix Version/s 10.11 [ 27614 ]
            Fix Version/s 10.10 [ 27530 ]
            ycp Yuchen Pei made changes -
            Assignee Nayuta Yanagisawa [ JIRAUSER47117 ] Yuchen Pei [ JIRAUSER52627 ]

            People

              ycp Yuchen Pei
              nayuta-yanagisawa Nayuta Yanagisawa (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:

                Git Integration

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