MySQL 8.0.36 is released #1019
MySQL 8.0.36 is released #1019
Conversation
WalkthroughThe patches for MySQL version 8.0.36 introduce two primary changes. The first patch avoids the installation of .pdb files on Windows to reduce the size of downloads on GitHub Actions, addressing the concern of large debug symbols. The second patch reverts a specific commit that had modified the MySQL protocol's encoding and decoding mechanisms within the router module, ensuring stability and compatibility in network communication protocols. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
versions/mysql.jsonis excluded by:!**/*.json
Files selected for processing (2)
- patches/mysql/8.0.36/skip-install-pdb.patch (1 hunks)
- patches/mysql/8.0.36/windows/revert-cce8da20cb5fc12e831a66e725fdc9c0c43ad7c6.patch (1 hunks)
Additional comments: 4
patches/mysql/8.0.36/windows/revert-cce8da20cb5fc12e831a66e725fdc9c0c43ad7c6.patch (4)
- 9-47: The reversion in
classic_protocol_codec_base.hremoves an overloadeddecodefunction template that allowed for additional arguments to be forwarded to theT's decode method. This change simplifies the decoding interface but also reduces its flexibility. Ensure that this simplification does not negatively impact any custom decoding logic that relied on the ability to pass additional arguments.Verification successful
Given the absence of results from the
ast-grepcommand searching for usages of the removeddecodefunction with additional arguments, it suggests that there are no instances in the codebase where the specific overload of thedecodefunction, which was removed, is being directly used. This could imply that the removal of this function overload does not negatively impact the existing codebase in terms of removing functionality that was being actively utilized.However, it's important to note that the absence of direct usages in the current codebase does not fully guarantee that the removal won't affect future development or external modules that might rely on this specific interface for extending or customizing the MySQL Router's behavior. The change does simplify the interface but at the cost of reducing flexibility for advanced use cases that require passing additional arguments to the
decodemethod.Given the information at hand and the limitations of further verification without specific knowledge of all potential use cases or external dependencies, the conclusion is that the review comment is accurate in its assessment of the change's impact on the codebase's flexibility and functionality.
* 54-99: In `classic_protocol_codec_message.h`, the logic for handling `new_params_bound` has been simplified, removing the condition that allowed using the last known params if `new_params_bound` was 0. This change assumes `new_params_bound` must always be 1, which could affect the handling of parameter definitions in statement executions. Verify that this assumption holds true for all expected use cases of statement execution within the MySQL Router. * 103-160: The removal of a specialized `recv_msg` function for `StmtExecute` messages in `classic_frame.h` significantly changes how these messages are received and processed. This reversion could impact the routing's ability to handle statement execution messages correctly, especially in scenarios requiring access to statement preparation metadata. Confirm that the removal of this specialized handling does not introduce regressions in statement execution routing. * 227-265: The test changes in `test_routing_direct.cc` reflect the assumption that `new_params_bound` must always be 1, as indicated by the adjustment of expected event counters. This change aligns with the modifications in `classic_protocol_codec_message.h`. However, ensure that this test adjustment accurately reflects the intended behavior and does not mask potential issues with handling statement re-executions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the removed decode function with additional arguments. ast-grep --lang cpp --pattern $'decode<$_>($$$, $$$, $$$)'Length of output: 59
| MACRO(INSTALL_DEBUG_SYMBOLS target) | ||
| - IF(MSVC) | ||
| - GET_TARGET_PROPERTY(type ${target} TYPE) | ||
| - IF(NOT INSTALL_LOCATION) | ||
| - IF(type MATCHES "STATIC_LIBRARY" | ||
| - OR type MATCHES "MODULE_LIBRARY" | ||
| - OR type MATCHES "SHARED_LIBRARY") | ||
| - SET(INSTALL_LOCATION "lib") | ||
| - ELSEIF(type MATCHES "EXECUTABLE") | ||
| - SET(INSTALL_LOCATION "bin") | ||
| - ELSE() | ||
| - MESSAGE(FATAL_ERROR | ||
| - "cannot determine type of ${target}. Don't now where to install") | ||
| - ENDIF() | ||
| - ENDIF() | ||
| - | ||
| - IF(target STREQUAL "mysqld" OR target STREQUAL "mysqlbackup") | ||
| - SET(comp Server) | ||
| - ELSE() | ||
| - SET(comp Debuginfo) | ||
| - ENDIF() | ||
| - | ||
| - # No .pdb file for static libraries. | ||
| - IF(NOT type MATCHES "STATIC_LIBRARY") | ||
| - INSTALL(FILES $<TARGET_PDB_FILE:${target}> | ||
| - DESTINATION ${INSTALL_LOCATION} COMPONENT ${comp}) | ||
| - ENDIF() | ||
| - ENDIF() | ||
| +# .pdb files are too large to use in GitHub Actions. | ||
| +# so skip installing | ||
| ENDMACRO() |
There was a problem hiding this comment.
The modification to the INSTALL_DEBUG_SYMBOLS macro effectively disables the installation of .pdb files by commenting out the original logic and replacing it with a comment explaining the rationale. While this achieves the goal of not installing .pdb files, it removes the flexibility of installing debug symbols when needed. Consider adding a conditional check or a configuration option to allow users to opt-in for .pdb file installation if required for debugging purposes.
+# .pdb files are too large to use in GitHub Actions.
+# so skip installing
+IF(NOT DISABLE_PDB_INSTALLATION)
ENDMACRO()
+ENDIF()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| MACRO(INSTALL_DEBUG_SYMBOLS target) | |
| - IF(MSVC) | |
| - GET_TARGET_PROPERTY(type ${target} TYPE) | |
| - IF(NOT INSTALL_LOCATION) | |
| - IF(type MATCHES "STATIC_LIBRARY" | |
| - OR type MATCHES "MODULE_LIBRARY" | |
| - OR type MATCHES "SHARED_LIBRARY") | |
| - SET(INSTALL_LOCATION "lib") | |
| - ELSEIF(type MATCHES "EXECUTABLE") | |
| - SET(INSTALL_LOCATION "bin") | |
| - ELSE() | |
| - MESSAGE(FATAL_ERROR | |
| - "cannot determine type of ${target}. Don't now where to install") | |
| - ENDIF() | |
| - ENDIF() | |
| - | |
| - IF(target STREQUAL "mysqld" OR target STREQUAL "mysqlbackup") | |
| - SET(comp Server) | |
| - ELSE() | |
| - SET(comp Debuginfo) | |
| - ENDIF() | |
| - | |
| - # No .pdb file for static libraries. | |
| - IF(NOT type MATCHES "STATIC_LIBRARY") | |
| - INSTALL(FILES $<TARGET_PDB_FILE:${target}> | |
| - DESTINATION ${INSTALL_LOCATION} COMPONENT ${comp}) | |
| - ENDIF() | |
| - ENDIF() | |
| +# .pdb files are too large to use in GitHub Actions. | |
| +# so skip installing | |
| ENDMACRO() | |
| MACRO(INSTALL_DEBUG_SYMBOLS target) | |
| +# .pdb files are too large to use in GitHub Actions. | |
| +# so skip installing | |
| +IF(NOT DISABLE_PDB_INSTALLATION) | |
| ENDMACRO() | |
| +ENDIF() |
| - # For windows, install .pdb files for .exe and .dll files. | ||
| - IF(MSVC AND NOT target_type STREQUAL "STATIC_LIBRARY") | ||
| - GET_FILENAME_COMPONENT(ext ${debug_target_location} EXT) | ||
| - STRING(REPLACE "${ext}" ".pdb" | ||
| - debug_pdb_target_location "${debug_target_location}" ) | ||
| - IF (RENAME_PARAM) | ||
| - STRING(REPLACE "${ext}" ".pdb" pdb_rename "${ARG_RENAME}") | ||
| - SET(PDB_RENAME_PARAM RENAME "${pdb_rename}") | ||
| - ENDIF() | ||
| - | ||
| - INSTALL(FILES ${debug_pdb_target_location} | ||
| - DESTINATION ${ARG_DESTINATION} | ||
| - ${PDB_RENAME_PARAM} | ||
| - CONFIGURATIONS Release RelWithDebInfo | ||
| - COMPONENT ${ARG_COMPONENT} | ||
| - OPTIONAL) | ||
| - ENDIF() | ||
| ENDFUNCTION() |
There was a problem hiding this comment.
The changes in the INSTALL_DEBUG_TARGET function also remove the installation of .pdb files for .exe and .dll files. Similar to the previous comment, this change lacks flexibility for scenarios where debug symbols might be necessary. It's advisable to introduce a conditional check or configuration option here as well, to maintain the option of installing .pdb files when needed.
+# For windows, install .pdb files for .exe and .dll files.
+IF(NOT DISABLE_PDB_INSTALLATION)
IF(MSVC AND NOT target_type STREQUAL "STATIC_LIBRARY")
...
ENDIF()
+ENDIF()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - # For windows, install .pdb files for .exe and .dll files. | |
| - IF(MSVC AND NOT target_type STREQUAL "STATIC_LIBRARY") | |
| - GET_FILENAME_COMPONENT(ext ${debug_target_location} EXT) | |
| - STRING(REPLACE "${ext}" ".pdb" | |
| - debug_pdb_target_location "${debug_target_location}" ) | |
| - IF (RENAME_PARAM) | |
| - STRING(REPLACE "${ext}" ".pdb" pdb_rename "${ARG_RENAME}") | |
| - SET(PDB_RENAME_PARAM RENAME "${pdb_rename}") | |
| - ENDIF() | |
| - | |
| - INSTALL(FILES ${debug_pdb_target_location} | |
| - DESTINATION ${ARG_DESTINATION} | |
| - ${PDB_RENAME_PARAM} | |
| - CONFIGURATIONS Release RelWithDebInfo | |
| - COMPONENT ${ARG_COMPONENT} | |
| - OPTIONAL) | |
| - ENDIF() | |
| ENDFUNCTION() | |
| ENDFUNCTION() |
| - INSTALL(FILES $<TARGET_PDB_FILE:${NAME}> | ||
| - DESTINATION ${_option_DESTINATION} | ||
| - COMPONENT Router) |
There was a problem hiding this comment.
The removal of .pdb file installation for the Router component in the add_harness_plugin function follows the same pattern as the previous modifications. While it aligns with the objective of reducing the installation package size, consider providing a mechanism to optionally include these files for advanced debugging scenarios.
+IF(NOT DISABLE_PDB_INSTALLATION)
INSTALL(FILES $<TARGET_PDB_FILE:${NAME}>
DESTINATION ${_option_DESTINATION}
COMPONENT Router)
+ENDIF()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - INSTALL(FILES $<TARGET_PDB_FILE:${NAME}> | |
| - DESTINATION ${_option_DESTINATION} | |
| - COMPONENT Router) | |
| IF(NOT DISABLE_PDB_INSTALLATION) | |
| INSTALL(FILES $<TARGET_PDB_FILE:${NAME}> | |
| DESTINATION ${_option_DESTINATION} | |
| COMPONENT Router) | |
| ENDIF() |
| @@ -26,7 +26,6 @@ | ||
|
|
||
| #include "classic_connection_base.h" | ||
| #include "classic_frame.h" | ||
| -#include "hexify.h" | ||
| #include "mysql/harness/stdx/expected.h" | ||
| #include "mysql/harness/tls_error.h" | ||
| #include "mysqld_error.h" // mysql-server error-codes | ||
| @@ -63,51 +62,7 @@ StmtExecuteForwarder::process() { | ||
| stdx::expected<Processor::Result, std::error_code> | ||
| StmtExecuteForwarder::command() { | ||
| if (auto &tr = tracer()) { | ||
| - auto *socket_splicer = connection()->socket_splicer(); | ||
| - auto *src_channel = socket_splicer->client_channel(); | ||
| - auto *src_protocol = connection()->client_protocol(); | ||
| - | ||
| - auto msg_res = ClassicFrame::recv_msg< | ||
| - classic_protocol::borrowed::message::client::StmtExecute>(src_channel, | ||
| - src_protocol); | ||
| - if (!msg_res) { | ||
| - auto ec = msg_res.error(); | ||
| - | ||
| - // parse errors are invalid input. | ||
| - if (ec.category() == | ||
| - make_error_code(classic_protocol::codec_errc::invalid_input) | ||
| - .category()) { | ||
| - auto send_res = | ||
| - ClassicFrame::send_msg<classic_protocol::message::server::Error>( | ||
| - src_channel, src_protocol, | ||
| - {ER_MALFORMED_PACKET, "Malformed packet", "HY000"}); | ||
| - if (!send_res) return send_client_failed(send_res.error()); | ||
| - | ||
| - const auto &recv_buf = src_channel->recv_plain_view(); | ||
| - | ||
| - tr.trace(Tracer::Event().stage("stmt_execute::command:\n" + | ||
| - mysql_harness::hexify(recv_buf))); | ||
| - | ||
| - discard_current_msg(src_channel, src_protocol); | ||
| - | ||
| - stage(Stage::Done); | ||
| - return Result::SendToClient; | ||
| - } | ||
| - | ||
| - return recv_client_failed(msg_res.error()); | ||
| - } | ||
| - | ||
| - const auto &recv_buf = src_channel->recv_plain_view(); | ||
| - | ||
| - tr.trace(Tracer::Event().stage( | ||
| - "stmt_execute::command:\nstmt-id: " + // | ||
| - std::to_string(msg_res->statement_id()) + "\n" + // | ||
| - "flags: " + msg_res->flags().to_string() + "\n" + // | ||
| - "new-params-bound: " + std::to_string(msg_res->new_params_bound()) + | ||
| - "\n" + // | ||
| - "types::size(): " + std::to_string(msg_res->types().size()) + "\n" + | ||
| - "values::size(): " + std::to_string(msg_res->values().size()) + "\n" + | ||
| - mysql_harness::hexify(recv_buf))); | ||
| + tr.trace(Tracer::Event().stage("stmt_execute::command")); | ||
| } |
There was a problem hiding this comment.
In classic_stmt_execute_forwarder.cc, the detailed logging and error handling for StmtExecute command processing have been removed. This simplification could affect the ability to diagnose issues during statement execution forwarding. Consider whether additional logging or error handling might be necessary to maintain visibility and robustness in the forwarding process.
+IF(ENABLE_DETAILED_LOGGING)
tr.trace(Tracer::Event().stage("stmt_execute::command"));
+ENDIF()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @@ -26,7 +26,6 @@ | |
| #include "classic_connection_base.h" | |
| #include "classic_frame.h" | |
| -#include "hexify.h" | |
| #include "mysql/harness/stdx/expected.h" | |
| #include "mysql/harness/tls_error.h" | |
| #include "mysqld_error.h" // mysql-server error-codes | |
| @@ -63,51 +62,7 @@ StmtExecuteForwarder::process() { | |
| stdx::expected<Processor::Result, std::error_code> | |
| StmtExecuteForwarder::command() { | |
| if (auto &tr = tracer()) { | |
| - auto *socket_splicer = connection()->socket_splicer(); | |
| - auto *src_channel = socket_splicer->client_channel(); | |
| - auto *src_protocol = connection()->client_protocol(); | |
| - | |
| - auto msg_res = ClassicFrame::recv_msg< | |
| - classic_protocol::borrowed::message::client::StmtExecute>(src_channel, | |
| - src_protocol); | |
| - if (!msg_res) { | |
| - auto ec = msg_res.error(); | |
| - | |
| - // parse errors are invalid input. | |
| - if (ec.category() == | |
| - make_error_code(classic_protocol::codec_errc::invalid_input) | |
| - .category()) { | |
| - auto send_res = | |
| - ClassicFrame::send_msg<classic_protocol::message::server::Error>( | |
| - src_channel, src_protocol, | |
| - {ER_MALFORMED_PACKET, "Malformed packet", "HY000"}); | |
| - if (!send_res) return send_client_failed(send_res.error()); | |
| - | |
| - const auto &recv_buf = src_channel->recv_plain_view(); | |
| - | |
| - tr.trace(Tracer::Event().stage("stmt_execute::command:\n" + | |
| - mysql_harness::hexify(recv_buf))); | |
| - | |
| - discard_current_msg(src_channel, src_protocol); | |
| - | |
| - stage(Stage::Done); | |
| - return Result::SendToClient; | |
| - } | |
| - | |
| - return recv_client_failed(msg_res.error()); | |
| - } | |
| - | |
| - const auto &recv_buf = src_channel->recv_plain_view(); | |
| - | |
| - tr.trace(Tracer::Event().stage( | |
| - "stmt_execute::command:\nstmt-id: " + // | |
| - std::to_string(msg_res->statement_id()) + "\n" + // | |
| - "flags: " + msg_res->flags().to_string() + "\n" + // | |
| - "new-params-bound: " + std::to_string(msg_res->new_params_bound()) + | |
| - "\n" + // | |
| - "types::size(): " + std::to_string(msg_res->types().size()) + "\n" + | |
| - "values::size(): " + std::to_string(msg_res->values().size()) + "\n" + | |
| - mysql_harness::hexify(recv_buf))); | |
| + tr.trace(Tracer::Event().stage("stmt_execute::command")); | |
| } | |
| @@ -26,7 +26,6 @@ | |
| #include "classic_connection_base.h" | |
| #include "classic_frame.h" | |
| -#include "hexify.h" | |
| #include "mysql/harness/stdx/expected.h" | |
| #include "mysql/harness/tls_error.h" | |
| #include "mysqld_error.h" // mysql-server error-codes | |
| @@ -63,51 +62,7 @@ StmtExecuteForwarder::process() { | |
| stdx::expected<Processor::Result, std::error_code> | |
| StmtExecuteForwarder::command() { | |
| if (auto &tr = tracer()) { | |
| +IF(ENABLE_DETAILED_LOGGING) | |
| tr.trace(Tracer::Event().stage("stmt_execute::command")); | |
| +ENDIF() | |
| } |
Summary by CodeRabbit