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

Upgrade to EF Core 8.0.0-rc.1 #1791

Merged
merged 35 commits into from
Sep 29, 2023

Conversation

lauxjpn
Copy link
Collaborator

@lauxjpn lauxjpn commented Sep 29, 2023

The upgrade is compatible with EF Core 8.0 RC1.

Everything compiles and all supported tests are green for all officially supported database engine versions:

  • MySQL 8.0
  • MySQL 5.7
  • MariaDB 11.1
  • MariaDB 11.0
  • MariaDB 10.11
  • MariaDB 10.10
  • MariaDB 10.6
  • MariaDB 10.5
  • MariaDB 10.4

The following versions have reached their end-of-live and are not actively tested by us anymore (but are expected to work anyway):

  • MariaDB 10.9
  • MariaDB 10.8
  • MariaDB 10.7
  • MariaDB 10.3

We currently have the following test statistics (here for MySQL 8):

Test suite Tests Succeeded Failed Skipped
Functional 29,527 29,144 0 383
Integration 37 37 0 0
Custom 51 51 0 0

Primitive collections support

There are some serious bugs in the implementation of JSON_TABLE() in both, MySQL and MariaDB.
Because of that, we have disabled primitive collections support by default for now.
It can be enabled using the EnablePrimitiveCollectionsSupport option. If you do, make sure you read about possible issues below.

MySQL

While current versions of MySQL pretty much implement the whole spectrum that we need for proper primitive collections support, there are multiple fatal bugs that will crash the MySQL database engine (not the Pomelo provider, but the actual mysqld process).

Most crashes are connected to using a parameter that contains a JSON array as the source of the JSON_TABLE() call (e.g. ... FROM JSON_TABLE(@p0, ...) with @p0 = "[1,2,3]"). The bug is kind of non-deterministic and only crashes the database engine process after a couple of those queries.
We work around this bug by inlining those constant values directly into the SQL, instead of using the parameter.

There is at least one more bug, that is deterministic and crashes the database engine process immediately every single time. This can be reproduced by running any of the following two tests:

  • NorthwindSelectQueryMySqlTest.Correlated_collection_after_distinct_not_containing_original_identifier(bool async)
  • NorthwindSelectQueryMySqlTest.Correlated_collection_after_groupby_with_complex_projection_not_containing_original_identifier(bool async)

MariaDB

MariaDB does not crash, but it still does not supports LATERAL join support (which is in many cases implicitly needed to make JSON_TABLE() useful).

If aggregates are used over JSON_TABLE() (e.g. COUNT(*)), then a referenced column from the outer table will always contain the value of the first row, instead of the current row, which is very much unexpected.
Referencing columns further up is not supported at all (due to missing LATERAL join support).
There are additional cases, where MariaDB behaves oddly, that have to be investigated further.

This all results in many primitive collection queries not working, when using JSON_TABLE().


Addresses #1746
Fixes #1784

…roperty).ElementAt(index), because it messes-up our JSON-Array handling in `MySqlSqlTranslatingExpressionVisitor`. See dotnet/efcore#30386.
… when using JSON_TABLE(@JSON, ..) with @JSON being a parameter.
…TABLE(@JSON, ..) with @JSON being a parameter, by directly inlining the parameter values in question into the generated SQL.
@lauxjpn lauxjpn added this to the 8.0.0-beta.1 milestone Sep 29, 2023
@lauxjpn lauxjpn self-assigned this Sep 29, 2023
@lauxjpn lauxjpn force-pushed the upgrade/8.0.0-rc.1 branch 5 times, most recently from b6a5a9a to af0bb36 Compare September 29, 2023 04:11
@lauxjpn lauxjpn merged commit 6d38ffe into PomeloFoundation:master Sep 29, 2023
11 checks passed
@lauxjpn lauxjpn deleted the upgrade/8.0.0-rc.1 branch September 29, 2023 11:21
@lauxjpn lauxjpn mentioned this pull request Sep 29, 2023
@roji
Copy link

roji commented Oct 1, 2023

Ouch, sorry to hear about these database engine issues @lauxjpn... Hopefully getting everything working in the old constant mode didn't present too many difficulties (as we also do in SQL Server when the compatibility level is too old).

Any issues on the MySQL/MariaDB side tracking these?? It certainly isn't great if the engine itself crashes 😅

@lauxjpn
Copy link
Collaborator Author

lauxjpn commented Oct 1, 2023

Hopefully getting everything working in the old constant mode didn't present too many difficulties (as we also do in SQL Server when the compatibility level is too old).

@roji Yeah, that is what we are currently doing. We will cleanup some code for the next release and will probably duplicate the test class a couple of times for the different database engine version and workaround cases.

Any issues on the MySQL/MariaDB side tracking these?? It certainly isn't great if the engine itself crashes 😅

I have not filed the bugs yet, but will over the next days, after I assembled the respective MREs. If Oracle isn't fixing this in a timely manner, I might do it myself to figure out appropriate workarounds along the way that we can implement.

@roji
Copy link

roji commented Oct 2, 2023

[...] We will cleanup some code for the next release and will probably duplicate the test class a couple of times for the different database engine version and workaround cases.

Yeah, we similarly have two implementations of PrimitiveCollectionsQueryRelationalTestBase, one for modern SQL Server and one with the compatibility flag set, where the older expand-to-constant strategy is used (source code).

I have not filed the bugs yet, but will over the next days, after I assembled the respective MREs. If Oracle isn't fixing this in a timely manner, I might do it myself to figure out appropriate workarounds along the way that we can implement.

Good luck! It will be interesting to follow what happens with this, and I hope MySQL/MariaDB get to a place where JSON_TABLE can be used safely by anyone...

@ajcvickers
Copy link

@lauxjpn Really appreciate your hard work on this, as always.

@roji
Copy link

roji commented Oct 2, 2023

Indeed!

@gthvidsten
Copy link

gthvidsten commented Jul 17, 2024

@lauxjpn Late to the party here, but I'm currently looking into primitive collections and especially the EnablePrimitiveCollectionsSupport switch. Do you know if this has been fixed in later versions of MySQL? If not, Is there a report in their bug tracker you could link to so that I can follow their progress?+

Also, I tried enabling the option (see below) but I still get a "Primitive collections support has not been enabled" (using v8.0.2)

services.AddDbContextFactory<MyDbContext>(
    options => options.UseMySql(
        connectionString,
        ServerVersion.AutoDetect(connectionString),
        x => x
            .EnablePrimitiveCollectionsSupport()
            .MigrationsAssembly("MyAssembly")));

@gthvidsten
Copy link

The order of the options extensions is important. If I move EnablePrimitiveCollectionsSupport() to be after .MigrationsAssembly() then the error goes away.

However, a standard EF query that produces correct results in a SQLite database, does not produce any result at all in MySQL so there might still be some issues with the implementation.

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.

DateTimeOffset.ToUnixTimeMilliseconds & DateTimeOffset.ToUnixTimeSeconds support
4 participants