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

            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+.

            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+.

            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 )

            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.