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

Revert inappropriate libmariadb stable ABI version changes #219

Open
wants to merge 1 commit into
base: 3.1
Choose a base branch
from

Conversation

ottok
Copy link
Contributor

@ottok ottok commented Mar 3, 2023

This reverts commits made on January 2023:
d204e83
d712484

In MariaDB 10.3.38 users complained that some of the software that relied on
MariaDB stopped working. This was due to functions mysql_get_client_info() and
mysql_get_client_version() which suddenly started to emit 3.1.20 and 30120
instead of the expected 10.3.38 and 100338.

The functions had been emitting the MariaDB server version for at least the past
8 years. There was no good justification to break the ABI in this way in a
stable release.

The issue with libqt5sql5-mysql failing was reported in https://bugs.debian.org/1031863,
which includes links to more similar issues due to this change.

Libraries must have a stable ABI, including the version they advertise. If a
library changes behaviour in a backwards incompatible manner, even if it is to
fix a bug, it should bump the version and not just silently get shipped.

This commit fixes if for both MariaDB 10.3, 10.4 and 10.5 which all include
the MariaDB Connector C branch 3.1 at v3.1.20 (ottok@d204e83).

The 10.6 series uses branch 3.3 at v3.3.4 (ottok@12bd1d5), which had this change
before 10.6 was widely used, and thus users did not end up having a breaking
change in a (fully) stable release.

Thus this fix is needed only on the 3.1 branch in MariaDB Connector C.

@uiopaubo
Copy link

uiopaubo commented Mar 3, 2023

Having reviewed possible exposure to this regression in the Debian version affected, there is one application, Kontact, which is rendered largely unusable as a consequence, at least in its default configuration, meaning that people cannot read their e-mail messages when the affected libmariadb has been installed on their system. Another application, digiKam, will probably also be affected for some users. This exposure occurs via the Qt 5 libraries.

There are other libraries, notably VTK in various versions, and applications that might also be affected. Given that this regression occurs in an older library version, exposure to it via packaged software may be limited. However, there may be a wealth of unpackaged and privately developed software deliberately relying on this older library version for stability and maintenance reasons, and so it may not be possible to obtain an overview of how much software has been broken by this unnecessary single-token change.

@9EOR9
Copy link
Collaborator

9EOR9 commented Mar 4, 2023

Hi Otto,

this was issue CONC-509 (mysql_get_client* api functions should return C/C version) which was fixed in 3.2 branch and merged into 3.3 branch 2 years ago. At the same time there was a decision to keep 3.1 but discontinue 3.2 branch - after a problem with Python I recognized that this was not fixed in 3.1 branch and added the fix into 3.1.

C/C 3.1 branch is used in 10.2 - 10.5, C/C 3.3 is used in 10.6 and later.

Without that fix it isn't possible to detect if the client library supports a specific feature or not, since instead of the client library version the server package version will be returned.

Example: To avoid a crash which was fixed in CONC-604/605 (C/C 3.1.18) you have to provide a workaround and you need to check the version number. C/C 3.1.18 was shipped with server version 10.3.36.

if (mysql_get_client_version() >= 100336)
  printf("No workaround required");
else
  printf("Workaround required");

Running this code with a client library shipped with 10.4.25 will report "No workaround required". But 10.4.25 bundles C/C 3.1.17 which doesn't provide the fix for CONC-604/605.

@uiopaubo
Copy link

uiopaubo commented Mar 4, 2023

this was issue CONC-509 (mysql_get_client* api functions should return C/C version) which was fixed in 3.2 branch and merged into 3.3 branch 2 years ago. At the same time there was a decision to keep 3.1 but discontinue 3.2 branch - after a problem with Python I recognized that this was not fixed in 3.1 branch and added the fix into 3.1.

I don't really want to restate what Otto wrote, but this fix breaks the behaviour of the 3.1 branch. In practice, what this meant was that for version 3.1.19 the function returned one kind of value, but for version 3.1.20 it returned a different kind of value.

C/C 3.1 branch is used in 10.2 - 10.5, C/C 3.3 is used in 10.6 and later.

Without that fix it isn't possible to detect if the client library supports a specific feature or not, since instead of the client library version the server package version will be returned.

This is a deficiency, and I can understand the need to fix it, but how are users of this library who are relying on the existing behaviour supposed to handle the sudden change in behaviour?

When the same change was merged into the 3.3 branch two years ago, it suddenly broke Qt's MySQL library. Now that it has been introduced to an earlier branch of this library, it now breaks the corresponding functionality in an earlier release of Qt.

@LinuxJedi
Copy link

As much as I can empathise with Debian's position, and agree this could have been done in a more API/ABI friendly way, now that this has been released it will cause more mess to revert it.

I suspect for now the best course of action would be a patch in Debian's build system or a patch for KDE's applications.

I also recommend that the Connector/C team implement some CI testing for API/ABI compatibility to stop something like this happening in future.

@vuvova
Copy link
Member

vuvova commented Mar 6, 2023

@LinuxJedi, what kind of CI testing could've prevented it? Any ideas?

@LinuxJedi
Copy link

@LinuxJedi, what kind of CI testing could've prevented it? Any ideas?

For starters a basic regression test that the version numbers produce predictable results based on the version information extracted from the source. Sounds dumb but you'd be surprised how often I've seen such a test break in projects when someone accidentally changes something else related to how the version information is generated. Even something regex based with a likely range of values would still flag this.

Granted this particular change would likely not be flagged in ABI tests specifically, but they are still valuable for libraries. ABICC is one example of a tool for this.

@uiopaubo
Copy link

uiopaubo commented Mar 6, 2023

As much as I can empathise with Debian's position, and agree this could have been done in a more API/ABI friendly way, now that this has been released it will cause more mess to revert it.

Agreed, but I want to emphasise (for the record, not to you personally) that it is not just Debian that is potentially affected by this. Any software relying on the behaviour of this function may be broken by this change. And only some of that software will be publicly available and supplied via Debian or other distributions.

I also agree that now that releases have been made with this change, it introduces the possibility that people will have started to rely on the changed behaviour. So, it is now arguably too late to do anything about this here, even in this branch. However...

I suspect for now the best course of action would be a patch in Debian's build system or a patch for KDE's applications.

The same logic would then apply to Debian's packaging of libmariadb. There exists a small possibility that developers might expect to be able to rely on this changed behaviour for software packaged for, and also developed on but not necessarily packaged for, Debian. Thus, an inconsistency between the Debian package and upstream is not desirable, either.

That leaves the workaround being made in Qt and other libraries and applications experiencing the change in library behaviour. I perceive a lack of appetite for introducing this workaround in the Debian Qt packaging, and I doubt that the Qt version involved is even maintained upstream, but my conclusion is that this would be the most appropriate place.

I also recommend that the Connector/C team implement some CI testing for API/ABI compatibility to stop something like this happening in future.

I just want to know what mysql_get_client_version was supposed to be returning all these years if that was not what it is supposed to be returning now.

@ericherman
Copy link
Contributor

I also agree that now that releases have been made with this change, it introduces the possibility that people will have started to rely on the changed behaviour. So, it is now arguably too late to do anything about this here, even in this branch. However...

Yes, it has already been released, however it is unlikely that there are many uses that rely upon the new behaviour. Thus, I'd argue that it's better to change it back now, and add a new function which returns the new value, in order to create the least problems overall.

@ottok
Copy link
Contributor Author

ottok commented Mar 11, 2023

From: https://salsa.debian.org/mariadb-team/mariadb-10.3/-/merge_requests/36

The job build mariadbclient consumer Python-MySQLdb in CI run of MariaDB 10.3.37 showed:

$ python3 -c "import MySQLdb; print(MySQLdb.get_client_info())"
10.3.37

In this commit the same CI job shows now:

$ python3 -c "import MySQLdb; print(MySQLdb.get_client_info())"
3.1.20

The intent was to revert to the same old behavior - thus this commit is actually not working.

@ottok ottok force-pushed the bugfix/libmariadb3-version-id branch from 819ba30 to 946047a Compare April 9, 2023 02:53
@ottok
Copy link
Contributor Author

ottok commented Apr 9, 2023

I did a test program and updated this PR so everything works now just like in 3.1.18 and regression in ABI change is solved:

$ # Build a test binary to verify API version strings # collapsed multi-line command
MARIADB_VERSION_ID: 100338
MYSQL_VERSION_ID: 100338
mysql_get_client_version(): 100338
mysql_get_client_info(): 10.3.38

Origin: https://salsa.debian.org/mariadb-team/mariadb-10.3/-/merge_requests/36/diffs?commit_id=92c641046fa3317dc20173d592cd52adcf25e29c

Please merge this 🙏

@ottok ottok changed the title Revert "Return correct client library version number instead of" Revert inappropriate libmariadb stable ABI version changes Apr 9, 2023
ottok added a commit to ottok/mariadb that referenced this pull request Apr 16, 2023
The libmariadb3.symbols already checks that the ABI is stable in terms
of symbols/functions. Add extra test to ensure that the version functions
also continue to output expected strings. This helps avoid issues such
as the one happened in MariaDB 10.3.38/10.5.19 and for which upstream
still has subission open:
mariadb-corporation/mariadb-connector-c#219

Example output:
    $ g++ b1031863.cpp -l mariadb && ./a.out
    MARIADB_VERSION_ID: 101102
    MYSQL_VERSION_ID: 101102
    mysql_get_client_version(): 101102
    mysql_get_client_info(): 10.11.02

On failure it might say:

    ERROR: MARIADB_VERSION_ID started with 100338 instead of the expected 1011!
ottok added a commit to ottok/mariadb that referenced this pull request Apr 17, 2023
The libmariadb3.symbols already checks that the ABI is stable in terms
of symbols/functions. Add extra test to ensure that the version functions
also continue to output expected strings. This helps avoid issues such
as the one happened in MariaDB 10.3.38/10.5.19 and for which upstream
still has submission open:
mariadb-corporation/mariadb-connector-c#219

Example output:
    $ g++ b1031863.cpp -l mariadb && ./a.out
    MARIADB_VERSION_ID: 30304
    MYSQL_VERSION_ID: 30304
    mysql_get_client_version(): 30304
    mysql_get_client_info(): 3.3.4

On failure it might say:

    ERROR: MARIADB_VERSION_ID started with 100338 instead of the expected 303!
ottok added a commit to ottok/mariadb that referenced this pull request May 15, 2023
The libmariadb3.symbols already checks that the ABI is stable in terms
of symbols/functions. Add extra test to ensure that the version functions
also continue to output expected strings. This helps avoid issues such
as the one happened in MariaDB 10.3.38/10.5.19 and for which upstream
still has submission open:
mariadb-corporation/mariadb-connector-c#219

Example output:
    $ g++ b1031863.cpp -l mariadb && ./a.out
    MARIADB_VERSION_ID: 30304
    MYSQL_VERSION_ID: 30304
    mysql_get_client_version(): 30304
    mysql_get_client_info(): 3.3.4

On failure it might say:

    ERROR: MARIADB_VERSION_ID started with 100338 instead of the expected 303!
This reverts commits made on January 2023:
mariadb-corporation@d204e83
mariadb-corporation@d712484

In MariaDB 10.3.38 users complained that some of the software that relied on
MariaDB stopped working. This was due to functions mysql_get_client_info() and
mysql_get_client_version() which suddenly started to emit 3.1.20 and 30120
instead of the expected 10.3.38 and 100338.

The functions had been emitting the MariaDB server version for at least the past
8 years. There was no good justification to break the ABI in this way in a
stable release.

The issue with libqt5sql5-mysql failing was reported in https://bugs.debian.org/1031863,
which includes links to more similar issues due to this change.

Libraries *must* have a stable ABI, including the version they advertise. If a
library changes behaviour in a backwards incompatible manner, even if it is to
fix a bug, it should bump the version and not just silently get shipped.

This commit fixes if for both MariaDB 10.3, 10.4 and 10.5 which all include
the MariaDB Connector C branch 3.1 at v3.1.20 (d204e83).

The 10.6 series uses branch 3.3 at v3.3.4 (12bd1d5), which had this change
*before* 10.6 was widely used, and thus users did not end up having a breaking
change in a (fully) stable release.

Thus this fix is needed only on the 3.1 branch in MariaDB Connector C.
@ottok ottok force-pushed the bugfix/libmariadb3-version-id branch from 946047a to 475120d Compare May 15, 2023 04:52
@ottok
Copy link
Contributor Author

ottok commented May 15, 2023

Rebased on latest MariaDB Connector C 3.1 head.

This should have been merged in March and included in the May 2023 MariaDB minor maintenance releases..

ottok added a commit to ottok/mariadb that referenced this pull request May 15, 2023
The libmariadb3.symbols already checks that the ABI is stable in terms
of symbols/functions. Add extra test to ensure that the version functions
also continue to output expected strings. This helps avoid issues such
as the one happened in MariaDB 10.3.38/10.5.19 and for which upstream
still has submission open:
mariadb-corporation/mariadb-connector-c#219

Example output:
    $ g++ b1031863.cpp -l mariadb && ./a.out
    MARIADB_VERSION_ID: 30304
    MYSQL_VERSION_ID: 30304
    mysql_get_client_version(): 30304
    mysql_get_client_info(): 3.3.4

On failure it might say:

    ERROR: MARIADB_VERSION_ID started with 100338 instead of the expected 303!
vuvova referenced this pull request Jun 27, 2023
Instead of server version the api functions mysql_get_client_info and
mysql_get_client_version should return MARIADB_PACKAGE_VERSION/ID.
@rmk92
Copy link

rmk92 commented Jun 27, 2023

I am dismayed with the comments above that seem to be believing that this is somehow Debian specific, or Debian's build system. This shows a lack of appreciation of just how silly the original change was.

Debian supply a package, libdbd-mysql-perl, which provides the mysql / mariadb integration for perl. This is not code Debian have created, it comes from the perl project. This code checks at runtime whether the version of the library it is linked against is suitable for various features. See for example:

https://github.com/perl5-dbi/DBD-mysql/blob/master/dbdimp.h#L107

Of course people are going to create code like this - it is expected that version numbers will only ever increase. This code is six years old!

However, the effect of the original commit is to rewind the version number to something substantially lower, which breaks this library. In effect, this commit has broken the expectation that version numbers increment, and has broken the perl mysql DBD driver, and from what I read above has broken many other users of mariadb.

This is not a Debian build issue. This is an issue with libmariadb having made a change that at least some of your users did not expect to ever happen.

@vuvova
Copy link
Member

vuvova commented Jun 27, 2023

I don't know why you've got an impression that this is claimed to be a Debian build issue.
It was reported by the Debian maintainer, true, but it absolutely is mariadb-connector-c issue.

@rmk92
Copy link

rmk92 commented Aug 19, 2023

Is there an update on this issue please?

@ivulfson
Copy link

ivulfson commented Sep 19, 2023

What's the status of this issue?

AFAIK this is a contributor of not being able to use TLS in perl-DBD-mysql, which is not great.

DBI connect('<snip>:mysql_ssl=1',<snip>...) failed: SSL connection error: Enforcing SSL encryption is not supported
# rpm -qiR perl-DBD-MySQL
Name        : perl-DBD-MySQL
Version     : 4.050
Release     : 13.el9
Architecture: x86_64
Install Date: Tue 29 Aug 2023 07:50:18 PM PDT
Group       : Unspecified
Size        : 372984
License     : GPL+ or Artistic
Signature   : RSA/SHA256, Tue 22 Mar 2022 02:54:04 PM PDT, Key ID d36cb86cb86b3716
Source RPM  : perl-DBD-MySQL-4.050-13.el9.src.rpm
Build Date  : Wed 16 Feb 2022 04:33:11 AM PST
Build Host  : x64-builder02.almalinux.org
Packager    : AlmaLinux Packaging Team <[email protected]>
Vendor      : AlmaLinux
URL         : https://metacpan.org/release/DBD-mysql
Summary     : A MySQL interface for Perl
Description :
DBD::mysql is the Perl5 Database Interface driver for the MySQL database. In
other words: DBD::mysql is an interface between the Perl programming language
and the MySQL programming API that comes with the MySQL relational database
management system.
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libc.so.6(GLIBC_2.34)(64bit)
libc.so.6(GLIBC_2.4)(64bit)
libmariadb.so.3()(64bit)
libmariadb.so.3(libmysqlclient_18)(64bit)
libperl.so.5.32()(64bit)
perl(:MODULE_COMPAT_5.32.1)
perl(:VERSION) >= 5.8.0
perl(Carp)
perl(DBD::mysql)
perl(DBI)
perl(DBI::Const::GetInfoType)
perl(DynaLoader)
perl(strict)
perl(warnings)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsZstd) <= 5.4.18-1
rtld(GNU_HASH)

@uiopaubo
Copy link

What's the status of this issue?

AFAIK this is a contributor of not being able to use TLS in perl-DBD-mysql, which is not great.

You might want to investigate the situation with your package provider, which appears to be AlmaLinux. As far as I can tell, Otto fixed this at least in the branch that affected me, and the update propagated through to Debian. Thanks, Otto!

Source RPM : perl-DBD-MySQL-4.050-13.el9.src.rpm
Build Date : Wed 16 Feb 2022 04:33:11 AM PST
Build Host : x64-builder02.almalinux.org
Packager : AlmaLinux Packaging Team [email protected]

These appear to be the packaging sources:

https://git.almalinux.org/rpms/mariadb-connector-c/src/branch/c9

Ignoring the awkward relationship between RHEL derivatives and Red Hat itself, I wonder what the chances are of Red Hat fixing their own packages. I recall using RHEL some time ago and discovering that the grep package's Perl-compatible regular expression support was completely broken, despite being fixed upstream years before, and it was something ridiculous like seven years before. Despite having access to Red Hat's support, the suggestion that drifted back to me via all the necessary contact people was effectively to just not use that feature. It was all very confidence inspiring: an "enterprise" distribution meaning that no-one wants to touch anything in case they break anything.

Well, I suppose here is an opportunity for AlmaLinux to add some value. Sorry for the rant!

@ivulfson
Copy link

That's an interesting observation, @uiopaubo, and I should've caught it myself. :)

I think I'm going to try to build the DBD::mysql RPM myself once I get my Alma VM squared off, and see if I can get it working without having to wait or rely on the Alma team.

@ottok
Copy link
Contributor Author

ottok commented Dec 3, 2023

FYI: I am about to push the fixed ABI (with version change reverted) to Ubuntu 20.04 "Focal" in https://salsa.debian.org/mariadb-team/mariadb-10.3/-/commits/ubuntu-20.04

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

Successfully merging this pull request may close these issues.

8 participants