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

[PATCH] Alter table xxx CHARACTER SET utf8, CONVERT TO CHARACTER SET latin1 should fail

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 10.0.15
    • Fix Version/s: 10.0.18
    • Component/s: Parser
    • Labels:
      None

      Description

      Both these terms set the default_table_charset attribute internally so whenever used they should be the same value.

      Alter table xxx CONVERT TO CHARACTER SET latin1, CHARACTER SET utf8 has protection mechanism that result in an error.

      However Alter table xxx CHARACTER SET utf8, CONVERT TO CHARACTER SET latin1 will ignore the utf8 and just use the latin1 for the default charset and conversion.

      How to repeat:
      Alter table xxx CHARACTER SET utf8, CONVERT TO CHARACTER SET latin1

      on any table.

      The resting table will have DEFAULT CHARSET=latin1

      Attached patch causes and error consistent with Alter table xxx CONVERT TO CHARACTER SET latin1, CHARACTER SET utf8

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              elenst Elena Stepanova added a comment -

              Thanks.

              I'm not quite certain that it's not the other way round, that is that
              Alter table xxx CONVERT TO CHARACTER SET latin1, CHARACTER SET utf8 is supposed to return an error. Why exactly are the definitions conflicting? One can apply every clause separately, in any order. I also don't see in the manual anything that explains this limitation (maybe I just haven't found it though).

              Anyway, assigning to Alexander Barkov, he will know for sure.

              Show
              elenst Elena Stepanova added a comment - Thanks. I'm not quite certain that it's not the other way round, that is that Alter table xxx CONVERT TO CHARACTER SET latin1, CHARACTER SET utf8 is supposed to return an error. Why exactly are the definitions conflicting? One can apply every clause separately, in any order. I also don't see in the manual anything that explains this limitation (maybe I just haven't found it though). Anyway, assigning to Alexander Barkov , he will know for sure.
              Hide
              danblack Daniel Black added a comment -

              > I'm not quite certain that it's not the other way round, that is that
              Alter table xxx CONVERT TO CHARACTER SET latin1, CHARACTER SET utf8 is supposed to return an error.

              it does.

              Currently Alter table xxx CHARACTER SET utf8, CONVERT TO CHARACTER SET latin1
              converts to latin1 and sets the default to latin1. the utf8 is ignored.

              The order matters because the parser sets a state and this state is only checked in the [DEFAULT] CHARACTER SET part.

              > Why exactly are the definitions conflicting?

              Internally both CONVERT TO and CHARACTER SET set the default character set and use the same variable underneath to represent this. If this wasn't the case we could make CHARACTER SET set the default and CONVERT do the conversion to anything however at the moment its just an internal limitation.

              > One can apply every clause separately, in any order.

              Yes.

              > I also don't see in the manual anything that explains this limitation.

              limitation is that is if CONVERT TO and CHARACTER SET are both set they have to be the same character set.

              Show
              danblack Daniel Black added a comment - > I'm not quite certain that it's not the other way round, that is that Alter table xxx CONVERT TO CHARACTER SET latin1, CHARACTER SET utf8 is supposed to return an error. it does. Currently Alter table xxx CHARACTER SET utf8, CONVERT TO CHARACTER SET latin1 converts to latin1 and sets the default to latin1. the utf8 is ignored. The order matters because the parser sets a state and this state is only checked in the [DEFAULT] CHARACTER SET part. > Why exactly are the definitions conflicting? Internally both CONVERT TO and CHARACTER SET set the default character set and use the same variable underneath to represent this. If this wasn't the case we could make CHARACTER SET set the default and CONVERT do the conversion to anything however at the moment its just an internal limitation. > One can apply every clause separately, in any order. Yes. > I also don't see in the manual anything that explains this limitation. limitation is that is if CONVERT TO and CHARACTER SET are both set they have to be the same character set.
              Show
              danblack Daniel Black added a comment - https://github.com/MariaDB/server/pull/22
              Hide
              bar Alexander Barkov added a comment -

              Hi Daniel! Thanks for the patch! It looks correct.

              I extended it slightly, basically 3 very small things:

              1. Collected similar code which used for both "CHARSET SET" and "CONVERT TO"
              into a method HA_CREATE_INFO::check_conflicting_charset_declarations(),
              to avoid code duplication.

              2. Added handling of conflicting declarations in cases:
              CHARACTER SET DEFAULT, CHARACTER SET utf8
              CHARACTER SET utf8, CHARACTER SET DEFAULT
              I realized that before this change the second specification was just silently ignored,
              which was not good.

              3. Then moved the rest of the code as methods to HA_CREATE_INFO.
              It's nice to take advantage of this occasion to offload sql_yacc.yy

              Please have a look.

              Many thanks for your contribution!
              This would certainly stay unfixed for many months otherwise.

              Show
              bar Alexander Barkov added a comment - Hi Daniel! Thanks for the patch! It looks correct. I extended it slightly, basically 3 very small things: 1. Collected similar code which used for both "CHARSET SET" and "CONVERT TO" into a method HA_CREATE_INFO::check_conflicting_charset_declarations(), to avoid code duplication. 2. Added handling of conflicting declarations in cases: CHARACTER SET DEFAULT, CHARACTER SET utf8 CHARACTER SET utf8, CHARACTER SET DEFAULT I realized that before this change the second specification was just silently ignored, which was not good. 3. Then moved the rest of the code as methods to HA_CREATE_INFO. It's nice to take advantage of this occasion to offload sql_yacc.yy Please have a look. Many thanks for your contribution! This would certainly stay unfixed for many months otherwise.
              Hide
              danblack Daniel Black added a comment -

              nice. can't see any faults and thanks for adding the test cases and extending the scope.

              Show
              danblack Daniel Black added a comment - nice. can't see any faults and thanks for adding the test cases and extending the scope.
              Hide
              bar Alexander Barkov added a comment -

              Pushed into the 10.0 git branch

              Show
              bar Alexander Barkov added a comment - Pushed into the 10.0 git branch

                People

                • Assignee:
                  bar Alexander Barkov
                  Reporter:
                  danblack Daniel Black
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: