Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

[Proposal] Decouple MariaDB support from MySQL #870

Open
paulofreitas opened this issue Nov 8, 2017 · 13 comments
Open

[Proposal] Decouple MariaDB support from MySQL #870

paulofreitas opened this issue Nov 8, 2017 · 13 comments
Labels

Comments

@paulofreitas
Copy link

paulofreitas commented Nov 8, 2017

Currently it is possible to use the (not officially supported) MariaDB database with the MySQL driver without many issues. But this may change in the future. Also, there are some differences between them that affects existing code.

Two things that I can list are:

They differ in more things that can still be implemented in Laravel and MariaDB 10.2.x started to get many behavior changes. I think it would be useful to officially support MariaDB through it's own driver (as other ORMs does) that would extend the MySQL one to reduce duplicated code. 😃

Things that differ in MariaDB would just overload the affected MySQL implementation.

I had a few further improvements ideas to illuminate/database that would benefit from this change.

Please, let me know what you think about that. 😄

If welcome, should I target this to 5.5 or 5.6?

@sisve
Copy link

sisve commented Nov 8, 2017

Other targets:

  1. SQL Server 2005 vs SQL Server 2008 (2005 does not support datetime2)
  2. Mysql 5.5 vs Mysql 5.6 (5.5 does not support specifying precision for several temporal data types)

@paulofreitas
Copy link
Author

paulofreitas commented Nov 8, 2017

@sisve Actually DATE, DATETIME2, DATETIMEOFFSET, GEOGRAPHY and TIME come from SQL Server 10.0 (2008), so we can say Laravel support SQL Server 10.0+ only. Sources: 1, 2, 3

Regarding MySQL, TIME(fsp) was added in 5.6, DATETIME(fsp) and TIMESTAMP(fsp) was added in 5.6.4, CURRENT_TIMESTAMP as a valid value for DATETIME was added in 5.6.5, JSON column type was added in 5.7 and virtual columns was added in 5.7.6.

I was preparing documentation updates about these limitations, I've also compiled a list for PostgreSQL, SQLite and MariaDB if it gets supported some day. 😄

I think that it would be Okay specifying a minimum required version and documenting possible unsupported features. Supporting MariaDB, on other hand, conflicts supporting MySQL, thus needing a decouple.

If intended, a future work could be done to create separate drivers for legacy versions as suggested, something like Hibernate did.

@sisve
Copy link

sisve commented Nov 8, 2017

We're supporting both SQL Server 2005 and MySQL 5.5 to a reasonable level. That is, people can create columns of types it support, datetime is one of those types.

This has happened before; see laravel/framework#18962 and laravel/framework#20465

You're welcome to suggest to drop support for these older databases, but it needs to be an explicit decision and not something that happened accidentally as a side-effect of a PR.

I imagine that dropping support for a database version in the middle of a LTS release is a breaking change...

@paulofreitas
Copy link
Author

paulofreitas commented Nov 8, 2017

@sisve I wasn't aware of laravel/framework#20465, good point. I've closed the laravel/framework#22000 temporarily to rebase the branch reverting this specific DATETIME2 change. 👍

What about the other changes? Do you think it's reasonable to target 5.6 instead?

@sisve
Copy link

sisve commented Nov 8, 2017

I believe that splitting the grammars up into database version specific things is a good thing. If this can be done with no breaking changes I believe it can target 5.5. However ...

  1. I would like the SQL Server 2008 grammar to throw NotSupportedException when it is being sent precision declarations when it isn't supported. I'm not sure this actually fits with the behavior of the migrations in general, you can do $table->string('test')->bazinga(true) and it would quietly ignore the bazinga since it isn't known.

  2. I would like the SQL Server 2008 grammar to throw NotSupportedException if it's being told to create datatypes that it does not support; like datetimeoffset. This sounds like a accepted change, it would already throw sql syntax errors, changing it to NotSupportedException would only make it easier to understand why it wasn't supported.

I'm sure you can apply the above logic to the original suggestion, MySQL vs MariaDB too.

Can we automatically detect the database version when connecting to it? Do we need to query for it with something like SELECT version();? Or do we have to let the user specify the grammar to use?

We must always have the ability to let the user override the automatic selection and pick a grammar manually. Someone could be running SQL Server 2012, but working with a database with the SQL Server 2008 compatibility level...

@paulofreitas
Copy link
Author

paulofreitas commented Nov 8, 2017

@sisve Yeah, I agree that offering legacy driver versions would be a nice idea and I also agree that legacy versions should raise exceptions for unsupported features.

Picking SQL Server for instance, currently only 2 drivers would be required: one mssql for SQL Server 10.0+ (2008+) and one mssql9 for supporting SQL Server 9.0 (2005). This would allow using DATETIME2 on mssql driver while using DATETIME on mssql9 with non-existing features disabled. 😃

Similar work could be done for other drivers, specially for MySQL and PostgreSQL (and MariaDB if implemented). This would be a win-win for everyone. 👍

For MySQL, we could split the existing implementation in 3 drivers: mysql (5.7+, all features included), mysql56 (5.6.x, with 5.7+ features disabled) and mysql55 (5.5.x, with 5.6+ features disabled).

Like the MySQL/MariaDB case, legacy drivers could extend from the main driver version and overload just what is needed.

I think it's reasonable to let the user pick an alternate grammar manually and let the framework always use the latest features on the main driver. 😉

@benyanke
Copy link

benyanke commented Nov 8, 2017

I agree this should be separated out now, while the two databases are binary compatible, so that once they diverge, the infrastructure is in place.

@paulofreitas
Copy link
Author

paulofreitas commented Nov 8, 2017

PostgreSQL: JSONB column type was added in 9.4, JSON column type was added in 9.2 and UUID column type was added in 8.3.

SQLite: foreign-keys support was added in 3.6.19.


Future features to existing drivers may add (or not) a need for a new legacy driver. For drivers providing legacy versions, it would be reasonable to provide at least (or most?) the last n previous major versions.

@staudenmeir
Copy link

@staudenmeir
Copy link

I would like to revive this idea and work on a sample implementation.

How do we structure the grammars? Does MySqlGrammar contain the latest features and we override unsupported methods in MySql55Grammar (and throw a NotSupportedException exception)?

Or is MySqlGrammar the foundation and MySql57Grammar adds new features?

Having the latest implementation in MySqlGrammar seems more logical to me. But overall, the second option sounds like it would be easier to maintain and less prone to breaking changes.

@sisve
Copy link

sisve commented Aug 27, 2018

I imagine that there would be no version-less grammar, there would be a Mysql55Grammar, Mysql56Grammar (extends Mysql55Grammar), Mysql57Grammar (extends Mysql56Grammar), etc.

This structure makes it every easy to figure when things are supported, and when there's a new version we just add a Mysql80Grammar. No need to take an existing version-less grammar and rename it.

@mfn
Copy link

mfn commented Aug 27, 2018

Having the latest implementation in MySqlGrammar seems more logical to me. But overall, the second option sounds like it would be easier to maintain and less prone to breaking changes.

Where would you specify/configure the grammar and what would the default be?

@staudenmeir
Copy link

staudenmeir commented Aug 27, 2018

@sisve I see one advantage of a version-less base grammar (if we continue not requiring a minimum database version): Having a MySql55Grammar could imply that we don't support MySQL < 5.5. A MySqlGrammar could be used for these legacy versions.

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

No branches or pull requests

6 participants