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

Fix for builing DBD-mysql together with MariaDB Connector/C. #133

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@9EOR9

9EOR9 commented May 28, 2017

Use mysql_option function instead of accessing internal members of MYSQL structure.

Fix for builing DBD-mysql together with MariaDB Connector/C.
Use mysql_option function instead of accessing internal members of MYSQL structure.
@pali

This comment has been minimized.

Show comment
Hide comment
@pali

pali May 28, 2017

Contributor

Some tests failed:

dbdimp.c:2272:29: error: ‘MYSQL_OPT_RECONNECT’ undeclared (first use in this function)

Also note that MariaDB Connector/C does not work with DBD::mysql, because MariaDB Connector/C has broken zerofill support, see: e1ea552 #98 (comment)

Contributor

pali commented May 28, 2017

Some tests failed:

dbdimp.c:2272:29: error: ‘MYSQL_OPT_RECONNECT’ undeclared (first use in this function)

Also note that MariaDB Connector/C does not work with DBD::mysql, because MariaDB Connector/C has broken zerofill support, see: e1ea552 #98 (comment)

@pali

This comment has been minimized.

Show comment
Hide comment
@pali

pali May 28, 2017

Contributor

This could help you:

http://web.archive.org/web/20110902122038/http://dev.mysql.com/doc/refman/5.0/en/auto-reconnect.html

The MYSQL_OPT_RECONNECT option is available as of MySQL 5.0.13.

Contributor

pali commented May 28, 2017

This could help you:

http://web.archive.org/web/20110902122038/http://dev.mysql.com/doc/refman/5.0/en/auto-reconnect.html

The MYSQL_OPT_RECONNECT option is available as of MySQL 5.0.13.

- Fixed preprocessor directives for MariaDB Connector/C
- Fixed length calculation if mysql_stmt_attr_set wasn't called with parameter STMT_ATTR_UPDATE_MAX_LENGTH: In this case buffer_length will be calculated from actual length of the field.
@@ -110,22 +124,22 @@
*/
/* Use mysql_options with MYSQL_OPT_SSL_VERIFY_SERVER_CERT */
#if ((MYSQL_VERSION_ID >= 50023 && MYSQL_VERSION_ID < 50100) || MYSQL_VERSION_ID >= 50111) && (MYSQL_VERSION_ID < 80000 || defined(MARIADB_BASE_VERSION))
#ifdef CLIENT_SSL_VERIFY_SERVER_CERT

This comment has been minimized.

@pali

pali May 31, 2017

Contributor

This is wrong. MySQL 8 has defined CLIENT_SSL_VERIFY_SERVER_CERT macro but does not support MYSQL_OPT_SSL_VERIFY_SERVER_CERT.

@pali

pali May 31, 2017

Contributor

This is wrong. MySQL 8 has defined CLIENT_SSL_VERIFY_SERVER_CERT macro but does not support MYSQL_OPT_SSL_VERIFY_SERVER_CERT.

This comment has been minimized.

@9EOR9

9EOR9 May 31, 2017

The verification is still in place:
https://github.com/mysql/mysql-server/blob/8.0/sql-common/client.cc#L3715

Even if MYSQL_OPT_SSL_VERIFY_SERVER_CERT was renoved, you can still pass CLIENT_SSL_VERIFY_SERVER_CERT flag to mysql_real_connect (last parameter).

@9EOR9

9EOR9 May 31, 2017

The verification is still in place:
https://github.com/mysql/mysql-server/blob/8.0/sql-common/client.cc#L3715

Even if MYSQL_OPT_SSL_VERIFY_SERVER_CERT was renoved, you can still pass CLIENT_SSL_VERIFY_SERVER_CERT flag to mysql_real_connect (last parameter).

This comment has been minimized.

@9EOR9

9EOR9 May 31, 2017

The verification is still in place:
https://github.com/mysql/mysql-server/blob/8.0/sql-common/client.cc#L3715

Even if MYSQL_OPT_SSL_VERIFY_SERVER_CERT was renoved, you can still pass CLIENT_SSL_VERIFY_SERVER_CERTflag to mysql_real_connect (last parameter).

@9EOR9

9EOR9 May 31, 2017

The verification is still in place:
https://github.com/mysql/mysql-server/blob/8.0/sql-common/client.cc#L3715

Even if MYSQL_OPT_SSL_VERIFY_SERVER_CERT was renoved, you can still pass CLIENT_SSL_VERIFY_SERVER_CERTflag to mysql_real_connect (last parameter).

This comment has been minimized.

@pali

pali May 31, 2017

Contributor

But this check is for defining HAVE_SSL_VERIFY. And it means DBD::mysql can use mysql_options() with MYSQL_OPT_SSL_VERIFY_SERVER_CERT. It has nothing with CLIENT_SSL_VERIFY_SERVER_CERT flag or mysql_real_connect() function.

Some of those mysql_* SSL related functions are broken in particular MySQL versions and therefore we need to correctly ifdef them and use safe variant.

See #114 and #110

Your change break SSL verification for some MySQL versions.

@pali

pali May 31, 2017

Contributor

But this check is for defining HAVE_SSL_VERIFY. And it means DBD::mysql can use mysql_options() with MYSQL_OPT_SSL_VERIFY_SERVER_CERT. It has nothing with CLIENT_SSL_VERIFY_SERVER_CERT flag or mysql_real_connect() function.

Some of those mysql_* SSL related functions are broken in particular MySQL versions and therefore we need to correctly ifdef them and use safe variant.

See #114 and #110

Your change break SSL verification for some MySQL versions.

This comment has been minimized.

@pali

pali Jun 11, 2017

Contributor

So... I guess correct should be this check which do not break SSL verification:

#if ((MYSQL_VERSION_ID >= 50023 && MYSQL_VERSION_ID < 50100) || MYSQL_VERSION_ID >= 50111) && (MYSQL_VERSION_ID < 80000 || defined(MARIADB_CLIENT))
@pali

pali Jun 11, 2017

Contributor

So... I guess correct should be this check which do not break SSL verification:

#if ((MYSQL_VERSION_ID >= 50023 && MYSQL_VERSION_ID < 50100) || MYSQL_VERSION_ID >= 50111) && (MYSQL_VERSION_ID < 80000 || defined(MARIADB_CLIENT))

This comment has been minimized.

@9EOR9

9EOR9 Jun 12, 2017

Yes, this looks better, I will fix and push later today. We need to think about to add ssl_mode also in Connector/C, so API's will not differ too much and iit would be easier to handle TLS/SSL options in the future (e.g. Connector/C additionally supports finger print check of server certificate).

@9EOR9

9EOR9 Jun 12, 2017

Yes, this looks better, I will fix and push later today. We need to think about to add ssl_mode also in Connector/C, so API's will not differ too much and iit would be easier to handle TLS/SSL options in the future (e.g. Connector/C additionally supports finger print check of server certificate).

@@ -2268,7 +2269,7 @@ MYSQL *mysql_dr_connect(
we turn off Mysql's auto reconnect and handle re-connecting ourselves
so that we can keep track of when this happens.
*/
result->reconnect=0;

This comment has been minimized.

@pali

pali Jun 11, 2017

Contributor
#if MYSQL_VERSION_ID >= 50013
mysql_options(result, MYSQL_OPT_RECONNECT, &reconnect);
#else
result->reconnect=0;
#endif
@pali

pali Jun 11, 2017

Contributor
#if MYSQL_VERSION_ID >= 50013
mysql_options(result, MYSQL_OPT_RECONNECT, &reconnect);
#else
result->reconnect=0;
#endif

NexediGitlab pushed a commit to SlapOS/slapos that referenced this pull request Jun 12, 2017

WIP: component/perl-DBD-mariadb: version up 4.0.42 and add a patch to…
… build with MariaDB 10.2

This fixes compilation issues but this does not seem enough:
  perl5-dbi/DBD-mysql#133

NexediGitlab pushed a commit to SlapOS/slapos that referenced this pull request Jun 12, 2017

perl-DBD-mysql: fix compilation issue with MariaDB 10.2.6
Because this does not seem to be enough:
perl5-dbi/DBD-mysql#133

We should keep using MariaDB 10.1 for ERP5 as long as perl-DBD-mysql
is not fixed properly.
@AdamWill

This comment has been minimized.

Show comment
Hide comment
@AdamWill

AdamWill Jul 14, 2017

Note downstream (Fedora) bug: https://bugzilla.redhat.com/show_bug.cgi?id=1470196 , where one of our mariadb maintainers suggests adding +#include <mysql_version.h> /* For MYSQL_VERSION_ID */ for the versioning stuff...

AdamWill commented Jul 14, 2017

Note downstream (Fedora) bug: https://bugzilla.redhat.com/show_bug.cgi?id=1470196 , where one of our mariadb maintainers suggests adding +#include <mysql_version.h> /* For MYSQL_VERSION_ID */ for the versioning stuff...

@pali

This comment has been minimized.

Show comment
Hide comment
@pali

pali Jul 14, 2017

Contributor

@AdamWill that would not work, because MYSQL_VERSION_ID was added only recently in commit MariaDB Connector/C commit: MariaDB/mariadb-connector-c@422d0f7

Contributor

pali commented Jul 14, 2017

@AdamWill that would not work, because MYSQL_VERSION_ID was added only recently in commit MariaDB Connector/C commit: MariaDB/mariadb-connector-c@422d0f7

@AdamWill

This comment has been minimized.

Show comment
Hide comment
@AdamWill

AdamWill Jul 19, 2017

@pali but no stable version was ever actually released without it, AFAICT. On the 2.x branch, nothing was renamed to MARIADB, so it still had MYSQL_VERSION_ID, see e.g. the 2.3.2 tag:

https://github.com/MariaDB/mariadb-connector-c/blob/v2.3.2/include/mysql_version.h.in

and in 3.0.2, the commit you mention was already included:

https://github.com/MariaDB/mariadb-connector-c/blob/v3.0.2/include/mariadb_version.h.in

So the only tagged release that does not have MYSQL_VERSION_ID is 3.0.1-beta, which was not a stable release. Does this still concern you?

edit: Hmm, but in 3.x, the file is still called something different (mariadb_version.h vs. mysql_version.h)...

AdamWill commented Jul 19, 2017

@pali but no stable version was ever actually released without it, AFAICT. On the 2.x branch, nothing was renamed to MARIADB, so it still had MYSQL_VERSION_ID, see e.g. the 2.3.2 tag:

https://github.com/MariaDB/mariadb-connector-c/blob/v2.3.2/include/mysql_version.h.in

and in 3.0.2, the commit you mention was already included:

https://github.com/MariaDB/mariadb-connector-c/blob/v3.0.2/include/mariadb_version.h.in

So the only tagged release that does not have MYSQL_VERSION_ID is 3.0.1-beta, which was not a stable release. Does this still concern you?

edit: Hmm, but in 3.x, the file is still called something different (mariadb_version.h vs. mysql_version.h)...

@pali

This comment has been minimized.

Show comment
Hide comment
@pali

pali Jul 19, 2017

Contributor

Ou right, so problematic is only the 3.0.1-beta and all older versions are working... Then we can use it.

Contributor

pali commented Jul 19, 2017

Ou right, so problematic is only the 3.0.1-beta and all older versions are working... Then we can use it.

@AdamWill

This comment has been minimized.

Show comment
Hide comment
@AdamWill

AdamWill Jul 19, 2017

Well, see my edit (the file still seems to have a different name on the 3.x branch). Also note the 'downstream bug' I referenced isn't actually for quite the same thing, it's about building against MariaDB 10.2 vs. 10.1 (not building against mariadb-connector-c).

Actually the more I look at this 3.x branch of mariadb-connector-c, the weirder it gets...AIUI, mariadb-connector-c / mysql-connector-c are supposed to be just the client library from the 'main' distribution (mariadb / mysql) isolated so you don't have to install the whole thing if you're just building a client. But mariadb-connector-c 3.x doesn't seem to match the client library from mariadb 10.2 at all, at least in terms of these header files. They haven't all been renamed in MariaDB itself - there's no mariadb_ or ma_ headers in https://github.com/MariaDB/server/tree/10.2/include . So that all seems a bit odd...

AdamWill commented Jul 19, 2017

Well, see my edit (the file still seems to have a different name on the 3.x branch). Also note the 'downstream bug' I referenced isn't actually for quite the same thing, it's about building against MariaDB 10.2 vs. 10.1 (not building against mariadb-connector-c).

Actually the more I look at this 3.x branch of mariadb-connector-c, the weirder it gets...AIUI, mariadb-connector-c / mysql-connector-c are supposed to be just the client library from the 'main' distribution (mariadb / mysql) isolated so you don't have to install the whole thing if you're just building a client. But mariadb-connector-c 3.x doesn't seem to match the client library from mariadb 10.2 at all, at least in terms of these header files. They haven't all been renamed in MariaDB itself - there's no mariadb_ or ma_ headers in https://github.com/MariaDB/server/tree/10.2/include . So that all seems a bit odd...

@9EOR9

This comment has been minimized.

Show comment
Hide comment
@9EOR9

9EOR9 Jul 20, 2017

Both MariaDB 10.2.7 (released last week) and Connector/C 3.0.2 (will be released today) have MYSQL_VERSION_ID

9EOR9 commented Jul 20, 2017

Both MariaDB 10.2.7 (released last week) and Connector/C 3.0.2 (will be released today) have MYSQL_VERSION_ID

@9EOR9

This comment has been minimized.

Show comment
Hide comment
@9EOR9

9EOR9 Jul 20, 2017

Adam,
Connector/C 3.0 is identical to the client library in 10.2. Please note that Connector/C in server tree is a git submodule in directory libmariadb (Connector/C branch 10.2-server).

9EOR9 commented Jul 20, 2017

Adam,
Connector/C 3.0 is identical to the client library in 10.2. Please note that Connector/C in server tree is a git submodule in directory libmariadb (Connector/C branch 10.2-server).

@AdamWill

This comment has been minimized.

Show comment
Hide comment
@AdamWill

AdamWill Jul 20, 2017

@9EOR9 But what about the header file names? They seem to be different between the two...

AdamWill commented Jul 20, 2017

@9EOR9 But what about the header file names? They seem to be different between the two...

@9EOR9

This comment has been minimized.

Show comment
Hide comment
@9EOR9

9EOR9 Jul 20, 2017

What header file names do you mean? The only file which should be included is mysql.h - it includes all necessary files for client applications.

9EOR9 commented Jul 20, 2017

What header file names do you mean? The only file which should be included is mysql.h - it includes all necessary files for client applications.

@AdamWill

This comment has been minimized.

Show comment
Hide comment
@AdamWill

AdamWill Jul 21, 2017

Hmm, I see. Well, the patch our maintainer suggested included an explicit #include mysql_version.h - so I guess that should be left out...

AdamWill commented Jul 21, 2017

Hmm, I see. Well, the patch our maintainer suggested included an explicit #include mysql_version.h - so I guess that should be left out...

@AdamWill

This comment has been minimized.

Show comment
Hide comment
@AdamWill

AdamWill Jul 21, 2017

@9EOR9 So poking into this further, I found what seems to be a bug in MariaDB itself: when you run an install of MariaDB 10.2, you ultimately wind up with the mysql.h from the libmariadb/ subdirectory (which, as you pointed out, is mariadb-connector-c as a subproject) installed. Not the mysql.h from MariaDB itself. So you get the mysql.h for the client library, which ultimately doesn't provide as much stuff as the 'full fat' mysql.h for the whole of MariaDB. This ultimately seems to be behind several of the build failures we observed with things that want to use stuff that's defined in MariaDB's mysql_version.h, which MariaDB's mysql.h includes, but not in the client library's mariadb_version.h, which the client library's mysql.h includes. Reported as https://jira.mariadb.org/browse/MDEV-13370 .

AdamWill commented Jul 21, 2017

@9EOR9 So poking into this further, I found what seems to be a bug in MariaDB itself: when you run an install of MariaDB 10.2, you ultimately wind up with the mysql.h from the libmariadb/ subdirectory (which, as you pointed out, is mariadb-connector-c as a subproject) installed. Not the mysql.h from MariaDB itself. So you get the mysql.h for the client library, which ultimately doesn't provide as much stuff as the 'full fat' mysql.h for the whole of MariaDB. This ultimately seems to be behind several of the build failures we observed with things that want to use stuff that's defined in MariaDB's mysql_version.h, which MariaDB's mysql.h includes, but not in the client library's mariadb_version.h, which the client library's mysql.h includes. Reported as https://jira.mariadb.org/browse/MDEV-13370 .

@GieltjE

This comment has been minimized.

Show comment
Hide comment
@GieltjE

GieltjE Sep 28, 2017

The bug has been fixed by MariaDB in 10.2.9, we really should be working on a fix as half the world is migrating to MariaDB and a lot of packages depend on this package (spamassassin, opendkim, opendmarc).

For now I use an old mysql.so which seems to work for now.

GieltjE commented Sep 28, 2017

The bug has been fixed by MariaDB in 10.2.9, we really should be working on a fix as half the world is migrating to MariaDB and a lot of packages depend on this package (spamassassin, opendkim, opendmarc).

For now I use an old mysql.so which seems to work for now.

@noplanman

This comment has been minimized.

Show comment
Hide comment
@noplanman

noplanman Nov 4, 2017

Any update on this? Has just smacked me in the face 😕

noplanman commented Nov 4, 2017

Any update on this? Has just smacked me in the face 😕

@mannih

This comment has been minimized.

Show comment
Hide comment
@mannih

mannih Jan 17, 2018

FWIW: I just ran into this problem after upgrading my fedora install from 26 to 27. I was able to install DBD::mysql after applying this patch set: https://src.fedoraproject.org/rpms/perl-DBD-MySQL/blob/c9a8c6732a3cad7016fe0cbf217a9f0846af6bb9/f/DBD-mysql-4.043-Fix-build-failures-for-MariaDB-10.2.patch
Of course, I have no idea how well this works on other systems with other libraries.

mannih commented Jan 17, 2018

FWIW: I just ran into this problem after upgrading my fedora install from 26 to 27. I was able to install DBD::mysql after applying this patch set: https://src.fedoraproject.org/rpms/perl-DBD-MySQL/blob/c9a8c6732a3cad7016fe0cbf217a9f0846af6bb9/f/DBD-mysql-4.043-Fix-build-failures-for-MariaDB-10.2.patch
Of course, I have no idea how well this works on other systems with other libraries.

@GieltjE

This comment has been minimized.

Show comment
Hide comment
@GieltjE

GieltjE Jan 17, 2018

@mannih Just tested it on debian with MariaDB 10.2.12 with spamassassin, opendkim and opendmarc, everything working as it should (also applied the other pathes availble in https://src.fedoraproject.org/rpms/perl-DBD-MySQL/tree/master).

GieltjE commented Jan 17, 2018

@mannih Just tested it on debian with MariaDB 10.2.12 with spamassassin, opendkim and opendmarc, everything working as it should (also applied the other pathes availble in https://src.fedoraproject.org/rpms/perl-DBD-MySQL/tree/master).

pali added a commit to pali/DBD-MariaDB that referenced this pull request Jan 23, 2018

Use API function for reconnect
For configuring reconnect functionality use standard API function
MYSQL_OPT_RECONNECT instead of direct modification of internal structures
which does not work for MariaDB. Fixes compilation with MariaDB 10.2.6+.

Fixes: https://rt.cpan.org/Ticket/Display.html?id=122065
Fixes: https://rt.cpan.org/Ticket/Display.html?id=122429
Fixes: https://rt.cpan.org/Ticket/Display.html?id=122431
Fixes: perl5-dbi/DBD-mysql#139
Fixes: perl5-dbi/DBD-mysql#169
Fixes: perl5-dbi/DBD-mysql#218
Fixes: perl5-dbi/DBD-mysql#219
Fixes: perl5-dbi/DBD-mysql#133
Fixes: perl5-dbi/DBD-mysql#224

pali added a commit to pali/DBD-MariaDB that referenced this pull request Jan 23, 2018

Properly set STMT_ATTR_UPDATE_MAX_LENGTH attribute when needed
STMT_ATTR_UPDATE_MAX_LENGTH attribute is needed to set for statements in
case at least one column contains non-primitive type (that which has no
fixed length). When this attribute is not set then database server does not
send maximal length of variable column. And so driver would not have enough
large buffer for storing column result.

Unify also logic for determining if result would be stored in perl's PV
slot or IV/NV slot.

This patch fixes also zerofill support for MariaDB 10.2.6+ which did not
work because of unknown length of buffer.

Fixes: perl5-dbi/DBD-mysql#133

pali added a commit to pali/DBD-MariaDB that referenced this pull request Jan 23, 2018

Ensure that MYSQL_VERSION_ID and MARIADB_BASE_VERSION are defined
Macro MYSQL_VERSION_ID needs to be always set as driver has a lot of ifdefs
with this macro. And it is needed also when compiling with MariaDB. In some
client versions it is not set by main mysql.h header file and for proper
definition is needed to include mysql_version.h header file.

On the other hand macro MARIADB_BASE_VERSION is used for determining if
compiling with MariaDB or not. But in some cases MariaDB does not set this
macro which leads to incorrect compilation and broken DBD::MariaDB driver.
So define MARIADB_BASE_VERSION based on another MARIADB_PACKAGE_VERSION
which seems to be present when MARIADB_BASE_VERSION is not.

This should fix compilation with MariaDB Connector/C of version 3.

Fixes: perl5-dbi/DBD-mysql#139
Fixes: perl5-dbi/DBD-mysql#133
@pali

This comment has been minimized.

Show comment
Hide comment
@pali

pali Jan 23, 2018

Contributor

Slightly different approach was used to fix these problems in https://github.com/gooddata/DBD-MariaDB repository.

Contributor

pali commented Jan 23, 2018

Slightly different approach was used to fix these problems in https://github.com/gooddata/DBD-MariaDB repository.

@mbeijen

This comment has been minimized.

Show comment
Hide comment
@mbeijen

mbeijen Feb 5, 2018

Contributor

Partly fixed with 0b1884f

Contributor

mbeijen commented Feb 5, 2018

Partly fixed with 0b1884f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment