Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MDEV-30824] Fix binlog to use 'String' for setting 'character_set_client' #2557

Closed
wants to merge 1 commit into from

Conversation

PhysicsTing
Copy link
Contributor

@PhysicsTing PhysicsTing commented Mar 17, 2023

  • The Jira issue number for this PR is: MDEV-30824

Description

Commit a923d6f disabled numeric setting of character_set_* variables with non-default values:

MariaDB [(none)]> set character_set_client=224;
ERROR 1115 (42000): Unknown character set: '224'

However the corresponding binlog functionality still write numeric values for log event, and this will break binlog replay if the value is not default. Now make the server use 'String' type for 'character_set_client' when generating binlog event.

Before:

/*!\C utf8mb4 *//*!*/;
SET @@session.character_set_client=224,@@session.collation_connection=224,@@session.collation_server=33/*!*/;

After:

/*!\C utf8mb4 *//*!*/;
SET @@session.character_set_client=utf8mb4,@@session.collation_connection=33,@@session.collation_server=8/*!*/;

Note: prior to the previous commit, setting with '224' or '45' or 'utf8mb4' have the same effect, as they all set the parameter to 'utf8mb4'.

How can this PR be tested?

MTR binglog related tests are rerecorded. The difference are as expected.
The rest of test cases in main suite all passed.

Basing the PR against the correct MariaDB version

  • This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced: 10.9

Backward compatibility

The setting of character_set_client originally supports variable type 'String', so this will not compromise any backward compatibility.

PR quality check

Copyright

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.

@CLAassistant
Copy link

CLAassistant commented Mar 17, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@PhysicsTing
Copy link
Contributor Author

PhysicsTing commented Mar 17, 2023

Some test failed due to some MTR tests not updated. I'm addressing them.

…ient'

Commit a923d6f disabled numeric setting
of character_set_* variables with non-default values:

  MariaDB [(none)]> set character_set_client=224;
  ERROR 1115 (42000): Unknown character set: '224'

However the corresponding binlog functionality still write numeric
values for log event, and this will break binlog replay if the value is
not default. Now make the server use 'String' type for
'character_set_client' when generating binlog events

Before:

  /*!\C utf8mb4 *//*!*/;
  SET @@session.character_set_client=224,@@session.collation_connection=224,@@session.collation_server=33/*!*/;

After:

  /*!\C utf8mb4 *//*!*/;
  SET @@session.character_set_client=utf8mb4,@@session.collation_connection=33,@@session.collation_server=8/*!*/;

Note: prior to the previous commit, setting with '224' or '45' or
'utf8mb4' have the same effect, as they all set the parameter to
'utf8mb4'.

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
@an3l
Copy link
Contributor

an3l commented Mar 21, 2023

The changes from this PR are pushed with commit dccbb5a to 10.5 @abarkov ok to close PR?

@an3l an3l added this to the 10.9 milestone Mar 21, 2023
@PhysicsTing
Copy link
Contributor Author

The changes from this PR are pushed with commit dccbb5a to 10.5 @abarkov ok to close PR?

Hi @an3l Do you mean 10.9? This PR is fixing a regression a923d6f that was first merged to 10.9.2. Thus 10.9 should the target branch, and then be merged up to 10.10 and 11.0.

@abarkov
Copy link
Contributor

abarkov commented Mar 21, 2023

Pushed to 10.5.

@abarkov abarkov closed this Mar 21, 2023
@abarkov
Copy link
Contributor

abarkov commented Mar 21, 2023

I changed the target version from 10.9 to 10.5, so a "mysqlbinlog" output from versions between 10.5 and 10.8 can also be loaded by a 10.9+ server.

@PhysicsTing
Copy link
Contributor Author

That makes sense. Thanks for the explanation.

@ottok
Copy link
Contributor

ottok commented Mar 21, 2023

@abarkov @LinuxJedi @ericherman Can you please make sure that PR reviewers don't close Pull Requests when they could be merged? Now Ting's Github profile will say that his contribution was rejected, and also anybody looking at this PR will not see any link to the commit etc.

In a situation like this (and basically any review situation) the reviewer should give feedback "Thanks looks good, please rebase on 10.5", wait for one day for this to happen, and then merge the PR.

@ottok
Copy link
Contributor

ottok commented Mar 23, 2023

For the record, this was committed to 10.5 in dccbb5a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants