Summary:
The `mustSucceed()` function is dangerous. It was intended as a shortcut during testing, but there are other ways to do that. Remove it so that it won't be used in production code accidentally.
The RequiredOperationFailedException was generated to be thrown by this function so remove it (at least as a globally available exception) as well.
Reviewed By: tensor-flower
Differential Revision: D72977621
fbshipit-source-id: 8162592d8761acbbb7391d167f19f2bc043ae6b1
Summary: No change in behaviour, create a default exception builder class in MysqlClientBase so that users of Async and Sync mysql client can customize exceptions
Reviewed By: jkedgar
Differential Revision: D72532560
fbshipit-source-id: 86736a2684a7578d98176d18cafe4f73b356d88c
Summary:
The `resume()` function was originally intended to match 1:1 with `pause()`, but this is not perfect as it would be nice to unconditionally resume in a destructor if the work is paused. Having a separate variable - or even checking the operation's state - leads to possible race conditions. Instead, change the function to just always resume if the state is `WaitForConsumer` and don't do anything in any other state.
Also, verify that `impl_` is not null when calling the top level function.
Differential Revision: D72884849
fbshipit-source-id: 3572ec174cd29f899225dc89d1622584be435e58
Summary: We have multiple places that generate timeout messages. Combine them into one place for each operation type.
Reviewed By: fadimounir
Differential Revision: D72823894
fbshipit-source-id: 7165fb3f76c9d56c7a61e641e53f2df736f13d13
Summary: If conn object is passed in as nullptr, it is incorrect client usage, not a mysql exception
Reviewed By: jkedgar
Differential Revision: D72975115
fbshipit-source-id: fbaa7cc69dcc0d748b4a9baac231b6a2c56fb7ac
Summary:
This diff adds support for surfacing matched rows and mysql_info details (https://dev.mysql.com/doc/c-api/8.0/en/mysql-info.html) as part of DbResult.
I added explicit support to expose matched rows as a seperate field for now, as I do not want customers to parse it, and for thrift client we can have better plumbing rather than relying on string regex parsing.
Reviewed By: jkedgar
Differential Revision: D72185541
fbshipit-source-id: fd9b9aca07ba14776e9be6f71f6c4d469a55caee
Summary: The `std::variant` was introduced in C++ 17 and we fully support it now. Switch from `boost::variant` to `std::variant` in Squangle.
Reviewed By: fadimounir
Differential Revision: D71571372
fbshipit-source-id: 12c704fa44f08e2fb8326d2c569ed7ad879a050e
Summary:
Do not count errors from clients being overloaded in SLA error.
This change will effect both Xstream scuba and ods metrics from cpp client.
Differential Revision: D71860369
fbshipit-source-id: fcd9ef5c86df6a78e1dabdb36f9d0f5cc19fc4ae
Summary:
When the query timeout is set in the connection option (and a proxy is used) we set the "query_timeout" query attribute and the proxy (MyRouter or Dvaar) will use this to do backend query timeouts (https://fburl.com/code/1jhodoml) for us.
Unfortunately, we weren't overriding this if the query options set a query timeout - we were only updating the client-side timeout. That left the proxy to have a different timeout than expected.
Add code to override the query_timeout query attribute when a query timeout is specified via query options.
Also add a test to validate this (and verify that the connection's overall query timeout doesn't get changed).
Reviewed By: fadimounir
Differential Revision: D71411501
fbshipit-source-id: e5389d03f899f5de9f410c79b3b82fdd4ca7b09e
Summary:
In D70333626 I attempted to not make active_fetch_action_ an atomic. This variable is only changed in the mysql event base and only needs to be read (very occasionally) outside of this thread. My thought was that inside the thread there would be no protection and from outside the thread we would schedule a function to read it inside the main thread.
Unfortunately this broke https://www.internalfb.com/intern/test/844425027465877?ref_report_id=0 as the test causes the variable to be read outside of the event base thread, but the event base thread is in a blocked state, so the code never runs and the test just freezes.
The correct solution is just to make the variable an atomic.
Differential Revision: D70713529
fbshipit-source-id: bd61baca28d337fa99cb8d389619501937af4ed2
Summary:
Move the cancel() operation (for fetches) into the mysql thread since we are releasing memory that is owned by that thread.
Make state_ atomic and provide a new `isCancelling()` function.
Make isPaused() work without TSAN issues when called from outside the MySQL thread.
Reviewed By: aditya-jalan
Differential Revision: D70333626
fbshipit-source-id: 1580830afdf37c0f04ec24ec8ae5db5d21e1627b
Summary:
LOG_EVERY_N uses two integers to keep track of how many times the line has been hit. If the code is run from multiple threads this will cause TSAN errors. Implement our own version that uses an atomic.
This will be a bit more expensive due to the `%` operator, but we can afford that in this code.
Reviewed By: aditya-jalan, sdunster
Differential Revision: D69489221
fbshipit-source-id: f2c191bdf11f3acf8027f4d6fdbbd46b1ef76ce4
Summary:
These flags disallow implicitly generated constructors for classes that have a user-provided destructor (such as base classes with a virtual destructor).
Thus, explicit default constructors have been provided for all the classes that need them.
Differential Revision: D68797540
fbshipit-source-id: 4aec3fd243b6ee773f1e4ac21eb3e0eb276cf4af
Summary:
Add support for tuples to easyquery lib. Stuff like:
WHERE (a, b) = (1, 2)
or
WHERE (a, b) < (1, 2)
or
WHERE (a, b) IN ((1, 2), (3, 4))
It is possible this is already doable in the existing lib but I couldn't figure out how in a nice way. Actually I still couldn't figure out how to do it in a nice way, but I wrapped up the complexity so lib users don't have to deal with it (they make a Tuple object with a list of values, which I convert to a Query object to embed in the larger query).
Differential Revision: D69356885
fbshipit-source-id: 212e7f481bb2cada9a12dfd2d3081a26812473ad
Summary:
This diff reverts D69283843
Squangle is OSS and `common/logging/logging.h` breaks it out in the wild.
Reviewed By: jkedgar
Differential Revision: D69467564
fbshipit-source-id: e36ab71998e9a37d16433550601a1b995bfc5eb7
Summary:
Archival code is running into TSAN issues:
{P1727122467}
I saw here that we can potentially fix by using the atomic version of the macro, probably with slightly higher CPU cost.
Proposing this diff so we can discuss and/or land the fix.
Reviewed By: jkedgar
Differential Revision: D69283843
fbshipit-source-id: 7f95fe55af592611f532ba282ef09746626e99d2
Summary:
Recent changes to Squangle broke the OSS version.
The `common/db/sql_builder/logging/OdsCounterHelper.h` header file is not available in the OSS world, so it shouldn't be referenced in Squangle.
Changes inspired by suggestions by Máté Szabó in https://github.com/facebook/squangle/pull/18
Reviewed By: fadimounir
Differential Revision: D69156956
fbshipit-source-id: 7dd7656e44b8b92fef9eaf28527128dd8667d4b8
Summary:
Recent changes to Squangle broke the OSS version.
Even though `memcpy` isn't the safest function, any safer alternatives are not available in the OSS build. Just use it.
Changes inspired by suggestions by Máté Szabó in https://github.com/facebook/squangle/pull/18
Reviewed By: fadimounir
Differential Revision: D69156815
fbshipit-source-id: 1b2c410a884217b465e594b6ee6f87bd8ddaeec1
Summary:
Recent changes to Squangle broke the OSS version.
Add formatters for the two enums that we were trying to format.
Changes inspired by suggestions by Máté Szabó in https://github.com/facebook/squangle/pull/18
Reviewed By: fadimounir
Differential Revision: D69156802
fbshipit-source-id: f4ec2780b4daf01f3800e3bd9fc41b4bdaa89adc
Summary:
Recent changes to Squangle broke the OSS version.
In this diff, move the definitions of the Query constructors and assignment operators into the .cpp file to avoid pointer arithmetic with an incomplete definition of QueryArgument.
Changes suggested by Máté Szabó in https://github.com/facebook/squangle/pull/18
Reviewed By: fadimounir
Differential Revision: D69155037
fbshipit-source-id: 478ab795d4bd30536fff617900c93da49c7d7e56
Summary:
Run linter on all files in common/db/mysql_client
```
arc lint --lintall --engine extra squangle/mysql_client/mysql_protocol/*
```
Reviewed By: tensor-flower
Differential Revision: D68467189
fbshipit-source-id: fedd7243700195daef0fcd38f48107ee72bdc4f2
Summary:
Run linter on all files in squangle/mysql_client:
```
arc lint --lintall --engine extra squangle/mysql_client/*
```
Differential Revision: D67904180
fbshipit-source-id: a22c2e5c2cdd1961de01a0b54684627041696dcd
Summary:
Customers have run into an occasional crash where the Operation appears to have been deallocated when certificate validation occurs (P1625691373).
My current theory is that the connect operation is being cancelled or timed out and freeing the operation even though it is still processing and OpenSSL calls the callback and attempts to use the memory from the operation.
In this diff I attempt to hold a weak_ptr to the operation for use in the certificate verification callback. When we lock the weak_ptr to generate a shared_ptr, if it fails because the operation is gone we will just return an error and not attempt to access the memory.
Note: this is a second attempt. The first (D66730727) caused some problems in HHVM that I had to resolve.
Reviewed By: mdko
Differential Revision: D67996104
fbshipit-source-id: bc61c688aff27e1421342bcc562b0b694593bbfe
Summary:
In D67400934 the RowStream object (stored in `current_row_stream_`) would be freed on a cancel. However, there was at least one code path where we could attempt to access it again in this scenario. This broke a handful of tests.
Fix the code to correctly handle this scenario.
Differential Revision: D67909302
fbshipit-source-id: cfad60ec70521378b73bd4e14a9e20324e30f0f2
Summary:
Customers have run into an occasional crash where the Operation appears to have been deallocated when certificate validation occurs (P1625691373).
My current theory is that the connect operation is being cancelled or timed out and freeing the operation even though it is still processing and OpenSSL calls the callback and attempts to use the memory from the operation.
In this diff I attempt to hold a weak_ptr to the operation for use in the certificate verification callback. When we lock the weak_ptr to generate a shared_ptr, if it fails because the operation is gone we will just return an error and not attempt to access the memory.
Reviewed By: gpaul
Differential Revision: D66730727
fbshipit-source-id: f592b3ba0419c9e0cfb607aa5c5397a6769019c8
Summary: When using a streaming version, it is possible for the fetch operation to be cancelled after receiving some rows. In this case the RowStream objects need to be released before the connection can be closed. This change allows this to happen by making the `cancel()` function a virtual function and overriding it in the fetch operation to do the free before continuing with the normal cancel.
Reviewed By: fadimounir
Differential Revision: D67400934
fbshipit-source-id: 3272ea77b52446716cfbbd1009c97d76a0d33f60
Summary:
In D65959868 I added the ability to get db_version when going straight to MySQL.
In D66184293 I fixed a bug in that implementation when a failed connection attempts to log this information when using the MySQL protocol.
Unfortunately, the Thrift protocol has the same problem (here: https://fburl.com/code/erpqxl3l).
Thinking it through more, it makes more sense for the `serverInfo()` function to return an empty string when there is no valid connection than to throw an exception, so I am making that change and undoing part of the fix for D66184293 since the `serverInfo()` function is now safe to call no matter the state of the connection.
Differential Revision: D66253298
fbshipit-source-id: 15e959c34c58295d81715c8cfd58da947227da17
Summary: D65959868 caused a crash when attempting to log the server information on an invalid connection. Add a check for this.
Reviewed By: fadimounir
Differential Revision: D66184293
fbshipit-source-id: 0f536ea8e2e4615d44fe942e08ccf8345e581ad1
Summary:
Currently we only populate the `db_version` field in the logging when it is returned as a response attribute for a query. This means only queries that go through MyRouter get this field included. However we have this information in the connection when the client connects. It will be the MyRouter's (or Dvaar's) version when using a proxy, but will be the correct MySQL version when going directly to MySQL (like TAO does).
If we populate this information into the sample first (for connections as well as queries) it can then be overridden for MyRouter/Dvaar when the response attribute comes back.
This change will allow queries going to MySQL to have the `db_version` field set.
In addition we will now have the `db_version` set for successful connections as well. For direct MySQL connections it will be the MySQL's version. For proxy connections it will be the MyRouter or Dvaar version - which is less meaningful but theoretically could be helpful.
Differential Revision: D65959868
fbshipit-source-id: 99d6ef76b7c3fbc4bbbdf0f41b14770729198252
Summary: We were building the exact same QueryLoggingData object in the success and failure paths of the logging function. Refactor to do this just once. Also, pull the conversion of `OperationResult` to `FailureReason` into a helper function to make the code cleaner.
Differential Revision: D65953455
fbshipit-source-id: a35982716b4bb7cab812f84963053f5d199c5cac
Summary: TSIA. This makes the migration a bit easy with no additional cost except some code. Example next diff: D66009586
Differential Revision: D66119825
fbshipit-source-id: cffe2810cf74c9a4f99fc90c3bedaccf96afb0d6
Summary:
When refactoring the Squangle code to handle data arriving in native format, I accidentally broke what happens when reading data that is null. Originally it would throw a `folly::ConversionError` (except for strings which would be empty), but I accidentally generated a failure using DCHECK.
Revert to the correct behavior and add a test for the future.
Differential Revision: D65502033
fbshipit-source-id: 96b79ebd535d92c6d53b259cbec2680a7c85dcbc
Summary: In D63903145 I changed the way data was stored in Squangle. Unfortunately I broke what should happen when a null is returned and the client asks for the data as a `folly::dynamic` and none of the tests caught it in that diff. Fixing that here.
Reviewed By: aditya-jalan
Differential Revision: D65434029
fbshipit-source-id: 000698994e72f05d39340a0be2dea1a03be81c2e
Summary:
Original summary:
The Squangle Row class always stored data in string format because that is the format it was delivered by the MySQL protocol. If a different protocol delivers it in native format it would be good to not convert it to strings. Add support for native formats.
Reviewed By: fadimounir
Differential Revision: D63903145
fbshipit-source-id: 4def520c0ffa489051f55ef87ef2d61516855a26
Summary:
This diff introduces a new class called StorageRow which is designed to store MySQL row data in a space-efficient manner. The class is intended to work when the protocol returns data in string format or in native format, and it stores data in a way that allows for efficient retrieval and manipulation.
The original implementation used a minimum of 8 bytes plus a bit for each column. The bit was for indicating NULL values and the 8 bytes was an offset in a data buffer where the actual data was written. This resulted in `8 bytes + 1 bit + actual data contents` data needed.
In this version we have 4 bytes plus actual data written, where the actual data written can be optimized (i.e. only write 1 byte for integers near 0). This is accomplished by splitting handling of small things (anything less than 4K) directly while offloading any column larger than 4K in separate storage. The 4 bytes are used as 3 bits holding the type of data and 29 bits holding an offset into the data buffer. 29 bits can reference 512M and since only columns smaller than 4K are stored in the referenced space, we could theoretically handle 131,000 columns. MySQL has a maximum number of columns per table of 4096, so this would support a result set with 32 tables joined with 4096 columns per table which should be way more than needed.
The final result is that for NULLs and booleans, only 4 bytes are needed. For integers, we have 4 bytes of overhead plus either 1, 2, 4, or 8 bytes of data (depending on how close to 0 the value is). Doubles take 4 bytes + 8 bytes. Strings shorter than 4K take 4 bytes plus the size of the string. Long strings are stored in a separate storage but take 4 bytes plus a variable integer size in main storage data section.
Differential Revision: D63793585
fbshipit-source-id: 7de28c2066bd831b0da32145fe0a1d6024183621
Summary:
This is a second attempt at this diff and combines the original diffs of D57460644, D63141477 and D63267393.
Original text:
In order to make client migration from MySQL protocol to Thrift protocol as seemless as possible, we are making the top level FbAsyncMysqlClient, FbAsyncConnectionPool, FbSyncMysqlClient and FbSyncConnectionPool into gating classes. This means renaming the existing clients and replacing them with the gating clients (the gating clients only choose between MySQL protocol and Thrift protocol).
The following get renamed:
- FbAsyncMysqlClient -> FbAsyncMsyqlProtocolClient
- FbAsyncConnectionPool -> FbAsyncMysqlProtocolConnectionPool
- FbSyncMysqlClient -> FbSyncMysqlProtocolClient
- FbSyncConnectionPool -> FbSyncMysqlProtocolConnectionPool
Next the original clients are recreated with code that checks the gating status and either routes request to the renamed class or to the FbThriftMysqlClient class.
Note that Thrift doesn't have expensive connections so there is no need to have connection pooling, so the connection pool versions go to FbThriftMysqlClient - not to some pooled version of the thrift client.
In the original version the FbThriftMysqlClient class included a FunctionScheduler, which is a fairly expensive object. The purpose was to provide for a background function that would run occasionally. Unfortunately, when lots of FbThriftMysqlClient objects get created this greatly increased the memory needed for the binary. The FunctionScheduler has been removed and instead a cancellable coroutine runs in a loop and is cleaned up on destruction. This is much less expensive.
In addition, when creating both a client and a pool from the client, we were mistakenly creating two FbThriftMysqlClient objects rather than reusing the one from the client in the pool.
Reviewed By: fadimounir
Differential Revision: D63903128
fbshipit-source-id: 04df32917408ab4dd15528770f784cf461e987c4
Summary: We've added the ods counter to GeneratorHelper class but missed the Connection class.
Differential Revision: D63849880
fbshipit-source-id: 7bce7f7223e73732ea6333cd27b33ca9292975aa
Summary: While working on S448897 I generated a quick diff to try to handle an error condition. The check should not be necessary as we should never get to these lines of code with a null `res_`. Later I found the real problem and fixed it in D63276382 which has landed. I can now revert this hack.
Differential Revision: D63646255
fbshipit-source-id: e233ddd9cdc9315778bbbd1e28d5f35fe4b69ca7
Summary: S452742 - need to revert this to revert D56488104
Differential Revision: D63403574
fbshipit-source-id: 5a403f88f43fbf1e74ce7448a9bf7a988af6b346
Summary: S452742 - need to revert this to revert D56488104
Differential Revision: D63403575
fbshipit-source-id: 9a979b559bd695d141baa1a55de948ce5910dfc5
Summary:
These two functions can return null and we are just putting this into a MysqlResult which assumes that it will not be null. We should not be building a MysqlResult under this scenario.
Something in the recent refactor seems to have changed what happens around this code and TAO is seeing an occasional crash as a result (S448897). This may resolve the issue.
Reviewed By: fuu
Differential Revision: D63276382
fbshipit-source-id: 365896635d9de45c434fe4df290b6ce53afb76cd
Summary:
The way the singletons for SyncMysqlClient and AsyncMysqlClients were created is out of date. Modify the code to match the suggested way in folly/Singleton.h.
Also removed some unneeded #include directives.
Differential Revision: D63267393
fbshipit-source-id: b90429ee0b6f0d8bc3320b2141f89633067ac9d4
Summary: As requested in D49613234, adding `[[nodiscard]]` where appropriate in the Connection class. Also added `noexcept` to a few functions that were trivial.
Reviewed By: aditya-jalan
Differential Revision: D58263970
fbshipit-source-id: 570c31b51e38e3ab62f23355e766ba215cdf0a72
Summary: I went through and found all the places using the functions that deal with operations directly and fixed them. That means these low-level functions for dealing with transactions should now be unused. Remove them.
Reviewed By: aditya-jalan
Differential Revision: D58263853
fbshipit-source-id: 0b99f7151c16b93e5fa209bf15cce18e5bf40786
Summary: The Squangle Row class always stored data in string format because that is the format it was delivered by the MySQL protocol. If a different protocol delivers it in native format it would be good to not convert it to strings. Add support for native formats.
Reviewed By: aditya-jalan
Differential Revision: D57574468
fbshipit-source-id: be142ba14c1cecb66cc1f8a84e45a406d5e374f8
Summary: I'm investigating a TAO crash and while I'm not yet sure of the root cause, I know where it is crashing. Add some checks in the code to prevent the crash from occurring to see if TAO can make progress while waiting for the root cause fix.
Differential Revision: D63038824
fbshipit-source-id: d01e733f152ccbf19f14055295d9a920e3a112ee
Summary:
A bunch of miscellaneous changes needed to add another client, including:
- Call `addOperation()` in `run()` instead of having each callsite do it before `run()`. This simplifies the code.
- Add a `AsyncConnectionHelper` class to derive AsyncConncetion` from. This helps future clients that have a Connection class that is asynchronous in nature to avoid duplicating this code.
- minor changes with dealing with running code in the "correct" thread
- other miscellaneous changes
Reviewed By: fadimounir
Differential Revision: D58366798
fbshipit-source-id: 18b6a56ea0b9dfe31c730b0faff3b1094d6fefb9
