-
Notifications
You must be signed in to change notification settings - Fork 256
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
base: 3.1
Are you sure you want to change the base?
Conversation
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. |
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.
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. |
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.
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. |
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. |
@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. |
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...
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 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. |
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. |
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:
In this commit the same CI job shows now:
The intent was to revert to the same old behavior - thus this commit is actually not working. |
819ba30
to
946047a
Compare
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:
Please merge this 🙏 |
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!
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!
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.
946047a
to
475120d
Compare
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.. |
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!
Instead of server version the api functions mysql_get_client_info and mysql_get_client_version should return MARIADB_PACKAGE_VERSION/ID.
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. |
I don't know why you've got an impression that this is claimed to be a Debian build issue. |
Is there an update on this issue please? |
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!
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! |
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. |
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 |
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.