Details

    Description

      MDEV-11371 introduced a change to libmariadb:

      commit e069fb8e76eeab096b8255805244f73048e3575a (HEAD, origin/svoj-MDEV-11371)
      Author: Sergey Vojtovich <svoj@mariadb.org>
      Date:   Thu Aug 31 15:09:10 2017 +0400
       
          MDEV-11371 - column compression
       
      diff --git a/include/mariadb_com.h b/include/mariadb_com.h
      index 727c66b..8ccc171 100644
      --- a/include/mariadb_com.h
      +++ b/include/mariadb_com.h
      @@ -330,6 +330,8 @@ enum enum_field_types { MYSQL_TYPE_DECIMAL, MYSQL_TYPE_TINY,
                               MYSQL_TYPE_TIMESTAMP2,
                               MYSQL_TYPE_DATETIME2,
                               MYSQL_TYPE_TIME2,
      +                        MYSQL_TYPE_BLOB_COMPRESSED= 140,
      +                        MYSQL_TYPE_VARCHAR_COMPRESSED= 141,
                               /* --------------------------------------------- */
                               MYSQL_TYPE_JSON=245,
                               MYSQL_TYPE_NEWDECIMAL=246,
      

      According to serg this should not have been added; the parameters should be private to the server, not exposed to the client.

      Now, a merge from 10.2 is causing a conflict for libmariadb, because there have been changes in the libmariadb that is used in 10.2. This conflict needs to be resolved in some way.

      It appears that some client code is depending on the definitions. And those files should probably not include mysql_com.h to get the server-side definition.

      Attachments

        Issue Links

          Activity

            serg Sergei Golubchik added a comment - - edited

            I didn't say they should not be added, the comment in include/mysql_com.h says that.

            What I'm saying — we cannot release connector-c from some arbitrary commit in the middle of a repository. So updating libmariadb submodule to branch origin/svoj-MDEV-11371, while perfectly fine for testing, should've never been pushed into the main 10.3 branch.

            serg Sergei Golubchik added a comment - - edited I didn't say they should not be added, the comment in include/mysql_com.h says that. What I'm saying — we cannot release connector-c from some arbitrary commit in the middle of a repository. So updating libmariadb submodule to branch origin/svoj- MDEV-11371 , while perfectly fine for testing, should've never been pushed into the main 10.3 branch.

            IIRC I didn't know which branch to use because it was pointing to 11321f16bfcd92e210d5736af7b7d5073a89c2ef, which was another "some arbitrary commit in the middle of a repository".

            svoj Sergey Vojtovich added a comment - IIRC I didn't know which branch to use because it was pointing to 11321f16bfcd92e210d5736af7b7d5073a89c2ef, which was another "some arbitrary commit in the middle of a repository".

            Apparently from 10.2-serg

            svoj Sergey Vojtovich added a comment - Apparently from 10.2-serg

            So this was exclusively needed to compile mysqlbinlog.

            Fixing this problem would be trivial, if I knew which branch is supposed to be used and what revisions should be taken from 10.2-serg?

            svoj Sergey Vojtovich added a comment - So this was exclusively needed to compile mysqlbinlog. Fixing this problem would be trivial, if I knew which branch is supposed to be used and what revisions should be taken from 10.2-serg?

            10.2-serg is what I use to test fixes in bb-10.2-serg. Later I push them to 10.2-server before pushing server changes into 10.2. So it happens that 10.2-server and 10.2-serg point to the same commit at the time of the push, yes.

            Right, let's ask.

            georg, what branch should we use for 10.3? 10.2-server? 10.3-server? trunk? something else?

            serg Sergei Golubchik added a comment - 10.2-serg is what I use to test fixes in bb-10.2-serg. Later I push them to 10.2-server before pushing server changes into 10.2. So it happens that 10.2-server and 10.2-serg point to the same commit at the time of the push, yes. Right, let's ask. georg , what branch should we use for 10.3? 10.2-server? 10.3-server? trunk? something else?

            Ah, sure 10.2-server! Back then I checked master branch, which didn't have this revision.

            svoj Sergey Vojtovich added a comment - Ah, sure 10.2-server! Back then I checked master branch, which didn't have this revision.

            On the second thought, this change should've not been pushed into C/C at all.

            C/C is the client library. It contains the code to connect clients to the server. Purely internal server stuff don't belong there. At all.

            The fact that mysqlbinlog is a complex mess of server and client code is regrettable, but it does not mean that C/C should get server's definitions. There are many ways to solve it. Eventually, I hope, we'll split mysqlbinlog.cc into client and server part, that will be linked together, but compiled separately. Short-term you can define missing constants directly in the mysqlbinlog.cc

            serg Sergei Golubchik added a comment - On the second thought, this change should've not been pushed into C/C at all. C/C is the client library . It contains the code to connect clients to the server. Purely internal server stuff don't belong there. At all. The fact that mysqlbinlog is a complex mess of server and client code is regrettable, but it does not mean that C/C should get server's definitions. There are many ways to solve it. Eventually, I hope, we'll split mysqlbinlog.cc into client and server part, that will be linked together, but compiled separately. Short-term you can define missing constants directly in the mysqlbinlog.cc

            Nice idea. Still I need to know what branch to use before fixing this. Or just use 10.2-server?

            svoj Sergey Vojtovich added a comment - Nice idea. Still I need to know what branch to use before fixing this. Or just use 10.2-server?

            Update 10.3 to use the latest commit in libmariadb/10.2-server branch. This will wipe out MYSQL_TYPE_BLOB_COMPRESSED and MYSQL_TYPE_VARCHAR_COMPRESSED from libmariadb. And fix mysqlbinlog.cc somehow differently.

            serg Sergei Golubchik added a comment - Update 10.3 to use the latest commit in libmariadb/10.2-server branch. This will wipe out MYSQL_TYPE_BLOB_COMPRESSED and MYSQL_TYPE_VARCHAR_COMPRESSED from libmariadb. And fix mysqlbinlog.cc somehow differently.

            So I get a warning if I move this to mysqlbinlog.cc:

            /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc: In member function ‘uint32 table_def::calc_field_size(uint, uchar*) const’:
            /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:310:3: warning: case value ‘140’ not in enumerated type ‘enum_field_types’ [-Wswitch]
               case MYSQL_TYPE_BLOB_COMPRESSED:
               ^
            /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:300:3: warning: case value ‘141’ not in enumerated type ‘enum_field_types’ [-Wswitch]
               case MYSQL_TYPE_VARCHAR_COMPRESSED:
               ^
            /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc: In constructor ‘table_def::table_def(unsigned char*, ulong, uchar*, int, uchar*, uint16)’:
            /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:1075:7: warning: case value ‘140’ not in enumerated type ‘enum_field_types’ [-Wswitch]
                   case MYSQL_TYPE_BLOB_COMPRESSED:
                   ^
            /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:1106:7: warning: case value ‘141’ not in enumerated type ‘enum_field_types’ [-Wswitch]
                   case MYSQL_TYPE_VARCHAR_COMPRESSED:
                   ^
            

            I can probably undef enum_field_types and then copy one from mysql_com.h.

            svoj Sergey Vojtovich added a comment - So I get a warning if I move this to mysqlbinlog.cc: /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc: In member function ‘uint32 table_def::calc_field_size(uint, uchar*) const’: /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:310:3: warning: case value ‘140’ not in enumerated type ‘enum_field_types’ [-Wswitch] case MYSQL_TYPE_BLOB_COMPRESSED: ^ /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:300:3: warning: case value ‘141’ not in enumerated type ‘enum_field_types’ [-Wswitch] case MYSQL_TYPE_VARCHAR_COMPRESSED: ^ /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc: In constructor ‘table_def::table_def(unsigned char*, ulong, uchar*, int, uchar*, uint16)’: /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:1075:7: warning: case value ‘140’ not in enumerated type ‘enum_field_types’ [-Wswitch] case MYSQL_TYPE_BLOB_COMPRESSED: ^ /home/svoj/devel/maria/mariadb/sql/rpl_utility.cc:1106:7: warning: case value ‘141’ not in enumerated type ‘enum_field_types’ [-Wswitch] case MYSQL_TYPE_VARCHAR_COMPRESSED: ^ I can probably undef enum_field_types and then copy one from mysql_com.h.

            serg, could you have a quick look at 4b46dab0338ad101171c9342ff6d263c833d5387 ? It seem to pass buildbot pretty well.

            svoj Sergey Vojtovich added a comment - serg , could you have a quick look at 4b46dab0338ad101171c9342ff6d263c833d5387 ? It seem to pass buildbot pretty well.

            looks ok. I'd add a comment before the define, to explain why it was needed. That mysqlbinlog is a mix of server and client code, it needs client includes but server internal values in enum_field_types.

            Another way to see it is that `Field::real_field_type()` and `Field::type()` should not use the same enum type. I hope we'll fix that eventually.

            serg Sergei Golubchik added a comment - looks ok. I'd add a comment before the define, to explain why it was needed. That mysqlbinlog is a mix of server and client code, it needs client includes but server internal values in enum_field_types. Another way to see it is that `Field::real_field_type()` and `Field::type()` should not use the same enum type. I hope we'll fix that eventually.
            svoj Sergey Vojtovich added a comment - Fixed in https://github.com/MariaDB/server/commit/4e1e5a32668bc717e0049961e789dd29883cc66c

            People

              svoj Sergey Vojtovich
              marko Marko Mäkelä
              Votes:
              0 Vote for this issue
              Watchers:
              5 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.