-
Notifications
You must be signed in to change notification settings - Fork 342
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
reconnect option is now deprecated #530
base: main
Are you sure you want to change the base?
Conversation
Since which version? It might make sense to have a pre-processor switch. |
e506bed
to
cd1f88d
Compare
Since 8.0.34 Yeah, it makes sense, I fixed it. |
@damian-tomczak
|
cd1f88d
to
ffc5a27
Compare
sry, fixed |
@damian-tomczak
Regarding the idea to set Moreover if they deprecate the reconnect functionality, I think that the MYSQL client library will just remove the So if my understanding of the issue is correct, then this PR will not help with the reconnect functionality. Once the functionality is removed from the MYSQL client, it will no longer work and if we need it in sqlpp11, we will have to implement it ourselves. |
On a side note, I always considered MYSQL's reconnect a useless and even harmful feature because it does not preserve transactions. Any serious application uses transactions and thus cannot use MYSQL's automatic reconnect. Correctly implemented automatic reconnects require support from the database access layer and from the application itself so they cannot be implemented purely in the database library, like MYSQL's automatic reconnect feature. I guess MYSQL's developers reached the same conclusion and finally decided to get rid of this evil feature which causes mostly harm. |
Quick note: My week is crazy. I will need a few days before I can look into this. But I am glad to see the discussion! Thanks! |
Based on what you have said, I would leave it how it now looks. Wdut? |
Honestly, I don't like really my proposition.
I have no idea how it looks on another platforms. |
The question is what is the purpose of the PR? Is it to silence the warnings or something else? It seems that the purpose of the patch is to silence the deprecation warnings and in order to achieve that the PR changes the mysql code in sqlpp so that it works directly with the value in MYSQL::reconnect. I consider the IMHO working with the internals of such an opaque structure just to silence a deprecation warning means begging for trouble. Regarding the correct approach to handling the deprecation warning, I am not sure, but I think one possible way handle this issue is to mark But let's also see what will rbock say about this issue. Regarding the other warnings when building the MySQL connector with MSVC, it seems that most of these are about loss of data when doing implicit casts from |
Note that the @MeanSquaredError has a good point that that whole reconnect idea is a bit wonky to begin with. Maybe the best path forward is to simply remove both the
@damian-tomczak Can you file separate issue for warnings? |
Currently I see several possible approaches:
For 5. I am thinking something along the lines of
So when the user needs to run an atomic block of SQL queries, he will call The first lambda will contain the SQL queries that the user wants to make atomically. The first lambda will return The second lambda will contain a custom handler that will be called when an exception occurs (e.g. a network error, or disconnect from the database), allowing the user to choose whether the block should be retried after a reconnect, or should be aborted. I am using a similar approach in my code and it works well for me. Moreover I think that it is the only reliable way to handle support for reconnects when you use transactions. What MySQL has/had only works with autocommit mode (i.e. no transactions), but for serious applications with transactions, the database access layer (i.e. sqlpp) needs a bit of help from the application that uses the database layer. Additionally I think that whichever approach sqlpp chooses, it should not work with the internals of the MYSQL object and should not directly change the reconnect flag there. That is more trouble than it is worth. If we just want to silence the deprecation warnings then we can just use compiler pragmas for that in the sqlpp code. |
Thanks for your detailed thoughts on this!
Proposal:
WDYT? |
@rbock
|
MySQL 8.0.34 deprecates `MYSQL_OPT_RECONNECT`. As discussed in #530, this commit is removing library support for (auto-)reconnect. It is of course still possible to reconnect directly using the native handle.
|
Actually the same pattern (atomic_block_of_work) is useful for handling all kinds of transaction failures, not just network errors. For example using pessimistic locking ( Luckily PostgreSQL offers the Repeatable read and Serializable isolation levels that effectively provide a kind of optimistic locking, you just run the transaction without any explicit locks, but if there were conflict with some other parallel transaction, then upon commit, you will get a serialization failure. In that case you repeat the whole transaction again, until it succeeds. In my opinion for complex database schemas it is much easier to use that instead of the explicit pessimistic locking. The only problem that we have with SQLPP11 is that if we use this interface
then in the retry handler we don't have an easy way to extract the exact SQL error. We don't want to repeat the transaction for all SQL error, just for some of the errors (e.g. serialization failures or network errors, etc). However So we probably need an overhaul of the exception hierarchy used by the different connectors and by sqlpp11 as a whole because currently it more or less in a state of anarchy. There is the really basic Additionally I am not sure what is the best way to pass the error to the Any comments or suggestions are welcome. |
WARNING: MYSQL_OPT_RECONNECT is deprecated and will be removed in a future version.