Support Percona MySQL 8 by pparshin · Pull Request #13 · tarantool/libslave

pparshin · 2020-09-19T11:14:10Z

This MR updates mysql submodule, fixes some compilation/linking errors

Totktonada · 2020-09-21T01:26:59Z

To be honest, it is hard to review due to lots of code style changes. I would extract them into a separate commit(s).

pparshin · 2020-09-21T08:57:30Z

@Totktonada, reverted code style changes

ligurio · 2020-09-22T16:21:00Z

@ParshinPavel, we are fixed CI in a master. Could you rebase your patch?

pparshin · 2020-09-23T14:18:31Z

@ligurio, done

ligurio · 2020-09-23T15:06:45Z

@ParshinPavel I still see changes for Travis in your patch - https://patch-diff.githubusercontent.com/raw/tarantool/libslave/pull/13.patch And GH also warns about conflicts in this PR (see at the bottom of PR page).

pparshin · 2020-09-23T15:28:24Z

@ligurio , i have no idea why github generates patch including all commits. If you open tab "Files changed", you see the real diff.
And i don't see any unresolved confilcts.

If you wish, I can just squash all commits and force push

ligurio · 2020-09-23T15:58:35Z

@ParshinPavel perhaps that happens because you did a merge and not a rebase: see commits in your own branch - https://github.com/ParshinPavel/libslave/commits/support-mysql8 You should drop your travis-related commits from a branch and then push updated commits with --force. See documentation: https://git-scm.com/docs/git-rebase

pparshin · 2020-10-07T10:37:12Z

Hope it's better now

ligurio

It is definitely better, thank you!

I have some comments regarding your changes. Please check them inline and below:

You have mixed several changes in a single commit (formatting changes in .travis.yml and CMakeLists.txt, changes in a build infrastructure and changes related to support of Percona 8). It's a bad practice. I would ask you to split changes for a several commits and probably remove formatting changes when you don't need to change code (although it's up to you).
You have bumped a submodule version to a branch with 8th version. I'm not sure it's a good idea. I would ask you to stick module to a fixed tag or commit to have predictable result each time when we will rebuild library.
We can stick it to a commit of a latest tag in 8th series:

$ git tag | grep Percona-Server-8.0 | tail -1
Percona-Server-8.0.20-11
$ git rev-list -n 1 Percona-Server-8.0.20-11
5b5a5d2584af8e47a66248218be0288164a69cb7

ligurio · 2020-10-08T09:25:29Z

.travis.yml

 before_script:
    - mkdir build
    - cd build
-    - cmake -DCMAKE_BUILD_TYPE=Release .. 


This change is not related to PR with Percona 8 support. Please remove it.

Totktonada · 2020-10-08T11:21:17Z

This raises the question: can we support different mysql versions in future? How about mariadb? Can we support different versions in one build with appropriate runtime checks?

ligurio · 2020-10-08T12:16:00Z

@Totktonada

This raises the question: can we support different mysql versions in future? How about mariadb? Can we support different versions in one build with appropriate runtime checks?

MariaDB has incompatible replication protocol [1]. It was never supported in libslave. If you (or any user) wants MariaDB support
then create a ticket about it.

https://mariadb.com/kb/en/mariadb-vs-mysql-compatibility/#replication-compatibility

ligurio · 2020-10-08T10:22:42Z

field.cpp

    char buffer[ value_length ];

-    if (::decimal2string(&dec, (char*)&buffer, &value_length, zerofill ? precision : 0, scale, '0') != E_DEC_OK) {
+    if (::decimal2string(&dec, (char*)&buffer, &value_length, zerofill ? precision : 0, scale) != E_DEC_OK) {


There are at least two supported versions of MySQL: 5.7 and 8.0 [1] [2]. This change will add compatibility with 8.x and will broke compatibility with 5.7. Perhaps we need to introduce a compilation flag (for example MYSQL57, MYSQL80) and pass it to cmake before a build.

https://www.mysql.com/support/supportedplatforms/database.html

https://www.percona.com/services/policies/percona-software-support-lifecycle

ligurio · 2020-10-08T12:02:46Z

CMakeLists.txt

        -DWITH_CLIENT_PROTOCOL_TRACING=0
        -DWITH_DEFAULT_FEATURE_SET=0
-        -DWITH_SSL=bundled
+        -DWITH_SSL=system


What are the reasons to change it?

ligurio · 2020-10-08T12:04:26Z

CMakeLists.txt

+        mysql_client_build
+        WORKING_DIRECTORY ${MYSQL_BIN}
+        DEPENDS binlogevents_build
+        COMMAND ${CMAKE_COMMAND} --build . --target perconaserverclient


It seems it's a formatting change and it is not related to Percona 8 support. Please remove it.

ligurio · 2020-10-08T12:07:38Z

CMakeLists.txt

-# Most probably static mysql is built without fPIC, so, we can't build dynamic library with it
 ADD_LIBRARY (slave ${LINK_TYPE} ${SRC})
-TARGET_LINK_LIBRARIES (slave ${MYSQL_LIBS} -lpthread)
+TARGET_LINK_LIBRARIES (slave PUBLIC libmysql ssl crypto Threads::Threads m rt dl)


What are the reasons to remove MYSQL_LIBS here?

ligurio · 2020-10-08T12:26:54Z

slave_log_event.h

+  START_5_7_ENCRYPTION_EVENT = 159,
+
+  MARIA_EVENTS_BEGIN = 160,
+


libslave has a unit test (test/unit_test.cpp) that covers different types of events. It would be good to add new testcases for a new event types.

ligurio · 2020-10-08T12:31:11Z

slave_log_event.cpp

    case VIEW_CHANGE_EVENT:
    case XA_PREPARE_LOG_EVENT:
+    case PARTIAL_UPDATE_ROWS_EVENT:
+    case START_5_7_ENCRYPTION_EVENT:


Looks like event MYSQL_END_EVENT added to slave_log_event.h is missed here.

pparshin force-pushed the support-mysql8 branch 3 times, most recently from 5c10617 to 3b9d523 Compare October 7, 2020 09:08

Updated MySQL submodule, fixed compilation and linker issues

712f6f6

pparshin force-pushed the support-mysql8 branch from 3b9d523 to 712f6f6 Compare October 7, 2020 10:08

ligurio self-requested a review October 8, 2020 09:23

ligurio requested changes Oct 8, 2020

View reviewed changes

ligurio self-requested a review October 8, 2020 10:17

ligurio requested changes Oct 8, 2020

View reviewed changes

ligurio force-pushed the support-mysql8 branch from 07ebc98 to 712f6f6 Compare October 10, 2020 11:02

Conversation

pparshin commented Sep 19, 2020

Uh oh!

Totktonada commented Sep 21, 2020

Uh oh!

pparshin commented Sep 21, 2020

Uh oh!

ligurio commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pparshin commented Sep 23, 2020

Uh oh!

ligurio commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pparshin commented Sep 23, 2020

Uh oh!

ligurio commented Sep 23, 2020

Uh oh!

pparshin commented Oct 7, 2020

Uh oh!

ligurio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

Totktonada commented Oct 8, 2020

Uh oh!

ligurio commented Oct 8, 2020

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

ligurio Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ligurio commented Sep 22, 2020 •

edited

Loading

ligurio commented Sep 23, 2020 •

edited

Loading

ligurio left a comment •

edited

Loading