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

Spider table with charset utf32/utf16/ucs2 tries to set client charset to unsupported value

Details

    Description

      --source plugin/spider/spider/include/init_spider.inc
       
      SET spider_same_server_link= on;
      eval create server s foreign data wrapper mysql options (host "127.0.0.1", database "test", user "root", port $MASTER_MYPORT);
       
      CREATE TABLE t (a INT);
      CREATE TABLE t_spider (a INT) ENGINE=SPIDER COMMENT = "wrapper 'mysql', srv 's', table 't'" CHARSET utf32;
      SELECT * FROM t_spider;
       
      # Cleanup
       
      DROP TABLE t_spider, t;
      DROP SERVER s;
       
      --source plugin/spider/spider/include/deinit_spider.inc
      

      10.4 3e3cfa89

      mysqltest: At line 8: query 'SELECT * FROM t_spider' failed: 1231: Variable 'character_set_client' can't be set to the value of 'utf32'
      

      At this point Spider tries to run SET NAMES utf32 (hopelessly of course).

      Note that the test case sets an explicit character set for the Spider table for simplicity; in reality, it would rather be either a database charset, or a system-wide character-set-server. The result is the same anyway.

      Attachments

        Activity

          elenst Elena Stepanova added a comment - - edited

          A similar problem affects Federated.

          INSTALL SONAME 'ha_federatedx'; 
          eval create server s foreign data wrapper mysql options (host "127.0.0.1", database "test", user "root", port $MASTER_MYPORT);
           
          CREATE TABLE t (a INT);
          CREATE TABLE t_fed (a INT) ENGINE=FEDERATED CONNECTION="s/t" CHARSET utf32;
          SELECT * FROM t_fed;
           
          # Cleanup
          DROP TABLE t_fed, t;
          DROP SERVER s;
          UNINSTALL SONAME 'ha_federatedx';
          

          10.3 92be8d20

          SELECT * FROM t_fed;
          bug.t2                                   [ fail ]
                  Test ended at 2022-11-06 16:03:01
           
          CURRENT_TEST: bug.t2
          mysqltest: At line 6: query 'SELECT * FROM t_fed' failed: 1231: Received error: 1231 : Variable 'character_set_client' can't be set to the value of 'utf32'
          

          However, for Federated it can be worked around by using --skip-character-set-client-handshake, which doesn't work for Spider (probably because Spider runs SET NAMES explicitly, while Federated apparently sets it as a connection attribute).

          elenst Elena Stepanova added a comment - - edited A similar problem affects Federated. INSTALL SONAME 'ha_federatedx' ; eval create server s foreign data wrapper mysql options (host "127.0.0.1" , database "test" , user "root" , port $MASTER_MYPORT);   CREATE TABLE t (a INT ); CREATE TABLE t_fed (a INT ) ENGINE=FEDERATED CONNECTION = "s/t" CHARSET utf32; SELECT * FROM t_fed;   # Cleanup DROP TABLE t_fed, t; DROP SERVER s; UNINSTALL SONAME 'ha_federatedx' ; 10.3 92be8d20 SELECT * FROM t_fed; bug.t2 [ fail ] Test ended at 2022-11-06 16:03:01   CURRENT_TEST: bug.t2 mysqltest: At line 6: query 'SELECT * FROM t_fed' failed: 1231: Received error: 1231 : Variable 'character_set_client' can 't be set to the value of ' utf32' However, for Federated it can be worked around by using --skip-character-set-client-handshake , which doesn't work for Spider (probably because Spider runs SET NAMES explicitly, while Federated apparently sets it as a connection attribute).
          ycp Yuchen Pei added a comment - - edited

          elenstnayuta-yanagisawa

          After discussing with Nayuta, we are going to prohibit creating spider tables with charset utf32, utf16 etc, that is, we are going to make a change so that creation of a spider table with banned charset explicitly (with CHARSET) or implicitly (inheriting from database charset) will be rejected with an error.

          Some rationale:

          • The behaviour of creating a spider table without specifying a charset should be consistent with that of creating a "normal" table on a server, i.e. the charset should be set to default (utf8). This is the current behaviour so there is no need to change anything w.r.t. this.
          • The behaviour of creating a spider table specifying a banned client charset should be consistent with that of a "normal" client trying to set client charset to the banned one, i.e. emitting an error. This is what needs to be implemented to resolve this issue.

          The issue with federated engine with be splitted out into a new ticket. I will update this ticket when that is done.

          ycp Yuchen Pei added a comment - - edited elenst nayuta-yanagisawa After discussing with Nayuta, we are going to prohibit creating spider tables with charset utf32, utf16 etc, that is, we are going to make a change so that creation of a spider table with banned charset explicitly (with CHARSET) or implicitly (inheriting from database charset) will be rejected with an error. Some rationale: The behaviour of creating a spider table without specifying a charset should be consistent with that of creating a "normal" table on a server, i.e. the charset should be set to default (utf8). This is the current behaviour so there is no need to change anything w.r.t. this. The behaviour of creating a spider table specifying a banned client charset should be consistent with that of a "normal" client trying to set client charset to the banned one, i.e. emitting an error. This is what needs to be implemented to resolve this issue. The issue with federated engine with be splitted out into a new ticket. I will update this ticket when that is done.

          I think that prohibiting to create Spider table with ucs2/utf16/utf16le/utf32 is acceptable mainly by the following reasons:

          • Currently, Spider doesn't work with ucs2/utf16/utf16le/utf32. So, even if we prohibit creating Spider tables with the charsets, it doesn't affect users.
          • An implicit behavior of the server/storage engine tends to confuse users. Thus, I think that just prohibiting is not so bad until someone complains.
          nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - I think that prohibiting to create Spider table with ucs2/utf16/utf16le/utf32 is acceptable mainly by the following reasons: Currently, Spider doesn't work with ucs2/utf16/utf16le/utf32. So, even if we prohibit creating Spider tables with the charsets, it doesn't affect users. An implicit behavior of the server/storage engine tends to confuse users. Thus, I think that just prohibiting is not so bad until someone complains.

          > Currently, Spider doesn't work with ucs2/utf16/utf16le/utf32. So, even if we prohibit creating Spider tables with the charsets, it doesn't affect users.

          Oh, I overlooked that the affect version is 10.4+. I'm checking why 10.3 is not affected.

          nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - > Currently, Spider doesn't work with ucs2/utf16/utf16le/utf32. So, even if we prohibit creating Spider tables with the charsets, it doesn't affect users. Oh, I overlooked that the affect version is 10.4+. I'm checking why 10.3 is not affected.

          I've confirmed that the bug is reproducible on 10.3 too.

          nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - I've confirmed that the bug is reproducible on 10.3 too.

          Oh, I overlooked that the affect version is 10.4+

          Sorry about that, I didn't know you are still fixing non-blockers in 10.3 Spider.

          On a general note, 10.3 goes EOL fairly soon, we start being more selective about what to fix in 10.3 and what not. With only two more planned releases, Q1 and Q2 2023, there will be very little time to correct regressions. Given that it is a cosmetic problem and a cosmetic fix, you can apply your own discretion to whether to fix it in 10.3 or not. And in 10.4 even.

          elenst Elena Stepanova added a comment - Oh, I overlooked that the affect version is 10.4+ Sorry about that, I didn't know you are still fixing non-blockers in 10.3 Spider. On a general note, 10.3 goes EOL fairly soon, we start being more selective about what to fix in 10.3 and what not. With only two more planned releases, Q1 and Q2 2023, there will be very little time to correct regressions. Given that it is a cosmetic problem and a cosmetic fix, you can apply your own discretion to whether to fix it in 10.3 or not. And in 10.4 even.
          ycp Yuchen Pei added a comment -

          Splitted issue with federated engine to https://jira.mariadb.org/browse/MDEV-30160.

          ycp Yuchen Pei added a comment - Splitted issue with federated engine to https://jira.mariadb.org/browse/MDEV-30160 .
          ycp Yuchen Pei added a comment - - edited

          https://github.com/MariaDB/server/commit/f67a21fed26
          https://github.com/MariaDB/server/commit/8b69032748f

          A couple of things worth noting / I am not sure about:

          • The placement of the error (early on in ha_spider::create)
          • The construction of the string charset_option is a bit ugly
          • As discussed on slack, the error message may be confusing if the bad charset comes from the database rather than explicitly specified by the user.
          ycp Yuchen Pei added a comment - - edited http s://github.com/MariaDB/server/commit/f67a21fed26 https://github.com/MariaDB/server/commit/8b69032748f A couple of things worth noting / I am not sure about: The placement of the error (early on in ha_spider::create) The construction of the string charset_option is a bit ugly As discussed on slack, the error message may be confusing if the bad charset comes from the database rather than explicitly specified by the user.

          Commented on GitHub.

          nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - Commented on GitHub.
          ycp Yuchen Pei added a comment -

          Thanks for the comments - I've addressed all of them. Please can you review the updated commit at https://github.com/MariaDB/server/commit/9f7a96864f4d346acc08b49ad85434622d090692 thank you.

          ycp Yuchen Pei added a comment - Thanks for the comments - I've addressed all of them. Please can you review the updated commit at https://github.com/MariaDB/server/commit/9f7a96864f4d346acc08b49ad85434622d090692 thank you.

          I agree with the patch with some comments.

          nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - I agree with the patch with some comments.
          ycp Yuchen Pei added a comment - - edited

          nayuta-yanagisawa Thanks for the comments. I have addressed them and updated my commit at https://github.com/MariaDB/server/commit/ccb738d868b https://github.com/MariaDB/server/commit/3f63aa18a7f, PTAL thanks.

          ycp Yuchen Pei added a comment - - edited nayuta-yanagisawa Thanks for the comments. I have addressed them and updated my commit at htt ps://github.com/MariaDB/server/commit/ccb738d868b https://github.com/MariaDB/server/commit/3f63aa18a7f , PTAL thanks.

          The last patch is OK to push.

          nayuta-yanagisawa Nayuta Yanagisawa (Inactive) added a comment - The last patch is OK to push.
          ycp Yuchen Pei added a comment -

          Looks like a bit more work is needed so that the commit builds for 10.6+.

          ycp Yuchen Pei added a comment - Looks like a bit more work is needed so that the commit builds for 10.6+.
          ycp Yuchen Pei added a comment -

          Pushed a commit[1] for 10.3-5 and a commit[2] for 10.6+ due to [3].

          [1] 3f63aa18a7f81540049c8414308c6c04fdc4a5ad
          [2] 5eb545e5c22318c79991473bb95f334eda6010ca
          [3] a206658b985fe5e18fb5692fdb3698dad5aca70a

          ycp Yuchen Pei added a comment - Pushed a commit [1] for 10.3-5 and a commit [2] for 10.6+ due to [3] . [1] 3f63aa18a7f81540049c8414308c6c04fdc4a5ad [2] 5eb545e5c22318c79991473bb95f334eda6010ca [3] a206658b985fe5e18fb5692fdb3698dad5aca70a

          People

            ycp Yuchen Pei
            elenst Elena Stepanova
            Votes:
            0 Vote for this issue
            Watchers:
            4 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.