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

Fix MariaDB fetching of default table character-set #6425

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

grooverdan
Copy link
Contributor

@grooverdan grooverdan commented Jun 7, 2024

Q A
Type bug
Fixed issues #6361

Summary

From MariaDB-10.10.1, where uca1400 was added, the information_schema.COLLATION_CHARACTER_SET_APPLICABILITY was extended to have FULL_COLLATION_NAME which corresponds to the information_schema.TABLES.TABLE_COLLATION value.

Executable comment syntax is used to limited to the applicable versions.

To preserve compatibility with older MariaDB versions, and MySQL versions where the previous COLLATION_NAME was the match is left as a JOIN criteria. In new MariaDB versions this won't result in an extra row match.

Closes: #6361

The lack of this fix did case the CI test to fail with a MariaDB-11.0+ container:

1) Doctrine\DBAL\Tests\Functional\Schema\MySQLSchemaManagerTest::testEnsureTableWithoutOptionsAreReflectedInMetadata
Undefined array key "engine"

/home/runner/work/dbal/dbal/src/Schema/Table.php:927
/home/runner/work/dbal/dbal/tests/Functional/Schema/MySQLSchemaManagerTest.php:550

With this fix all version of MySQL and MariaDB (even those new ones soon to be in a different PR) will pass like: https://github.com/grooverdan/dbal/actions/runs/9412110648/job/25926481038

@grooverdan grooverdan changed the base branch from 4.0.x to 3.8.x June 7, 2024 05:47
@grooverdan
Copy link
Contributor Author

SQL example:

MariaDB [test]> select version();
+----------------+
| version()      |
+----------------+
| 11.5.2-MariaDB |
+----------------+
1 row in set (0.001 sec)


MariaDB [test]> create table t (c varchar(30)) charset 'utf32' collate 'uca1400_ai_ci';
Query OK, 0 rows affected (0.002 sec)

MariaDB [test]> SELECT t.TABLE_NAME,            t.ENGINE,            t.AUTO_INCREMENT,            t.TABLE_COMMENT,            t.CREATE_OPTIONS,            t.TABLE_COLLATION,            ccsa.CHARACTER_SET_NAME       FROM information_schema.TABLES t         INNER JOIN information_schema.COLLATION_CHARACTER_SET_APPLICABILITY ccsa             ON /*M!101001 ccsa.FULL_COLLATION_NAME = t.TABLE_COLLATION OR */ COLLATION_NAME = t.TABLE_COLLATION WHERE t.TABLE_NAME='t';
+------------+--------+----------------+---------------+----------------+---------------------+--------------------+
| TABLE_NAME | ENGINE | AUTO_INCREMENT | TABLE_COMMENT | CREATE_OPTIONS | TABLE_COLLATION     | CHARACTER_SET_NAME |
+------------+--------+----------------+---------------+----------------+---------------------+--------------------+
| t          | InnoDB |           NULL |               |                | utf32_uca1400_ai_ci | utf32              |
+------------+--------+----------------+---------------+----------------+---------------------+--------------------+

From MariaDB-10.10.1, where uca1400 was added, the
information_schema.COLLATION_CHARACTER_SET_APPLICABILITY was
extended to have FULL_COLLATION_NAME which corresponds to
the information_schema.TABLES.TABLE_COLLATION value.

Executable comment syntax is used to limited to the applicable
versions.

To preserve compatibility with older MariaDB versions, and
MySQL versions where the previous COLLATION_NAME was the match
is left as a JOIN critieria. In new MariaDB versions this won't
result in an extra row match.

Closes: #6361
Comment on lines -570 to +576
ON ccsa.COLLATION_NAME = t.TABLE_COLLATION
ON /*M!101001 ccsa.FULL_COLLATION_NAME = t.TABLE_COLLATION OR */
ccsa.COLLATION_NAME = t.TABLE_COLLATION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid version conditionals in SQL. We have our platform class hierarchy for this purpose.

I'll accept this change as a hotfix for MariaDB 11.4 on the 3.8.x branch. Can you work on a PR for 3.9.x that moves the SQL statement to AbstractMySQLPlatform and that introduces a new platform class for MariaDB 10.10 which overrides the generated SQL?

@derrabus derrabus added this to the 3.8.5 milestone Jun 7, 2024
@derrabus derrabus merged commit 4aa9cf9 into doctrine:3.8.x Jun 7, 2024
92 checks passed
@derrabus derrabus changed the title Fix MariaDB fetching of default table character-set (#6361) Fix MariaDB fetching of default table character-set Jun 7, 2024
derrabus added a commit that referenced this pull request Jun 7, 2024
* 3.8.x:
  Fix MariaDB fetching of default table character-set (#6361) (#6425)
  Fix the portability documentation (#6429)
  Update tests/Platforms/AbstractPlatformTestCase.php
  Update tests/Platforms/AbstractPlatformTestCase.php
  add test
  Fix: Skip type comparison if disableTypeComments is true
derrabus pushed a commit that referenced this pull request Jun 8, 2024
<!-- Fill in the relevant information below to help triage your pull
request. -->

|      Q       |   A
|------------- | -----------
| Type         | improvement
| Fixed issues | N/A <!-- use #NUM format to reference an issue -->

#### Summary

Improve the CI to cover newer versions of MariaDB 11.4+.

Also cover the quay.io/mariadb-foundation/mariadb-devel images from
https://quay.io/repository/mariadb-foundation/mariadb-devel?tab=tags
where they are updated with completed, reviewed and tested changes in
the MariaDB server main branch.

These are completed features/bug fixes in the next server version. As
such this is a good thing to test before the users get the final
product.

ref:
https://mariadb.org/new-service-quay-io-mariadb-foundation-mariadb-devel/

The test failures here are fixed by #6425.

<!-- Provide a summary of your change. -->
derrabus added a commit that referenced this pull request Jun 14, 2024
* 4.0.x:
  CI: Update MariaDB versions (#6426)
  CI MariaDB: add 11.4, remove 11.0 (#6432)
  Display warnings when running PHPUnit in CI (#6431)
  Fix typo in the portability documentation (#6430)
  Fix MariaDB fetching of default table character-set (#6361) (#6425)
  Fix the portability documentation (#6429)
  Update tests/Platforms/AbstractPlatformTestCase.php
  Update tests/Platforms/AbstractPlatformTestCase.php
  add test
  Fix: Skip type comparison if disableTypeComments is true
derrabus added a commit to derrabus/dbal that referenced this pull request Jun 19, 2024
* 4.1.x: (25 commits)
  Simplify signature of fetchTableOptionsByTable
  Add MariaDb1010Platform for fetchTableOptionsByTable
  PHPUnit 10.5.21 (doctrine#6447)
  Move schema split for SQLite CREATE INDEX only (doctrine#6352)
  PHPStan 1.11.5 (doctrine#6446)
  Default to distinct union queries (doctrine#6439)
  Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection"
  Add `QueryBuilder` support for `UNION` clause (doctrine#6369)
  CI: Update MariaDB versions (doctrine#6426)
  CI MariaDB: add 11.4, remove 11.0 (doctrine#6432)
  Display warnings when running PHPUnit in CI (doctrine#6431)
  Fix typo in the portability documentation (doctrine#6430)
  Fix MariaDB fetching of default table character-set (doctrine#6361) (doctrine#6425)
  Fix the portability documentation (doctrine#6429)
  Update tests/Platforms/AbstractPlatformTestCase.php
  Update tests/Platforms/AbstractPlatformTestCase.php
  add test
  Fix: Skip type comparison if disableTypeComments is true
  Remove redundant variable (doctrine#6326)
  Fix test names to reflect their actual purpose
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

COLLATION_CHARACTER_SET_APPLICABILITY.COLLATION_NAME vs MariaDB FULL_COLLATION_NAME
2 participants