Conversation
This is the final PR Bugbot will review for you during this billing cycleYour free Bugbot reviews will reset on November 19 DetailsYou are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle. To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Bug: Memory Leak and Dangling Pointers in Argument HandlingThe |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors build system to use ELOQSQL_SOURCE_DIR / ELOQSQL_BINARY_DIR, moves several submodules from Changes
Sequence Diagram(s)sequenceDiagram
participant CMake as CMake
participant Src as ELOQSQL_SOURCE_DIR
participant Bin as ELOQSQL_BINARY_DIR
participant DataSub as data_substrate (optional)
Note over CMake,Src: Before change (conceptual)
CMake->>Src: use CMAKE_SOURCE_DIR
CMake->>Bin: use CMAKE_BINARY_DIR
Note over CMake,Src: After change
CMake->>Src: use ELOQSQL_SOURCE_DIR
CMake->>Bin: use ELOQSQL_BINARY_DIR
alt BUILD_ELOQSQL_AS_LIBRARY=ON
CMake->>DataSub: link against data_substrate (converged)
else
CMake->>DataSub: add_subdirectory data_substrate (standalone)
end
sequenceDiagram
participant App as mysqld
participant Flags as gflags
participant DataSub as DataSubstrate (if converged)
App->>Flags: Parse --eloqsql_config / flags
alt MYSQLD_LIBRARY_MODE defined
Flags->>App: strict parse (unknown flags => error)
App->>DataSub: wait for DataSubstrate::GetGlobal() init (converged flow)
else
Flags->>App: permissive parse (undefok="*")
App->>App: legacy startup flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing focused review:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
storage/mroonga/CMakeLists.txt (1)
23-27: Avoid hard dependency on ELOQSQL_SOURCE_DIR for non-bundled builds; fix MRN_SOURCE_DIR.If ELOQSQL_SOURCE_DIR is undefined (standalone build), current logic will mis-detect bundling and resolve paths incorrectly. Add a fallback and use CMAKE_CURRENT_SOURCE_DIR for the non-bundled MRN_SOURCE_DIR.
Apply:
@@ -project(mroonga) +project(mroonga) +# Fallback for standalone builds where ELOQSQL_SOURCE_DIR isn't defined +if(NOT DEFINED ELOQSQL_SOURCE_DIR) + set(ELOQSQL_SOURCE_DIR ${CMAKE_SOURCE_DIR}) +endif() @@ -if("${ELOQSQL_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}") +if("${ELOQSQL_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}") set(MRN_BUNDLED FALSE) else() set(MRN_BUNDLED TRUE) endif() @@ -if(MRN_BUNDLED) - set(MRN_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}/storage/mroonga) -else() - set(MRN_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}) -endif() +if(MRN_BUNDLED) + set(MRN_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}/storage/mroonga) +else() + # Standalone: mroonga sources are in the current project + set(MRN_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}) +endif()Also applies to: 77-81
storage/eloq/eloq_i_s.cc (1)
65-93: Possible nullptr dereference of storage_hd. Add a guard.
storage_hd->DiscoverAllTableNames(...)is called without checkingstorage_hd. Add a fast-fail with logging to avoid crashes when the service isn’t initialized.@@ int ret= 0; auto [yield_func, resume_func]= thd_get_coro_functors(thd); std::vector<std::string> table_names; - bool ok= - storage_hd->DiscoverAllTableNames(table_names, yield_func, resume_func); + if (storage_hd == nullptr) { + ret = HA_ERR_INTERNAL_ERROR; + sql_print_error("DataStoreHandler not initialized (storage_hd is null)."); + DBUG_RETURN(ret); + } + bool ok = storage_hd->DiscoverAllTableNames(table_names, yield_func, resume_func);sql/CMakeLists.txt (1)
67-90: Fix Linux kernel parsing and minor-version check (currently broken).
- string(REGEX MATCH) uses the output var incorrectly; CMAKE_MATCH_* won’t be set reliably.
- Minor check mistakenly tests LINUX_KERNEL_MAJOR_VERSION twice.
Apply:
- string(REGEX MATCH "([0-9]+)\\.([0-9]+)\\..*" ${LINUX_KERNEL_VERSION} ${LINUX_KERNEL_VERSION}) - set(LINUX_KERNEL_MAJOR_VERSION ${CMAKE_MATCH_1}) - set(LINUX_KERNEL_MINOR_VERSION ${CMAKE_MATCH_2}) + string(REGEX MATCH "([0-9]+)\\.([0-9]+)\\..*" _KVER "${LINUX_KERNEL_VERSION}") + set(LINUX_KERNEL_MAJOR_VERSION "${CMAKE_MATCH_1}") + set(LINUX_KERNEL_MINOR_VERSION "${CMAKE_MATCH_2}") @@ - if (${LINUX_KERNEL_MAJOR_VERSION} MATCHES "([0-9]+)") + if ("${LINUX_KERNEL_MAJOR_VERSION}" MATCHES "^[0-9]+$") @@ - if (${LINUX_KERNEL_MAJOR_VERSION} MATCHES "([0-9]+)") + if ("${LINUX_KERNEL_MINOR_VERSION}" MATCHES "^[0-9]+$")storage/eloq/ha_eloq.cc (1)
1496-1503: Guard possible null eloq_insert_semantic.eloq_insert_semantic is a char* and may be null; constructing std::string from null is UB.
- std::string insert_semantic_str= eloq_insert_semantic; + std::string insert_semantic_str = eloq_insert_semantic ? eloq_insert_semantic : "insert";storage/eloq/eloq_db_dl.h (4)
28-31: Guard ELOQ_ENGINE_NAME (uses STRING_WITH_LEN) to avoid build-time dependency in static modeWith dynamic path disabled, this initializer can fail if STRING_WITH_LEN isn’t visible. Guard it with the same switch or move it into the dynamic branch.
-#define plugin_handle(pi) ((plugin_dlib(pi))->handle) - -static LEX_CSTRING ELOQ_ENGINE_NAME= {STRING_WITH_LEN("ELOQ")}; +#if !defined(ELOQ_LINK_STATIC) +#define plugin_handle(pi) ((plugin_dlib(pi))->handle) +static LEX_CSTRING ELOQ_ENGINE_NAME= {STRING_WITH_LEN("ELOQ")}; +#endif
20-27: Missing fwd-decls and uchar definition/includeDiscovered_table_list and File_option are used later but never declared; uchar appears without a guarantee of definition. Add lightweight fwd-decls and ensure uchar is defined.
#include <string> +// Ensure uchar and common MySQL/MariaDB typedefs +#include <my_global.h> + +// Lightweight forward declarations for pointer/reference usage +struct Discovered_table_list; +struct File_option;If including my_global.h is undesirable here, alternatively add a local fallback alias guarded by a project macro you already use for this purpose.
151-153: Forward-declare Discovered_table_listUsed by reference; add a forward declaration near the top to prevent incomplete-type errors on some compilers/include orders.
- MONOCALL_DEFINE2(eloq_discover_database_names_wild, THD *, thd, - Discovered_table_list &, tl); + MONOCALL_DEFINE2(eloq_discover_database_names_wild, THD *, thd, + Discovered_table_list &, tl); + // Note: requires `struct Discovered_table_list;` forward-declared above.
161-165: Forward-declare File_option and ensure uchar is availableBoth appear here without local guarantees. Add
struct File_option;and make uchar available via <my_global.h> (or a guarded typedef).- MONOCALL_DEFINE6(eloq_upsert_view, THD *, thd, LEX_CSTRING, db, LEX_CSTRING, - view, const LEX_CSTRING *, type, uchar *, base, File_option *, - parameter); + MONOCALL_DEFINE6(eloq_upsert_view, THD *, thd, LEX_CSTRING, db, LEX_CSTRING, + view, const LEX_CSTRING *, type, uchar *, base, File_option *, + parameter); // needs `struct File_option;` forward-declared
🧹 Nitpick comments (21)
storage/eloq/eloq_db.cpp (1)
590-610: Consider log level and potential verbosity.The added logging provides useful operational tracing for the view fetch path selection logic. However, LOG(INFO) may generate excessive output in production environments, especially if this is a frequently executed path.
Consider using a DEBUG or TRACE level for these detailed operational logs to reduce production verbosity:
- LOG(INFO) << "load_view_fetch_func " << ok << " exists: " << exists << " version_ts: " << version_ts; + LOG(DEBUG) << "load_view_fetch_func " << ok << " exists: " << exists << " version_ts: " << version_ts;Apply similar changes to lines 596, 602, and 609.
storage/innobase/innodb.cmake (1)
129-130: Path variable update looks correct.The replacement of
CMAKE_SOURCE_DIRwithELOQSQL_SOURCE_DIRaligns with the broader refactoring for library-mode compilation support. The paths are resolved correctly.Note: There are duplicate
INCLUDE_DIRECTORIESdeclarations at lines 129-130 and 153-155. While this duplication predates your changes, consider consolidating them in a future refactoring.Also applies to: 153-155
storage/mroonga/CMakeLists.txt (1)
170-174: Use find_program for MYSQL_CONFIG (not find_path) in bundled branch.find_path on a full file path can yield unexpected results. Prefer find_program to resolve the binary.
-if(MRN_BUNDLED) - set(MYSQL_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}) - set(MYSQL_BUILD_DIR ${MYSQL_SOURCE_DIR}) - set(MYSQL_CONFIG ${ELOQSQL_SOURCE_DIR}/scripts/mysql_config) +if(MRN_BUNDLED) + set(MYSQL_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}) + set(MYSQL_BUILD_DIR ${MYSQL_SOURCE_DIR}) + find_program(MYSQL_CONFIG mysql_config HINTS ${ELOQSQL_SOURCE_DIR}/scripts NO_DEFAULT_PATH) @@ -find_path(MYSQL_CONFIG "${MYSQL_CONFIG}") +# keep MYSQL_CONFIG as a full path to the executable; no find_path heresql/circular_queue.h (5)
212-216: Guard Peek() against empty queue (debug).Avoid UB when empty.
- T &Peek() + T &Peek() { + assert(cnt_ > 0 && "Peek on empty CircularQueue"); return vec_[head_]; }
227-231: Fix MemUsage to account for element size.Using sizeof(uint64_t) undercounts for larger T and overcounts for smaller T.
- size_t MemUsage() const + size_t MemUsage() const { - return (sizeof(CircularQueue) + capacity_ * sizeof(uint64_t)); + return sizeof(*this) + capacity_ * sizeof(T); }
232-235: Const-correctness and bounds.Returning T& from a const method breaks const-correctness and there’s no bounds check.
- T &Get(size_t index) const + const T &Get(size_t index) const { - return vec_[(head_ + index) % capacity_]; + assert(index < cnt_ && "Get index out of range"); + return vec_[(head_ + index) % capacity_]; }Add a non-const overload:
// place below the const overload T &Get(size_t index) { assert(index < cnt_ && "Get index out of range"); return vec_[(head_ + index) % capacity_]; }
39-46: Declare move operations noexcept.Helps composition and enables stronger guarantees in containers.
- CircularQueue(CircularQueue &&rhs) + CircularQueue(CircularQueue &&rhs) noexcept @@ - CircularQueue &operator=(CircularQueue &&rhs) + CircularQueue &operator=(CircularQueue &&rhs) noexceptAlso applies to: 47-57
199-210: Optional: consider clearing slots on Dequeue for resource-heavy T.If T holds resources, leaving the old slot alive can retain memory. If acceptable, ignore; otherwise, consider assigning a default-constructed T when popping (requires default-constructible T) or switch to manual storage/placement-new model.
Confirm expected T constraints (trivially movable? default-constructible?) across usages to decide.
scripts/mysql_install_db.sh (1)
544-544: Address POSIX compatibility warnings.Static analysis flags two issues:
- The
localkeyword is not POSIX-compliant (bash-specific)- Variables should be declared and assigned separately to avoid masking return values
For maximum portability, consider this refactor:
- local temp_config=$(mktemp) + temp_config="" + temp_config=$(mktemp)and
- local exit_code=$? + exit_code=$?However, since this script likely requires bash anyway (due to other constructs), these warnings can be safely ignored if bash is the intended shell.
Also applies to: 559-559
cmake/info_macros.cmake.in (1)
29-31: ELOQSQL_ variable assumptions. Add safe fallbacks.*If top-level configuration forgets to set these, macro execution will fail. Provide conservative fallbacks to the standard CMake vars.
# Fallbacks if configured values are empty IF(NOT ELOQSQL_SOURCE_DIR) SET(ELOQSQL_SOURCE_DIR "${CMAKE_SOURCE_DIR}") ENDIF() IF(NOT ELOQSQL_BINARY_DIR) SET(ELOQSQL_BINARY_DIR "${CMAKE_BINARY_DIR}") ENDIF()cmake/dtrace.cmake (2)
74-74: Use PERL_EXECUTABLE and quote paths for portability.Hardcoding
perlcan fail on systems without it in PATH. Prefer${PERL_EXECUTABLE}and quote script path.find_package(Perl REQUIRED) COMMAND "${PERL_EXECUTABLE}" "${ELOQSQL_SOURCE_DIR}/scripts/dheadgen.pl" -f ${provider} > ${header_no_dtrace}
131-136: Single shared outfile can collide across targets/configs.
${ELOQSQL_BINARY_DIR}/probes_mysql.ois global. Parallel builds or multi-config generators may clobber. Prefer per-target (and per-config) location.# Per-target, per-config object set(outfile "${CMAKE_CURRENT_BINARY_DIR}/${target}_$<CONFIG>_probes_mysql.o") execute_process( COMMAND ${DTRACE} -G -s ${ELOQSQL_SOURCE_DIR}/include/probes_mysql.d.base -o ${outfile} )cmake/make_dist.cmake.in (1)
42-76: Path substitutions look correct; consider quoting for spaces.All ELOQSQL_* path rewrites look consistent. For robustness on paths with spaces, quote variable expansions in EXECUTE_PROCESS/CONFIGURE_FILE.
Also applies to: 78-83, 127-136, 145-146
sql/mysqld.cc (3)
5528-5531: Expose shutdown API: consider C linkage if used cross‑langFunction is fine. If called from C or dynamic loaders, expose with
extern "C"and add to a public header.-void shutdown_mysqld() +extern "C" void shutdown_mysqld() { break_connect_loop(); }
5565-5614: Make argv augmentation robust and portable
- This block depends on gflags; guard it to avoid builds without gflags failing.
- Avoid
const_cast<char*>onstd::string::c_str(). Lifetime is okay here, but casting away const is fragile; pass a writable buffer or keep guarded.Apply:
- // Build argv for load_defaults + // Build argv for load_defaults +#ifdef WITH_GFLAGS // Copy all original arguments first, then append --eloqsql_config and --bootstrap if needed int additional_args = 0; if (!FLAGS_eloqsql_config.empty()) additional_args++; if (FLAGS_bootstrap) additional_args++; @@ - std::string defaults_file_arg; + std::string defaults_file_arg; if (!FLAGS_eloqsql_config.empty()) { defaults_file_arg = std::string("--defaults-file=") + FLAGS_eloqsql_config; - load_default_argv[load_default_argc] = const_cast<char*>(defaults_file_arg.c_str()); + // Safe: load_defaults_or_exit consumes argv immediately; avoid const_cast by using a writable buffer + load_default_argv[load_default_argc] = const_cast<char*>(defaults_file_arg.data()); load_default_argc++; } @@ if (FLAGS_bootstrap) { load_default_argv[load_default_argc] = const_cast<char*>("--bootstrap"); load_default_argc++; } +#endifNote: using
std::string::data()(C++17) yields a non‑const pointer to the internal buffer and avoids writing. If portability requires, keep as is; the main point is guarding the whole block withWITH_GFLAGS.
7841-7841: Prefer guarded logging or core logger; also fix minor phrasing
- Guard LOG(INFO) with WITH_GLOG or use
sql_print_informationfor consistency with existing logging.- Message text is fine here.
- LOG(INFO) << "default mysql data home " << mysql_real_data_home; +#ifdef WITH_GLOG + LOG(INFO) << "default mysql data home " << mysql_real_data_home; +#else + sql_print_information("default mysql data home %s", mysql_real_data_home); +#endifsql/CMakeLists.txt (1)
95-101: URING package name may differ across environments.Many distros expose liburing via PkgConfig or Findliburing. Add a fallback:
- find_package(URING REQUIRED) + find_package(URING QUIET) + if(NOT URING_FOUND) + find_package(PkgConfig REQUIRED) + pkg_check_modules(LIBURING REQUIRED IMPORTED_TARGET liburing) + set(URING_LIBRARIES PkgConfig::LIBURING) + endif()storage/eloq/CMakeLists.txt (1)
18-23: Minor: message formatting.Prefer STATUS and a single string.
-message(NOTICE "WITH_DATA_STORE" : ${WITH_DATA_STORE}) +message(STATUS "WITH_DATA_STORE: ${WITH_DATA_STORE}")storage/eloq/ha_eloq.cc (1)
1572-1576: Shutdown symmetry between modes.In standalone you shutdown DataSubstrate; in library mode you don’t. Ensure the host process is responsible for orderly shutdown; otherwise leaks persist across plugin unload.
storage/eloq/eloq_db_dl.h (2)
56-80: Replace commented-out build switch with real #if or delete dead codeLarge commented regions are fragile. Either keep both paths behind a proper preprocessor switch or remove the dynamic path entirely.
Minimal guard to retain both modes:
-// #if defined(ELOQ_LINK_STATIC) -// Statically linked: declare the symbol and let the linker resolve it to the -// implementation provided in the static library (see eloq_db.cpp extern "C" block). -#define MONOCALL_DEFINEx(x, name, ...) \ - extern "C" int name(__SC_DECL##x(__VA_ARGS__)); -// #else -// #define MONOCALL_DEFINEx(x, name, ...) \ -// static inline int name(__SC_DECL##x(__VA_ARGS__)) \ -// { \ -// using T= decltype(&name); \ -// int ret= 0; \ -// static_assert(std::is_same<decltype(__ARG1(__VA_ARGS__)), THD *>::value); \ -// static THD *t= __ARG1(__VA_ARGS__); \ -// static plugin_ref pi= ha_resolve_by_name(t, &ELOQ_ENGINE_NAME, false); \ -// static void *handle= plugin_handle(pi); \ -// static T fp= reinterpret_cast<T>(dlsym(handle, __FUNCTION__)); \ -// ret= fp(__SC_CAST##x(__VA_ARGS__)); \ -// return ret; \ -// } -// #endif +#if defined(ELOQ_LINK_STATIC) + // Statically linked: let the linker resolve to implementation. + #define MONOCALL_DEFINEx(x, name, ...) \ + extern "C" int name(__SC_DECL##x(__VA_ARGS__)); +#else + #define MONOCALL_DEFINEx(x, name, ...) \ + static inline int name(__SC_DECL##x(__VA_ARGS__)) \ + { \ + using T= decltype(&name); \ + int ret= 0; \ + static_assert(std::is_same<decltype(__ARG1(__VA_ARGS__)), THD *>::value, \ + "THD* must be first"); \ + THD *t= __ARG1(__VA_ARGS__); \ + plugin_ref pi= ha_resolve_by_name(t, &ELOQ_ENGINE_NAME, false); \ + void *handle= plugin_handle(pi); \ + T fp= reinterpret_cast<T>(dlsym(handle, __FUNCTION__)); \ + ret= fp(__SC_CAST##x(__VA_ARGS__)); \ + return ret; \ + } +#endif
20-20: Minor: drop unused includeNot used in this header; remove to cut compile time.
-#include <string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (106)
-
.gitignore(1 hunks) -
.gitmodules(1 hunks) -
CMakeLists.txt(8 hunks) -
client/CMakeLists.txt(3 hunks) -
cmake/abi_check.cmake(1 hunks) -
cmake/build_configurations/mysql_release.cmake(1 hunks) -
cmake/cpack_deb.cmake(1 hunks) -
cmake/cpack_rpm.cmake(3 hunks) -
cmake/ctest.cmake(1 hunks) -
cmake/dtrace.cmake(4 hunks) -
cmake/info_macros.cmake.in(4 hunks) -
cmake/install_macros.cmake(2 hunks) -
cmake/libutils.cmake(1 hunks) -
cmake/make_dist.cmake.in(4 hunks) -
cmake/mysql_add_executable.cmake(1 hunks) -
cmake/mysql_version.cmake(2 hunks) -
cmake/pcre.cmake(1 hunks) -
cmake/plugin.cmake(4 hunks) -
cmake/readline.cmake(1 hunks) -
cmake/sign.cmake.in(1 hunks) -
cmake/ssl.cmake(1 hunks) -
cmake/submodules.cmake(2 hunks) -
cmake/tags.cmake(1 hunks) -
cmake/wsrep.cmake(2 hunks) -
cmake/zlib.cmake(1 hunks) -
configure.cmake(2 hunks) -
data_substrate(1 hunks) -
dbug/CMakeLists.txt(1 hunks) -
extra/CMakeLists.txt(3 hunks) -
extra/mariabackup/CMakeLists.txt(1 hunks) -
extra/readline/CMakeLists.txt(1 hunks) -
libmysqld/CMakeLists.txt(2 hunks) -
libmysqld/examples/CMakeLists.txt(1 hunks) -
libservices/CMakeLists.txt(1 hunks) -
mysql-test/CMakeLists.txt(2 hunks) -
mysql-test/lib/My/SafeProcess/CMakeLists.txt(1 hunks) -
mysql-test/mtr.out-of-source(1 hunks) -
mysys/CMakeLists.txt(1 hunks) -
mysys_ssl/CMakeLists.txt(1 hunks) -
plugin/auth_dialog/CMakeLists.txt(1 hunks) -
plugin/disks/CMakeLists.txt(1 hunks) -
plugin/feedback/CMakeLists.txt(1 hunks) -
plugin/file_key_management/CMakeLists.txt(1 hunks) -
plugin/locale_info/CMakeLists.txt(1 hunks) -
plugin/qc_info/CMakeLists.txt(1 hunks) -
plugin/type_geom/CMakeLists.txt(1 hunks) -
plugin/userstat/CMakeLists.txt(1 hunks) -
plugin/wsrep_info/CMakeLists.txt(1 hunks) -
scripts/CMakeLists.txt(3 hunks) -
scripts/mysql_install_db.sh(1 hunks) -
scripts/sys_schema/CMakeLists.txt(1 hunks) -
sql/CMakeLists.txt(10 hunks) -
sql/circular_queue.h(1 hunks) -
sql/glog_error_logging.cc(1 hunks) -
sql/main.cc(2 hunks) -
sql/mdl.h(1 hunks) -
sql/mysql_metrics.h(1 hunks) -
sql/mysqld.cc(10 hunks) -
sql/sql_class.h(1 hunks) -
sql/sql_parse.cc(1 hunks) -
sql/sql_select.cc(1 hunks) -
sql/threadpool_generic.h(1 hunks) -
storage/eloq/CMakeLists.txt(3 hunks) -
storage/eloq/build_eloq_log_service.cmake(0 hunks) -
storage/eloq/build_eloq_metrics.cmake(0 hunks) -
storage/eloq/build_log_service.cmake(0 hunks) -
storage/eloq/build_tx_service.cmake(0 hunks) -
storage/eloq/eloq_db.cpp(3 hunks) -
storage/eloq/eloq_db_dl.h(1 hunks) -
storage/eloq/eloq_i_s.cc(1 hunks) -
storage/eloq/eloq_key_def.cc(2 hunks) -
storage/eloq/eloq_key_def.h(1 hunks) -
storage/eloq/eloq_metrics(0 hunks) -
storage/eloq/eloq_system_handler.h(1 hunks) -
storage/eloq/eloqsql_catalog_factory.cpp(2 hunks) -
storage/eloq/eloqsql_catalog_factory.h(1 hunks) -
storage/eloq/eloqsql_schema.cpp(1 hunks) -
storage/eloq/ha_eloq.cc(14 hunks) -
storage/eloq/ha_eloq.h(2 hunks) -
storage/eloq/log_service(0 hunks) -
storage/eloq/metrics_registry_impl.cpp(0 hunks) -
storage/eloq/metrics_registry_impl.h(0 hunks) -
storage/eloq/store_handler(0 hunks) -
storage/eloq/tx_service(0 hunks) -
storage/innobase/CMakeLists.txt(1 hunks) -
storage/innobase/innodb.cmake(2 hunks) -
storage/innobase/unittest/CMakeLists.txt(1 hunks) -
storage/maria/unittest/CMakeLists.txt(1 hunks) -
storage/mroonga/CMakeLists.txt(4 hunks) -
storage/mroonga/vendor/groonga/CMakeLists.txt(1 hunks) -
storage/mroonga/vendor/groonga/vendor/plugins/groonga-normalizer-mysql/CMakeLists.txt(2 hunks) -
storage/perfschema/CMakeLists.txt(1 hunks) -
storage/perfschema/unittest/CMakeLists.txt(1 hunks) -
storage/rocksdb/unittest/CMakeLists.txt(1 hunks) -
storage/spider/CMakeLists.txt(1 hunks) -
strings/CMakeLists.txt(1 hunks) -
tests/CMakeLists.txt(1 hunks) -
unittest/embedded/CMakeLists.txt(1 hunks) -
unittest/json_lib/CMakeLists.txt(1 hunks) -
unittest/my_decimal/CMakeLists.txt(1 hunks) -
unittest/mytap/CMakeLists.txt(1 hunks) -
unittest/sql/CMakeLists.txt(1 hunks) -
vio/CMakeLists.txt(1 hunks) -
win/packaging/CMakeLists.txt(3 hunks) -
win/packaging/ca/CMakeLists.txt(1 hunks) -
win/upgrade_wizard/CMakeLists.txt(2 hunks)
💤 Files with no reviewable changes (10)
- storage/eloq/build_eloq_metrics.cmake
- storage/eloq/tx_service
- storage/eloq/log_service
- storage/eloq/build_eloq_log_service.cmake
- storage/eloq/metrics_registry_impl.cpp
- storage/eloq/build_log_service.cmake
- storage/eloq/store_handler
- storage/eloq/build_tx_service.cmake
- storage/eloq/eloq_metrics
- storage/eloq/metrics_registry_impl.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T11:41:40.111Z
Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
Applied to files:
storage/eloq/CMakeLists.txt
🧬 Code graph analysis (1)
sql/mysqld.cc (1)
sql/sql_parse.cc (2)
bootstrap(982-1112)bootstrap(982-982)
🪛 Shellcheck (0.11.0)
scripts/mysql_install_db.sh
[warning] 544-544: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 544-544: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 544-544: Quote this to prevent word splitting.
(SC2046)
[warning] 559-559: In POSIX sh, 'local' is undefined.
(SC3043)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (98)
cmake/cpack_deb.cmake (1)
15-17: LGTM! Variable rename aligns with CMake best practices.The change from
CMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRis appropriate for supporting both standalone and library-mode builds. Verification confirms thatELOQSQL_SOURCE_DIRis properly defined inCMakeLists.txt:47and used consistently throughout the codebase. This prevents conflicts when the project is included as a subproject in another CMake build.cmake/libutils.cmake (1)
234-234: No issues found. The code change is correct and properly integrated.ELOQSQL_BINARY_DIR is properly defined in the top-level CMakeLists.txt (line 48) as
${CMAKE_CURRENT_BINARY_DIR}before cmake/libutils.cmake is included. The merge_archives_unix.cmake script correctly receives and uses the TOP_DIR parameter at lines 6876 and 6880. All references align consistently with the ELOQSQL_* naming convention used throughout the project.cmake/wsrep.cmake (2)
59-60: LGTM with verification.The include directory paths have been updated consistently with the same variable change. Assuming
ELOQSQL_SOURCE_DIRis properly defined (as requested above), these include paths will resolve correctly.
36-43: ELOQSQL_SOURCE_DIR is properly defined and correctly applied.Verification confirms:
ELOQSQL_SOURCE_DIRis defined inCMakeLists.txt:47viaSET(ELOQSQL_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})cmake/wsrep.cmakeis included atCMakeLists.txt:271viaINCLUDE(wsrep), which occurs after the variable definition- All 4 changed lines (36, 43, 59–60) correctly use
ELOQSQL_SOURCE_DIRand will resolve properly- The
wsrep-libdirectory structure exists and is valid- Variable scope is correct; the variable is available in the included file
The changes are correct and consistent.
data_substrate (1)
1-1: The submodule reference update aligns with the PR's restructuring but should be manually verified for content integrity.The gitmodules configuration shows that submodules have been intentionally reorganized under the
data_substrate/directory (eloq_metrics, tx_service, store_handler, log_service), which aligns with the PR's stated refactoring objectives. The commit hash09b423ebb72f83d9013116bf7b7b591a7105692cis present in HEAD and appears to be part of this intentional restructuring.However, due to sandbox limitations, we cannot inspect the actual commit contents to verify there are no unintended breaking changes. Please manually verify that the submodule commit contains only the expected changes related to the directory restructuring and migration goals, and does not introduce incompatible API or configuration changes.
plugin/qc_info/CMakeLists.txt (1)
1-1: Consistent with project-wide refactoring pattern.This aligns with the systematic rename from
CMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRobserved across the codebase. The change is straightforward and isolated.Ensure the same verification applies—
ELOQSQL_SOURCE_DIRmust be defined before CMake processes this file.plugin/file_key_management/CMakeLists.txt (1)
3-3: LGTM.Consistent with the project-wide directory variable refactoring. No issues identified.
unittest/embedded/CMakeLists.txt (1)
2-5: LGTM.All include directory references properly updated to use
ELOQSQL_SOURCE_DIR. The refactoring is consistent with other files.plugin/type_geom/CMakeLists.txt (1)
1-1: LGTM.Consistent with the project-wide directory variable refactoring.
plugin/feedback/CMakeLists.txt (1)
1-1: LGTM.Consistent variable refactoring. No functional changes.
unittest/json_lib/CMakeLists.txt (1)
16-19: LGTM.All include directory paths consistently updated to use
ELOQSQL_SOURCE_DIRacross the multi-line block.storage/eloq/eloq_key_def.h (1)
59-59: No issues found with the header rename.The verification confirms that
eloqsql_schema.hexists atstorage/eloq/eloqsql_schema.hand the old header has been removed as part of the refactoring. No stray references remain in this file. The header rename is valid and will not cause build failures.mysys_ssl/CMakeLists.txt (1)
16-17: ELOQSQL_SOURCE_DIR is properly defined and the refactoring is safe.Verification confirms that
ELOQSQL_SOURCE_DIRis defined at line 47 in the top-level CMakeLists.txt asSET(ELOQSQL_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})and is used throughout the file in subsequent lines. Since CMake processes the top-level file before including subdirectories, the variable will be available when mysys_ssl/CMakeLists.txt is included. The refactoring is correct.strings/CMakeLists.txt (1)
16-16: LGTM! Directory variable updated correctly.The change from
CMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRaligns with the project-wide refactoring to support the new library-mode build system.plugin/userstat/CMakeLists.txt (1)
1-1: LGTM! Include path updated correctly.The migration to
ELOQSQL_SOURCE_DIRis consistent with the broader build system refactoring.cmake/sign.cmake.in (1)
16-16: LGTM! Binary directory variable updated correctly.The change from
@CMAKE_BINARY_DIR@to@ELOQSQL_BINARY_DIR@in this template file is consistent with the project-wide migration to ELOQSQL-specific directory variables.plugin/locale_info/CMakeLists.txt (1)
1-1: LGTM! Include paths updated correctly.Both sql and regex include directories correctly migrated to use
ELOQSQL_SOURCE_DIR.storage/maria/unittest/CMakeLists.txt (1)
16-17: LGTM! Test include directories updated correctly.All three include paths (include, zlib, unittest/mytap) correctly migrated to use
ELOQSQL_SOURCE_DIR.storage/mroonga/vendor/groonga/CMakeLists.txt (1)
24-28: LGTM! Bundling detection updated for new build structure.The condition now correctly determines bundling status relative to
ELOQSQL_SOURCE_DIRrather than the genericCMAKE_SOURCE_DIR, which aligns with the new library-mode build architecture where EloqSQL is the containing project..gitignore (1)
547-547: LGTM! Cursor IDE directory added to ignore list.Appropriately placed in the Visual Studio Code workspace section alongside the existing
.vscode/entry.cmake/ctest.cmake (1)
11-12: LGTM! Test macro include paths updated correctly.The
MY_ADD_TESTSmacro now correctly usesELOQSQL_SOURCE_DIRfor test include directory resolution, consistent with the broader build system migration.libmysqld/examples/CMakeLists.txt (1)
16-19: LGTM! Consistent path variable migration.The change from
CMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRaligns with the project-wide refactoring to support library/converged build modes.unittest/mytap/CMakeLists.txt (1)
16-16: LGTM! Consistent with project-wide path refactoring.The migration to
ELOQSQL_SOURCE_DIRis consistent with the broader build system changes.extra/readline/CMakeLists.txt (1)
16-16: LGTM! Consistent path migration.The change aligns with the project-wide migration to
ELOQSQL_SOURCE_DIR.extra/mariabackup/CMakeLists.txt (1)
32-33: LGTM! Consistent path variable update.The migration to
ELOQSQL_SOURCE_DIRfollows the same pattern as other CMake files in this PR.plugin/disks/CMakeLists.txt (1)
22-22: LGTM! Path variable updated consistently.The change to
ELOQSQL_SOURCE_DIRis consistent with the project-wide refactoring.storage/eloq/eloq_key_def.cc (1)
36-36: LGTM! Header includes updated consistently.The header includes have been updated to use the renamed
eloqsql_schema.handeloqsql_key.hfiles, consistent with the broader header renaming effort in this PR.Also applies to: 67-67
storage/eloq/eloqsql_schema.cpp (1)
25-27: Header file renames verified as complete.The renamed headers (
eloqsql_schema.handeloqsql_key.h) exist and are used consistently across the codebase. No remaining references to the old names (eloq_schema.handeloq_key.h) were found. The migration is complete.plugin/wsrep_info/CMakeLists.txt (1)
2-3: Path variable update is correct.The replacement of
CMAKE_SOURCE_DIRwithELOQSQL_SOURCE_DIRis consistent with the project-wide refactoring.storage/innobase/CMakeLists.txt (1)
403-403: Path variable updates are correct.The
ADD_SUBDIRECTORYcall correctly uses bothELOQSQL_SOURCE_DIRfor the source path andELOQSQL_BINARY_DIRfor the binary output path, which is the proper pattern for out-of-tree subdirectory builds.mysql-test/lib/My/SafeProcess/CMakeLists.txt (1)
17-17: Path variable updates are correct.The replacements consistently use
ELOQSQL_SOURCE_DIRfor both global and target-specific include directories, maintaining proper scoping.Also applies to: 22-22
libservices/CMakeLists.txt (1)
16-16: Path variable update is correct.The replacement is straightforward and aligns with the project-wide refactoring.
dbug/CMakeLists.txt (1)
17-18: Path variable updates are correct.Both include directory paths are properly updated to use
ELOQSQL_SOURCE_DIR.scripts/sys_schema/CMakeLists.txt (1)
177-177: Binary directory path update is correct.Using
ELOQSQL_BINARY_DIRfor the generated SQL file output ensures it's placed in the project-specific binary directory, which is essential for library-mode builds.storage/perfschema/unittest/CMakeLists.txt (1)
23-29: Path variable definitions verified.The verification confirms that
ELOQSQL_SOURCE_DIRandELOQSQL_BINARY_DIRare properly defined in the root CMakeLists.txt (lines 47–48). All include directories in storage/perfschema/unittest/CMakeLists.txt correctly reference these variables. The changes are consistent with the project-wide refactoring.cmake/build_configurations/mysql_release.cmake (1)
89-91: LGTM: Consistent path variable update.The replacement of
CMAKE_SOURCE_DIRwithELOQSQL_SOURCE_DIRaligns with the project-wide refactoring to support library-mode builds.plugin/auth_dialog/CMakeLists.txt (1)
19-20: LGTM: Maintaining consistency in commented code.The path variable update in the commented-out plugin definition maintains consistency with the project-wide refactoring.
storage/rocksdb/unittest/CMakeLists.txt (1)
2-5: LGTM: Include directories updated consistently.All include directory paths have been correctly updated to use
ELOQSQL_SOURCE_DIR, maintaining consistency with the build system refactoring.storage/perfschema/CMakeLists.txt (1)
23-28: LGTM: Include directories updated to use project-specific variables.Both source and binary directory paths have been correctly updated to use
ELOQSQL_SOURCE_DIRandELOQSQL_BINARY_DIR.cmake/mysql_add_executable.cmake (1)
45-48: LGTM: Windows manifest path updated correctly.The change from
PROJECT_SOURCE_DIRtoELOQSQL_SOURCE_DIRensures consistent behavior when building as a subproject or library.storage/mroonga/vendor/groonga/vendor/plugins/groonga-normalizer-mysql/CMakeLists.txt (2)
60-61: LGTM: Binary directory variable updated consistently.The include directory correctly uses
ELOQSQL_BINARY_DIRinstead ofCMAKE_BINARY_DIR.
35-39: No issues found with bundled detection logic.The variable
ELOQSQL_SOURCE_DIRis properly defined at the root CMakeLists.txt level (line 47 in git/CMakeLists.txt) as${CMAKE_CURRENT_SOURCE_DIR}. The comparison logic is correct: it determines the plugin is unbundled when located at its expected position within the source tree (whereELOQSQL_SOURCE_DIRequalsCMAKE_CURRENT_SOURCE_DIR), and bundled otherwise. This pattern is consistently used throughout the codebase, including in mroonga and groonga CMakeLists files, indicating this is the established convention for this build system. The change fromCMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRis intentional and makes the logic more specific to the EloqSQL build environment.storage/spider/CMakeLists.txt (2)
42-45: LGTM: Include directories updated consistently for MySQL storage engine path.All include paths correctly use
ELOQSQL_SOURCE_DIRinstead ofCMAKE_SOURCE_DIR.
51-51: LGTM: Spider client include path updated consistently.The include directory for the HandleSocket client correctly uses
ELOQSQL_SOURCE_DIR.sql/threadpool_generic.h (1)
32-32: Based on the verification results, I cannot locate theconcurrent_queue_wsize.hheader file anywhere in the repository. Thefdsearch found no matches, and the only references appear in generated BUILD artifacts, not in actual source code or include directories.Given that:
- The file is not present in the repository
- CMake is configured to search in
${ELOQSQL_SOURCE_DIR}/includeand${ELOQSQL_SOURCE_DIR}/sql- The original nested relative path has been replaced with just the filename
Verify the original include path and header location before merging.
The simplified path assumes the header exists in one of the configured include directories, but I cannot find it. Either confirm the header exists post-build, verify the original path location, or ensure the file is present at the expected location in the include directories.
mysql-test/mtr.out-of-source (1)
3-5: LGTM!The replacement of
CMAKE_BINARY_DIRandCMAKE_SOURCE_DIRwithELOQSQL_BINARY_DIRandELOQSQL_SOURCE_DIRis consistent and correct throughout this test harness script.cmake/cpack_rpm.cmake (2)
231-239: LGTM!The RPM script file paths have been correctly updated to use
ELOQSQL_SOURCE_DIRinstead ofCMAKE_SOURCE_DIR. All script references follow the same pattern.
306-328: LGTM!The file glob patterns and RPM command paths have been correctly updated to use
ELOQSQL_SOURCE_DIR. The logic for building MariaDB-compat packages remains unchanged.cmake/readline.cmake (1)
96-98: LGTM!The bundled readline paths have been correctly updated to use
ELOQSQL_SOURCE_DIRinstead ofCMAKE_SOURCE_DIR.cmake/submodules.cmake (1)
8-46: LGTM!The Git submodule handling paths have been correctly updated to use
ELOQSQL_SOURCE_DIRinstead ofCMAKE_SOURCE_DIR. All working directories and path checks are consistent.win/packaging/CMakeLists.txt (3)
144-144: LGTM!The default third-party download location has been correctly updated to use
ELOQSQL_BINARY_DIR.
199-199: LGTM!The MSI target's TOP_BINDIR parameter has been correctly updated to use
ELOQSQL_BINARY_DIR.
224-236: LGTM!The working directories for all Windows packaging targets have been correctly updated to use
ELOQSQL_BINARY_DIR.storage/innobase/unittest/CMakeLists.txt (1)
16-19: LGTM!The include directories have been correctly updated to use
ELOQSQL_SOURCE_DIRinstead ofCMAKE_SOURCE_DIR.cmake/ssl.cmake (1)
52-54: LGTM!The bundled SSL include directories have been correctly updated to use
ELOQSQL_BINARY_DIRandELOQSQL_SOURCE_DIRinstead ofCMAKE_BINARY_DIRandCMAKE_SOURCE_DIR.cmake/zlib.cmake (1)
17-17: LGTM!The migration from
CMAKE_SOURCE_DIRandCMAKE_BINARY_DIRtoELOQSQL_SOURCE_DIRandELOQSQL_BINARY_DIRis correct and consistent with the project-wide directory variable standardization.cmake/abi_check.cmake (1)
38-79: LGTM!The directory variable migration is applied consistently across all API preprocessor headers and custom targets (abi_check, abi_update, abi_check_all). The changes align with the project-wide standardization to ELOQSQL-specific directory variables.
mysql-test/CMakeLists.txt (2)
19-19: LGTM!The out-of-source build check correctly uses the updated ELOQSQL directory variables.
109-109: LGTM!The NIST test suite path check correctly references
ELOQSQL_SOURCE_DIRinstead ofCMAKE_SOURCE_DIR.extra/CMakeLists.txt (3)
16-16: LGTM!The include directory path correctly uses
ELOQSQL_SOURCE_DIRfor project includes.
31-47: LGTM!The custom command for generating error headers consistently uses
ELOQSQL_BINARY_DIRfor all output and intermediate file paths. The migration is thorough and correct.
69-71: LGTM!The InnoDB include paths correctly reference
ELOQSQL_SOURCE_DIRfor storage engine headers.configure.cmake (2)
171-171: LGTM!The libwrap source file path correctly uses
ELOQSQL_SOURCE_DIR.
681-684: LGTM!The stack direction detection test correctly uses
ELOQSQL_BINARY_DIRandELOQSQL_SOURCE_DIRfor the TRY_RUN command.mysys/CMakeLists.txt (1)
17-17: LGTM!The include directories for the mysys library correctly use
ELOQSQL_SOURCE_DIRfor both the public include directory and the mysys-specific headers.win/upgrade_wizard/CMakeLists.txt (2)
23-23: LGTM!The upgrade wizard source path correctly references
ELOQSQL_SOURCE_DIRfor the winservice.c file.
34-34: LGTM!The include directory path correctly uses
ELOQSQL_SOURCE_DIRfor SQL headers.win/packaging/ca/CMakeLists.txt (2)
18-18: LGTM!The include directory path correctly uses
ELOQSQL_SOURCE_DIRfor SQL headers in the Windows packaging custom action.
23-23: LGTM!The winservice.c source path in the custom action library correctly references
ELOQSQL_SOURCE_DIR.storage/mroonga/CMakeLists.txt (1)
425-426: LGTM: test suite path migration.Using ${ELOQSQL_SOURCE_DIR}/mysql-test/suite/mroonga with fallback is correct.
cmake/mysql_version.cmake (1)
26-28: LGTM: VERSION and rc outputs moved to ELOQSQL_ paths.*Consistent with the repository-wide path migration.
Also applies to: 34-34, 130-141, 136-143
client/CMakeLists.txt (1)
18-25: Path references verified—all files and directories exist.The ELOQSQL_SOURCE_DIR and ELOQSQL_BINARY_DIR variables are defined in the root CMakeLists.txt (lines 47–48), sql/sql_string.cc exists, and libmariadb/ is present. All include paths and file references in client/CMakeLists.txt (lines 18–25, 29–32, 40–41, 51–52, 88–89) are valid.
cmake/plugin.cmake (1)
45-49: Verification confirms all paths and files are correctly configured.The cmake/plugin.cmake file uses consistent
${ELOQSQL_SOURCE_DIR}references throughout:
- Lines 45-49 include INCLUDE_DIRECTORIES with ${ELOQSQL_SOURCE_DIR}/include, sql, and other standard paths
- Lines 270-271 correctly reference the RPM scripts at ${ELOQSQL_SOURCE_DIR}/support-files/rpm/plugin-postin.sh and server-posttrans.sh
- Lines 309-310 in CONFIGURE_PLUGINS glob ${ELOQSQL_SOURCE_DIR}/storage/* and ${ELOQSQL_SOURCE_DIR}/plugin/*
All referenced files and directories exist in the repository:
./support-files/rpm/plugin-postin.sh✓./support-files/rpm/server-posttrans.sh✓./storage/✓./plugin/✓The paths use a uniform root variable, and CMake will resolve
${ELOQSQL_SOURCE_DIR}at configure time. No issues found.libmysqld/CMakeLists.txt (1)
21-25: No issues found; all path migrations and variable definitions are correct and consistent.Verification confirms:
- ELOQSQL_SOURCE_DIR and ELOQSQL_BINARY_DIR are defined in root CMakeLists.txt (lines 47–48) before any subdirectories are processed.
- Generators (yy_*.cc, lex_hash.h) use CMAKE_CURRENT_BINARY_DIR, which correctly resolves to ${ELOQSQL_BINARY_DIR}/sql in the sql/ subdirectory context.
- File naming is consistent: sql_builtin.cc (singular generated file) builds the sql_builtins library (plural name intentional).
- References across libmysqld/CMakeLists.txt and sql/CMakeLists.txt use the correct paths and naming.
sql/mdl.h (1)
26-26: LGTM: Include path simplified.The change from a nested relative path to a local include aligns with the project-wide directory restructuring.
unittest/my_decimal/CMakeLists.txt (1)
16-19: LGTM: Path variables updated correctly.The change from
CMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRis consistent with the project-wide directory variable refactoring.vio/CMakeLists.txt (1)
16-16: LGTM: Path variable updated correctly.Consistent with the project-wide migration to
ELOQSQL_SOURCE_DIR.storage/eloq/eloqsql_catalog_factory.cpp (1)
21-21: LGTM: Header includes updated to eloqsql-prefixed variants.The header renames are consistent with the project-wide migration to
eloqsql_*naming.Also applies to: 32-32
unittest/sql/CMakeLists.txt (1)
18-21: LGTM: Path variables updated correctly.The migration to
ELOQSQL_SOURCE_DIRis consistent with the project-wide refactoring.storage/eloq/ha_eloq.h (2)
65-66: LGTM: Header includes updated to eloqsql-prefixed variants.The header renames are consistent with the project-wide migration.
Also applies to: 72-72
81-81: Verify lifecycle management with external DataSubstrate library.The local codebase shows proper patterns:
storage_hdis initialized tonullptr, obtained fromDataSubstrate::GetGlobal()->GetStoreHandler()(line 1506), includes defensive null checks (lines 5395, 7698, and test files), and cleanup is delegated toDataSubstrate::GetGlobal()->Shutdown()(line 1574). No explicitdeleteornewoperations exist locally.However, the pointer lifecycle ultimately depends on the external
data_substratesubmodule (https://github.com/eloqdata/tx_service.git), whose source is not available in this environment. Verify with the DataSubstrate library documentation or maintainers that:
GetStoreHandler()returns a properly managed pointer (borrowed reference from DataSubstrate)Shutdown()correctly deallocates the handler- No double-free or leak scenarios exist across the integration
storage/eloq/eloq_system_handler.h (1)
43-43: LGTM: Added missingoverridekeyword.Adding
overrideensures compile-time verification thatShutdown()correctly overrides the base class method, improving code safety.cmake/pcre.cmake (1)
8-8: LGTM! Directory variable updated consistently.The change from
CMAKE_BINARY_DIRtoELOQSQL_BINARY_DIRaligns with the project-wide standardization of directory variables for the library build mode.cmake/tags.cmake (1)
20-20: LGTM! Working directory updated consistently.The working directory changes from
CMAKE_SOURCE_DIRtoELOQSQL_SOURCE_DIRare consistent with the project-wide directory variable standardization.Also applies to: 24-24
tests/CMakeLists.txt (1)
19-23: LGTM! Include directories updated consistently.The include directory paths have been properly updated from
CMAKE_SOURCE_DIR/CMAKE_BINARY_DIRtoELOQSQL_SOURCE_DIR/ELOQSQL_BINARY_DIR, maintaining the same logical structure.storage/eloq/eloqsql_catalog_factory.h (1)
27-28: LGTM! Header includes renamed consistently.The header includes have been updated to use the
eloqsql_prefix, aligning with the project-wide header renaming effort.cmake/install_macros.cmake (1)
62-65: LGTM! Directory paths updated consistently.The manpage glob expressions and debug build directory computation have been properly updated to use
ELOQSQL_SOURCE_DIRandELOQSQL_BINARY_DIR, maintaining the same logical behavior.Also applies to: 263-263
scripts/CMakeLists.txt (1)
32-32: LGTM! Directory variables updated consistently.All directory references have been properly updated from
CMAKE_SOURCE_DIR/CMAKE_BINARY_DIRtoELOQSQL_SOURCE_DIR/ELOQSQL_BINARY_DIR, maintaining the same logical behavior throughout the script configuration.Also applies to: 43-43, 87-87
scripts/mysql_install_db.sh (1)
543-565: The--eloqsql_configflag is properly supported in mysqld binary.Verification confirms the flag is defined at
sql/mysqld.cc:646as a gflags string parameter and is actively used in the bootstrap initialization flow. The implementation correctly converts--eloqsql_configto--defaults-file=internally, validating your config file approach. The temporary file creation, cleanup, and exit code preservation all follow best practices.cmake/info_macros.cmake.in (1)
63-76: Path rebase to ELOQSQL_ looks consistent.*Git detection, Docs/INFO_SRC reads, and feature flag dump now point at the new dirs; this matches the project-wide layout change. LGTM.
Also applies to: 85-96, 151-154
cmake/make_dist.cmake.in (1)
37-37: PACKAGE_DIR rebase to ELOQSQL_BINARY_DIR — OK.Matches the converged build layout. No issues.
storage/eloq/eloq_i_s.cc (1)
65-69: Manual verification required: Unable to locate DiscoverAllTableNames method definition in searchable codebase.The method is called at three locations with two different signatures (1-parameter and 3-parameter variants), confirming overloads exist. However, the actual method definition could not be found in repository files despite extensive searching. This suggests the method may be defined in an external library or header not included in the searchable codebase.
The review concern about type mismatch between pointer types (returned by
thd_get_coro_functors) and method parameter expectations is valid in principle. To conclusively verify this, inspect theDiscoverAllTableNamesmethod signature in its definition header to confirm whether the 3-parameter version expectsstd::function<void()>*(pointers) orstd::function<void()>&(references).sql/mysqld.cc (3)
4045-4047: Comment‑only changeNo functional impact.
5638-5639: Comment‑only changeNo functional impact.
645-649: Change DECLARE_bool to DEFINE_bool for the bootstrap flagVerification shows
DECLARE_bool(bootstrap)at sql/mysqld.cc:648 has no correspondingDEFINE_bool(bootstrap)anywhere in the codebase, despiteFLAGS_bootstrapbeing actively used at lines 5570 and 5596–5597 in the same file. Additionally,sql/main.ccdoes not exist. SinceDEFINE_string(eloqsql_config)is already in sql/mysqld.cc and the flag is used there, replaceDECLARE_bool(bootstrap)withDEFINE_bool(bootstrap, ...)in the same section:-DECLARE_bool(bootstrap); +DEFINE_bool(bootstrap, false, "Enable bootstrap mode");Likely an incorrect or invalid review comment.
storage/eloq/ha_eloq.cc (1)
213-220: Verify DEFINE_string/FLAGS_config availability.This relies on a flags library (e.g., gflags). Ensure the header is included and the build always links the right lib when this unit is compiled; otherwise, replace with a simple string setter in DataSubstrate.
Would you confirm which flags lib provides DEFINE_string here and that it’s linked whenever ha_eloq.cc is compiled? If needed, I can propose a guard to compile out the flag in non-gflags builds.
CMakeLists.txt (1)
466-475: Ensure data_substrate target is available when eloq storage engine links in library mode.The eloq storage engine (storage/eloq/CMakeLists.txt:136) is MANDATORY and unconditionally requires
LINK_LIBRARIES data_substrate. In library mode, no data_substrate target is created, causing linker failure when eloq tries to resolve the dependency. The proposed fix—checking target existence, attemptingfind_package, and creating anINTERFACE IMPORTEDplaceholder as fallback—is a valid approach to ensure downstream availability.-else() - # Use parent data_substrate (converged mode) - message(STATUS "EloqSQL: Using parent data_substrate") - # When building as library, mysqld will NOT initialize data substrate - # It will use synchronization with converged main instead - add_compile_definitions(MYSQLD_LIBRARY_MODE) -endif() +else() + message(STATUS "EloqSQL: Using parent data_substrate") + add_compile_definitions(MYSQLD_LIBRARY_MODE) + if(NOT TARGET data_substrate) + find_package(data_substrate CONFIG QUIET) + if(NOT TARGET data_substrate) + add_library(data_substrate INTERFACE IMPORTED) # placeholder, parent must replace + endif() + endif() +endif()sql/sql_select.cc (1)
28-28: Include path updated correctly to reflect directory restructuring.The relative path adjustment aligns with the PR's refactoring objective to restructure headers under
storage/eloq/with new include paths. The path resolution fromsql/sql_select.ccto../storage/eloq/eloq_errors.his correct.
| [submodule "data_substrate/eloq_metrics"] | ||
| path = data_substrate/eloq_metrics | ||
| url = https://github.com/eloqdata/eloq-metrics.git | ||
| [submodule "storage/eloq/tx_service"] | ||
| path = storage/eloq/tx_service | ||
| [submodule "data_substrate/tx_service"] | ||
| path = data_substrate | ||
| url = https://github.com/eloqdata/tx_service.git | ||
| [submodule "storage/eloq/store_handler"] | ||
| path = storage/eloq/store_handler | ||
| [submodule "data_substrate/store_handler"] | ||
| path = data_substrate/store_handler | ||
| url = https://github.com/eloqdata/store_handler.git | ||
| [submodule "storage/eloq/log_service"] | ||
| path = storage/eloq/log_service | ||
| [submodule "data_substrate/log_service"] | ||
| path = data_substrate/log_service | ||
| url = https://github.com/eloqdata/log_service.git |
There was a problem hiding this comment.
🧩 Analysis chain
Verify submodule paths are correctly initialized.
After fixing the tx_service path issue, ensure all submodules are properly initialized and checked out to the correct locations.
Run the following script to verify submodule configuration:
🏁 Script executed:
#!/bin/bash
# Verify that submodule paths don't conflict and exist in the expected locations
echo "=== Checking submodule configuration ==="
git config --file .gitmodules --get-regexp 'submodule\.data_substrate.*\.(path|url)'
echo ""
echo "=== Verifying no path conflicts ==="
# Check if any two submodules share the same path
git config --file .gitmodules --get-regexp 'submodule\..*\.path' | \
awk '{print $2}' | sort | uniq -d | \
while read -r duplicate; do
echo "ERROR: Duplicate path found: $duplicate"
done
echo ""
echo "=== Checking if submodule directories exist ==="
for submodule in data_substrate/eloq_metrics data_substrate/tx_service data_substrate/store_handler data_substrate/log_service; do
if [ -d "$submodule" ]; then
echo "✓ $submodule exists"
else
echo "✗ $submodule does not exist"
fi
doneLength of output: 917
Fix tx_service submodule path configuration.
The verification confirms a critical submodule path conflict:
data_substrate/tx_servicesubmodule is configured with pathdata_substrate(the parent directory)- Other submodules (
eloq_metrics,store_handler,log_service) are configured as subdirectories ofdata_substrate
This creates a structural conflict. The tx_service path should be data_substrate/tx_service to match the naming convention and avoid conflicts with sibling submodules.
Update .gitmodules line 25 to:
path = data_substrate/tx_service
🤖 Prompt for AI Agents
In .gitmodules around lines 20 to 31, the tx_service submodule is incorrectly
set with path = data_substrate which conflicts with sibling submodules; change
that entry so the tx_service submodule's path is data_substrate/tx_service
(i.e., update line 25 to path = data_substrate/tx_service), then run git
submodule sync and git submodule update --init --recursive to apply and fetch
the corrected submodule mapping.
| [submodule "data_substrate/tx_service"] | ||
| path = data_substrate |
There was a problem hiding this comment.
Critical: Submodule path mismatch.
The submodule is named data_substrate/tx_service but the path is set to just data_substrate. This inconsistency will cause the tx_service repository to be checked out at the data_substrate directory root, which conflicts with other submodules (eloq_metrics, store_handler, log_service) that use data_substrate/* as their paths.
Apply this diff to fix the path:
[submodule "data_substrate/tx_service"]
- path = data_substrate
+ path = data_substrate/tx_service
url = https://github.com/eloqdata/tx_service.git📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [submodule "data_substrate/tx_service"] | |
| path = data_substrate | |
| [submodule "data_substrate/tx_service"] | |
| path = data_substrate/tx_service |
🤖 Prompt for AI Agents
In .gitmodules around lines 23-24, the submodule declaration name is
"data_substrate/tx_service" but the path is incorrectly set to "data_substrate";
update the path to "data_substrate/tx_service" so the submodule is checked out
into its own directory (data_substrate/tx_service) and no longer conflicts with
other data_substrate/* submodules; ensure the path value exactly matches the
submodule name.
| # IF(PLUGIN_ELOQ) | ||
| ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE) | ||
| ENDIF() | ||
| # ENDIF() | ||
|
|
There was a problem hiding this comment.
Unconditional -DWITH_ELOQ_STORAGE_ENGINE define changes client semantics; gate it.
This define was previously conditional. Making it unconditional may affect all client binaries.
Apply:
-# IF(PLUGIN_ELOQ)
- ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE)
-# ENDIF()
+IF(PLUGIN_ELOQ)
+ ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE)
+ENDIF()If PLUGIN_ELOQ is no longer available, consider introducing an option(WITH_ELOQ_STORAGE_ENGINE OFF) to keep behavior controllable.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # IF(PLUGIN_ELOQ) | |
| ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE) | |
| ENDIF() | |
| # ENDIF() | |
| IF(PLUGIN_ELOQ) | |
| ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE) | |
| ENDIF() |
🤖 Prompt for AI Agents
In client/CMakeLists.txt around lines 47 to 50, the -DWITH_ELOQ_STORAGE_ENGINE
define is being applied unconditionally which changes client semantics; restore
gating by making the define conditional: add an option(WITH_ELOQ_STORAGE_ENGINE
OFF) at top (or re-check PLUGIN_ELOQ if that variable still exists) and wrap the
ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE) in an IF(WITH_ELOQ_STORAGE_ENGINE)
... ENDIF() so the define is only added when explicitly enabled.
| CONFIGURE_FILE(${ELOQSQL_SOURCE_DIR}/include/probes_mysql.d.base | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql.d COPYONLY) | ||
| DTRACE_HEADER( | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql.d | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql_dtrace.h | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql_nodtrace.h | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql.d | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql_dtrace.h | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql_nodtrace.h | ||
| ) | ||
| ADD_CUSTOM_TARGET(gen_dtrace_header | ||
| DEPENDS | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql.d | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql_dtrace.h | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql_nodtrace.h | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql.d | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql_dtrace.h | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql_nodtrace.h | ||
| ) | ||
| ELSE() | ||
| CONFIGURE_FILE(${CMAKE_SOURCE_DIR}/include/probes_mysql_nodtrace.h.in | ||
| ${CMAKE_BINARY_DIR}/include/probes_mysql_nodtrace.h COPYONLY) | ||
| CONFIGURE_FILE(${ELOQSQL_SOURCE_DIR}/include/probes_mysql_nodtrace.h.in | ||
| ${ELOQSQL_BINARY_DIR}/include/probes_mysql_nodtrace.h COPYONLY) | ||
| ENDIF() |
There was a problem hiding this comment.
Create binary include directory before writing headers.
configure_file won’t create missing parent dirs reliably in all CMake versions. Ensure ${ELOQSQL_BINARY_DIR}/include exists to avoid first-config build breaks.
# Before CONFIGURE_FILE calls
FILE(MAKE_DIRECTORY "${ELOQSQL_BINARY_DIR}/include")🤖 Prompt for AI Agents
In cmake/dtrace.cmake around lines 82 to 98 the script calls CONFIGURE_FILE to
write into ${ELOQSQL_BINARY_DIR}/include but does not ensure that the
${ELOQSQL_BINARY_DIR}/include directory exists, which can break on first-config
with some CMake versions; fix it by creating the binary include directory before
any CONFIGURE_FILE/DTRACE_HEADER/ADD_CUSTOM_TARGET calls (e.g. invoke
FILE(MAKE_DIRECTORY "${ELOQSQL_BINARY_DIR}/include") immediately before the
CONFIGURE_FILE calls so the parent directory is guaranteed to exist).
| COMMAND ${CMAKE_CPACK_COMMAND} -G TGZ --config ./CPackSourceConfig.cmake | ||
| ${CMAKE_BINARY_DIR}/CPackSourceConfig.cmake | ||
| ${ELOQSQL_BINARY_DIR}/CPackSourceConfig.cmake | ||
|
|
||
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR} | ||
| WORKING_DIRECTORY ${ELOQSQL_BINARY_DIR} | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
CPack invocation appears to pass two config paths; verify args.
--config ./CPackSourceConfig.cmake followed by ${ELOQSQL_BINARY_DIR}/CPackSourceConfig.cmake may duplicate/override. Confirm expected CLI and remove the extra arg.
🏁 Script executed:
#!/bin/bash
# Check cpack help and show effective args
${CMAKE_CPACK_COMMAND} --help | head -n 20
rg -n 'CPackSourceConfig.cmake' -C2Length of output: 411
**Remove extraneous config path argument on line 115.**
The `--config` option specifies a single configuration file. The command as written passes `--config ./CPackSourceConfig.cmake` followed by `${ELOQSQL_BINARY_DIR}/CPackSourceConfig.cmake` as a separate argument, which CPack does not recognize and will likely ignore or fail on. Remove the second path from line 115, keeping only the `--config` option with its value on line 114.
COMMAND ${CMAKE_CPACK_COMMAND} -G TGZ --config ./CPackSourceConfig.cmake
WORKING_DIRECTORY ${ELOQSQL_BINARY_DIR}
---
🌐 Web query:
```web
CPack command line --config option syntax positional arguments
💡 Result:
Short answer: cpack uses options (no positional package-name arguments). The --config option takes a filename: --config (defaults to CPackConfig.cmake in the current directory). Typical invocation form:
cpack []
Common options:
- --config Specify which CPack config file to read (e.g. CPackConfig.cmake). [1][2]
- -G Generator list (semicolon-separated or repeated) — not a positional argument. [3]
- -C Build configuration (Debug/Release) for multi-config generators. [3]
- -D = Override a CPACK variable. [3]
There are no documented positional arguments for selecting a package; cpack is driven by the config file and the above options. [2][4]
Sources:
[1] CPack man page (cmake.org) — description of --config.
[2] cpack(1) synopsis/description (cmake.org).
[3] cpack options (-G, -C, -D) (cmake docs).
[4] CPack module / CPackConfig.cmake behavior (cmake.org).
Remove extraneous positional argument on line 115.
CPack's --config option accepts a single config file path. The command passes --config ./CPackSourceConfig.cmake on line 114 (correct), followed by ${ELOQSQL_BINARY_DIR}/CPackSourceConfig.cmake on line 115 as a spurious positional argument. Since CPack accepts no positional arguments, remove line 115.
🤖 Prompt for AI Agents
In cmake/make_dist.cmake.in around lines 114 to 118, the CPack invocation
includes a spurious positional argument on line 115
(${ELOQSQL_BINARY_DIR}/CPackSourceConfig.cmake) after already passing --config
./CPackSourceConfig.cmake; remove the extraneous positional argument so the
COMMAND only invokes ${CMAKE_CPACK_COMMAND} -G TGZ --config
./CPackSourceConfig.cmake (with WORKING_DIRECTORY ${ELOQSQL_BINARY_DIR}
unchanged).
|
|
||
| extern std::unique_ptr<TxService> tx_service; | ||
| extern std::unique_ptr<store::DataStoreHandler> storage_hd; | ||
| extern store::DataStoreHandler *storage_hd; |
There was a problem hiding this comment.
🧩 Analysis chain
Global raw pointer: clarify ownership/lifetime to avoid dangling use.
Switching to extern store::DataStoreHandler* storage_hd; requires a clear owner to allocate, set, and null it on shutdown. If this pointer can change during plugin unload/reload, guard against races. At minimum, document ownership and ensure it’s set before any use and cleared before teardown. Consider wrapping the non-owning global in a gsl::not_null<...> at call sites or using an atomic pointer if concurrent replacement is possible.
To verify initialization/teardown paths and assignments, run:
🏁 Script executed:
#!/bin/bash
# Declarations and definitions
rg -nP -C3 '\bstore::DataStoreHandler\s*\*\s*storage_hd\b'
# Assignments (set/clear)
rg -nP -C3 '\bstorage_hd\s*=\s*'Length of output: 4800
Gathering additional context on shutdown/teardown paths:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for shutdown, cleanup, or plugin unload handlers
rg -nP -C5 '(shutdown|cleanup|unload|deinit|destroy|reset)' storage/eloq/ha_eloq.cc | head -100Length of output: 3049
🏁 Script executed:
#!/bin/bash
# Check if storage_hd is ever set to nullptr after initialization
rg -nP 'storage_hd\s*=\s*(nullptr|NULL|0)' storage/eloq/Length of output: 73
🏁 Script executed:
#!/bin/bash
# Search for thread-related calls or locks near storage_hd usage
rg -nP -B5 -A5 'storage_hd' storage/eloq/ha_eloq.cc | grep -E '(mutex|lock|atomic|thread|concurrent)' | head -20Length of output: 0
Storage handler pointer not cleared on plugin shutdown—dangling pointer risk.
In eloq_done_func() (line 1568), DataSubstrate::GetGlobal()->Shutdown() invalidates the underlying storage but does not clear the storage_hd pointer. After plugin unload, storage_hd remains pointing to deallocated memory. If the plugin is reloaded or accessed during reload, this becomes a dangling pointer.
Required fix: After DataSubstrate::GetGlobal()->Shutdown() in eloq_done_func(), explicitly add:
storage_hd = nullptr;Additional recommendations:
- Document ownership/lifetime in extern declarations (e.g., comment: "// Non-owning pointer, lifecycle managed by DataSubstrate")
- Consider wrapping at call sites with
gsl::not_null<>or using an atomic if concurrent replacement during reload is possible
🤖 Prompt for AI Agents
In storage/eloq/eloq_i_s.cc around line 34 (and in eloq_done_func at ~line
1568), the extern pointer storage_hd remains pointing at deallocated storage
after DataSubstrate::GetGlobal()->Shutdown(), creating a dangling pointer;
update eloq_done_func so that immediately after calling Shutdown() you set
storage_hd to nullptr to clear the non-owning reference; optionally add a brief
comment above the extern declaration documenting that this is a non-owning
pointer whose lifetime is managed by DataSubstrate, and consider using
gsl::not_null or an atomic wrapper at call sites if concurrent reloads are
possible.
| #ifdef MYSQLD_LIBRARY_MODE | ||
| #include <mutex> | ||
| #include <condition_variable> | ||
| // Define synchronization variables | ||
| namespace mysqld_converged_sync { | ||
| std::mutex init_mutex; | ||
| std::condition_variable mysqld_basic_init_done_cv; | ||
| std::condition_variable data_substrate_init_done_cv; | ||
| bool mysqld_basic_init_done = false; | ||
| bool data_substrate_init_done = false; | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Library-mode init can deadlock; add a timeout and document the handshake.
If the converged main never signals data_substrate_init_done, mysqld blocks indefinitely. Use wait_for with a reasonable timeout and log a hard error.
#ifdef MYSQLD_LIBRARY_MODE
#include <mutex>
#include <condition_variable>
+#include <chrono>
@@
- // Wait for data substrate initialization to complete
- mysqld_converged_sync::data_substrate_init_done_cv.wait(lock, [] {
- return mysqld_converged_sync::data_substrate_init_done;
- });
+ // Wait for data substrate initialization to complete
+ if (!mysqld_converged_sync::data_substrate_init_done_cv.wait_for(
+ lock, std::chrono::seconds(60),
+ [] { return mysqld_converged_sync::data_substrate_init_done; })) {
+ LOG(ERROR) << "Timed out waiting for data substrate initialization.";
+ return 1; // abort plugin init; prevents undefined behavior
+ }
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the actual wait implementation in ha_eloq.cc
fd -t f "ha_eloq.cc" --exec cat {} \; | head -250 | tail -80
Length of output: 2573
🏁 Script executed:
#!/bin/bash
# Search for wait() calls related to data_substrate_init_done
rg -n "data_substrate_init_done_cv\." storage/eloq/ha_eloq.cc -A2 -B2Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check for LOG macro availability in header or this file
rg -n "^#include.*log|#define LOG|LOG(ERROR)" storage/eloq/ha_eloq.cc | head -20Length of output: 202
🏁 Script executed:
#!/bin/bash
# Search for eloq_insert_semantic definition/declaration
rg -n "eloq_insert_semantic" storage/eloq/ -C2Length of output: 1104
🏁 Script executed:
#!/bin/bash
# Search for DEFINE_string usage in the codebase
rg -n "DEFINE_string" storage/eloq/ -C1Length of output: 160
🏁 Script executed:
#!/bin/bash
# Check if there's a return statement or error handling in plugin init context
rg -n "plugin_init|storage_engine_init" storage/eloq/ha_eloq.cc -A10 -B2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get broader context around the wait() call at line 1466
sed -n '1450,1480p' storage/eloq/ha_eloq.cc
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Get context around eloq_insert_semantic usage at line 1496
sed -n '1490,1510p' storage/eloq/ha_eloq.ccLength of output: 862
🏁 Script executed:
#!/bin/bash
# Search for the function that contains line 1466 (search backwards for function definition)
sed -n '1400,1470p' storage/eloq/ha_eloq.cc | grep -n "^[a-zA-Z_].*{" | tail -1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find init functions - try different patterns
rg -n "^[a-zA-Z_].*int.*init|^[a-zA-Z_].*int.*Init" storage/eloq/ha_eloq.cc | head -10Length of output: 129
🏁 Script executed:
#!/bin/bash
# Check for any return statements near the wait() call
sed -n '1460,1475p' storage/eloq/ha_eloq.ccLength of output: 710
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full eloq_init_func function to see return statements and error handling
sed -n '1440,1550p' storage/eloq/ha_eloq.cc
Length of output: 3918
🏁 Script executed:
#!/bin/bash
# Check for null handling or guards for eloq_insert_semantic before line 1496
sed -n '1480,1505p' storage/eloq/ha_eloq.ccLength of output: 1127
🏁 Script executed:
#!/bin/bash
# Search for return statements in eloq_init_func
rg -n "^\s*return" storage/eloq/ha_eloq.cc | grep -A2 -B2 "154"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the ending of eloq_init_func to see return value
sed -n '1520,1560p' storage/eloq/ha_eloq.ccLength of output: 921
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the end of eloq_init_func to see its return statement
sed -n '1560,1600p' storage/eloq/ha_eloq.cc
Length of output: 890
🏁 Script executed:
#!/bin/bash
# Search for DBUG_RETURN in the function
rg -n "DBUG_RETURN" storage/eloq/ha_eloq.cc | head -10Length of output: 369
🏁 Script executed:
#!/bin/bash
# Check what eloq_init_abort returns
rg -n "eloq_init_abort" storage/eloq/ha_eloq.cc -A5Length of output: 373
🏁 Script executed:
#!/bin/bash
# Look for any existing timeout handling or chrono usage in the file
rg -n "wait_for|chrono|timeout" storage/eloq/ha_eloq.ccLength of output: 579
Add timeout to wait() and null-check eloq_insert_semantic.
Two critical issues found:
-
Line 1466 deadlock risk: The
wait()call has no timeout. If the converged main never signals, mysqld blocks indefinitely. Add a 60-second timeout with error logging and abort. -
Line 1496 null dereference:
eloq_insert_semanticis initialized tonullptr(line 241) but used directly without null checking at line 1496 in the string assignment. Add a null guard before dereferencing.
The DEFINE_string(config, ...) is already properly defined at line 213. The shutdown behavior is safe—eloq_done_func handles partial initialization cleanly.
🤖 Prompt for AI Agents
In storage/eloq/ha_eloq.cc around lines 189 and especially referencing lines
241, 1466 and 1496: add a 60-second timeout to the condition_variable wait used
at line 1466 so the thread won't block forever — use wait_for with a 60s
deadline, log an error if it times out and abort/exit cleanly; and before the
string assignment at line 1496 add a null check for eloq_insert_semantic
(initialized at line 241) so you never dereference a nullptr — if it's null, log
an error and handle the failure path (abort/return) instead of proceeding.
Ensure existing DEFINE_string(config, ...) at line 213 is left as-is.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
storage/eloq/mysql-test/mono_basic/t/mvcc_scan_sk_rc_isolvl.cnf (1)
12-18: File uses outdated inline configuration instead of centralized eloq_config approach.The verification confirms the review concern is valid. This file retains inline per-instance settings (plugin_load_add, eloq_core_num, eloq_enable_mvcc, eloq_hm_bin_path), while other similar test files like mvcc_ds.cnf have been refactored to centralize these settings via
eloq_config. The inconsistency is real and needs correction to align with the established centralization pattern.storage/eloq/ha_eloq.cc (1)
1503-1509: Null-deref risk: eloq_insert_semantic may be nullptr.Constructing std::string from a null C string is UB. Default safely when unset.
Apply this diff:
- std::string insert_semantic_str= eloq_insert_semantic; + std::string insert_semantic_str = + (eloq_insert_semantic && *eloq_insert_semantic) + ? eloq_insert_semantic + : "insert";
♻️ Duplicate comments (5)
sql/CMakeLists.txt (2)
263-263: Guard data_substrate target linkage for library/converged mode.This was flagged in the previous review but remains unresolved. In library mode (BUILD_ELOQSQL_AS_LIBRARY=ON), the data_substrate target is not locally defined, causing a CMake "Unknown target" error. Wrap the linkage in a TARGET check.
-MYSQL_ADD_PLUGIN(thread_pool_info thread_pool_info.cc DEFAULT STATIC_ONLY NOT_EMBEDDED LINK_LIBRARIES data_substrate) +if(TARGET data_substrate) + set(_tp_extra_libs data_substrate) +endif() +MYSQL_ADD_PLUGIN(thread_pool_info thread_pool_info.cc DEFAULT STATIC_ONLY NOT_EMBEDDED LINK_LIBRARIES ${_tp_extra_libs})
314-318: Uncomment and gate WITH_ELOQ_STORAGE_ENGINE on PLUGIN_ELOQ.This was flagged in the previous review. Currently the lines remain commented out despite being marked as changed. Unconditionally defining WITH_ELOQ_STORAGE_ENGINE will break non‑ELOQ builds. Uncomment and wrap in an IF(PLUGIN_ELOQ) conditional.
-# IF(PLUGIN_ELOQ) +if(PLUGIN_ELOQ) ADD_DEFINITIONS(-DWITH_ELOQ_STORAGE_ENGINE) - SET(ELOQ_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}/storage/eloq) - set(SQL_INCLUDE_DIR ${SQL_INCLUDE_DIR} ${ELOQ_SOURCE_DIR}) -# ENDIF() + set(ELOQ_SOURCE_DIR ${ELOQSQL_SOURCE_DIR}/storage/eloq) + list(APPEND SQL_INCLUDE_DIR ${ELOQ_SOURCE_DIR}) +endif()sql/mysqld.cc (1)
5565-5614: Fix argv construction: avoid const_cast on std::string::c_str(), guard FLAGS_ usage, and free owned memory.*
- Passing const_cast<char*> from std::string risks UB if mutated by callees; use strdup.
- Wrap FLAGS_* access with WITH_GFLAGS to keep non‑gflags builds working.
- Ensure duplicated C‑string is freed.
// Build argv for load_defaults // Copy all original arguments first, then append --eloqsql_config and --bootstrap if needed - int additional_args = 0; - if (!FLAGS_eloqsql_config.empty()) - additional_args++; - if (FLAGS_bootstrap) - additional_args++; + int additional_args = 0; +#ifdef WITH_GFLAGS + if (!FLAGS_eloqsql_config.empty()) additional_args++; + if (FLAGS_bootstrap) additional_args++; +#endif @@ - char **load_default_argv = (char**)malloc((argc + additional_args + 1) * sizeof(char*)); + char **load_default_argv = (char**)malloc((argc + additional_args + 1) * sizeof(char*)); if (!load_default_argv) { fprintf(stderr, "Failed to allocate memory for load_default_argv\n"); return 1; } // Copy all original arguments int load_default_argc = argc; for (int i = 0; i < argc; i++) { load_default_argv[i] = argv[i]; } // Append --defaults-file if --eloqsql_config specified - std::string defaults_file_arg; - if (!FLAGS_eloqsql_config.empty()) - { - defaults_file_arg = std::string("--defaults-file=") + FLAGS_eloqsql_config; - load_default_argv[load_default_argc] = const_cast<char*>(defaults_file_arg.c_str()); - load_default_argc++; - } + char* defaults_file_cstr = nullptr; +#ifdef WITH_GFLAGS + if (!FLAGS_eloqsql_config.empty()) { + const std::string defaults_file_arg = std::string("--defaults-file=") + FLAGS_eloqsql_config; + defaults_file_cstr = ::strdup(defaults_file_arg.c_str()); + if (!defaults_file_cstr) { + fprintf(stderr, "Out of memory duplicating --defaults-file\n"); + free(load_default_argv); + return 1; + } + load_default_argv[load_default_argc++] = defaults_file_cstr; + } +#endif // Append --bootstrap if FLAGS_bootstrap is true - if (FLAGS_bootstrap) - { - load_default_argv[load_default_argc] = const_cast<char*>("--bootstrap"); - load_default_argc++; - } + #ifdef WITH_GFLAGS + if (FLAGS_bootstrap) { + load_default_argv[load_default_argc++] = const_cast<char*>("--bootstrap"); + } + #endif // Null-terminate the array load_default_argv[load_default_argc] = nullptr; @@ - load_defaults_or_exit(MYSQL_CONFIG_NAME, load_default_groups, + load_defaults_or_exit(MYSQL_CONFIG_NAME, load_default_groups, &load_default_argc, &load_default_argv_ptr); @@ - defaults_argc= load_default_argc; + defaults_argc= load_default_argc; defaults_argv= load_default_argv_ptr; remaining_argc= load_default_argc; remaining_argv= load_default_argv_ptr;And free the duplicated C‑string together with the temporary argv array:
@@ - // Free the temporary argv array we allocated - free(load_default_argv); + // Free the temporary argv array and any duplicated strings we allocated + #ifdef WITH_GFLAGS + if (defaults_file_cstr) free(defaults_file_cstr); + #endif + free(load_default_argv);storage/eloq/ha_eloq.cc (1)
1460-1476: Library-mode CV wait can hang forever; add timeout and fail fast.If converged main never signals, mysqld blocks indefinitely. Use wait_for with a deadline and abort plugin init cleanly on timeout.
Apply this diff:
#ifdef MYSQLD_LIBRARY_MODE - { + { std::unique_lock<std::mutex> lock(mysqld_converged_sync::init_mutex); mysqld_converged_sync::mysqld_basic_init_done= true; mysqld_converged_sync::mysqld_basic_init_done_cv.notify_one(); LOG(INFO) << "MySQL basic initialization complete, waiting for data " "substrate..."; - // Wait for data substrate initialization to complete - mysqld_converged_sync::data_substrate_init_done_cv.wait( - lock, [] { return mysqld_converged_sync::data_substrate_init_done; }); + // Wait for data substrate initialization to complete (timeout: 60s) + using namespace std::chrono_literals; + const bool ok = mysqld_converged_sync::data_substrate_init_done_cv.wait_for( + lock, 60s, + [] { return mysqld_converged_sync::data_substrate_init_done; }); + if (!ok) { + LOG(ERROR) << "Timed out waiting for data substrate initialization."; + DBUG_RETURN(eloq_init_abort()); + } LOG(INFO) << "Data substrate initialized, MySQL continuing..."; } #elseAdd header:
#include <tuple> +#include <chrono>#!/bin/bash # Check for any other unconditional waits on converged sync to harden similarly rg -n 'data_substrate_init_done_cv\.(wait|wait_for)' storage/eloq -C2storage/eloq/eloq_db_dl.h (1)
30-143: Missing required dependencies still make this header uncompilable.
STRING_WITH_LEN,Discovered_table_list,File_option, anducharare all referenced here but never brought into scope, so any TU that includes this header without prior transitive includes keeps failing to compile.STRING_WITH_LENlives inm_string.h,Discovered_table_listis a class declared insql/handler.cc,File_optionis defined viaparse_file.h, anducharcomes frommy_inttypes.h.(dev.mysql.com) This exact gap was flagged in the previous review, so we still need to add the missing includes/forward declarations.#include <string> +#include "m_string.h" +#include "my_inttypes.h" + +class Discovered_table_list; +struct File_option;Without these additions the static-link refactor continues to break consumers that include this header directly.
🧹 Nitpick comments (6)
storage/eloq/mysql-test/mono_basic/t/mvcc_scan.cnf (1)
13-13: Quoteeloq_configfor whitespace-safe paths
MYSQLTEST_VARDIRoften expands to a build directory; on Windows and many developer setups that path includes spaces. Without quotes, the option parser splits the value and the server rejects the config. Please wrap the filename in quotes (and mirror the change in sibling CNFs).-eloq_config = @ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf +eloq_config = "@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf"storage/eloq/mysql-test/mono_basic/t/range_split_keycache.cnf (1)
18-18: Quoteeloq_configfor whitespace-safe pathsSame concern here: without quotes the value breaks when
MYSQLTEST_VARDIRcontains spaces. Please quote the assignment (and align any other copies in this series).-eloq_config = @ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/key_cache_ds.cnf +eloq_config = "@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/key_cache_ds.cnf"storage/eloq/mysql-test/mono_basic/t/mvcc_scan_sk.cnf (1)
14-14: Quoteeloq_configfor whitespace-safe pathsPlease wrap the value in quotes so configurations still load when the underlying path contains spaces.
-eloq_config = @ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf +eloq_config = "@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf"storage/eloq/mysql-test/mono_basic/t/mvcc_read_after_flushing_deleted_record.cnf (1)
14-14: Quoteeloq_configfor whitespace-safe pathsQuoting the path prevents MySQL from tokenizing it at embedded spaces—important for typical Windows builds.
-eloq_config = @ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf +eloq_config = "@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf"storage/eloq/mysql-test/mono_basic/t/mvcc_scan_after_flushing_deleted_record.cnf (1)
13-13: Quoteeloq_configfor whitespace-safe pathsSame fix applies: add quotes so this config value survives directories containing spaces.
-eloq_config = @ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf +eloq_config = "@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf"sql/mysqld.cc (1)
5266-5289: Don’t globally relax unknown‑option handling; gate it to library/converged mode.Always skipping/ignoring unknown options can silently mask typos in regular server runs.
- my_getopt_skip_unknown= TRUE; + /* Allow unknown options only in library/converged mode (so gflags can own them) */ +#if defined(MYSQLD_LIBRARY_MODE) || defined(BUILD_ELOQSQL_AS_LIBRARY) + my_getopt_skip_unknown= TRUE; +#else + my_getopt_skip_unknown= FALSE; +#endif @@ - // if (remaining_argc > 1) - // { - // fprintf(stderr, "%s: Too many arguments (first extra is '%s').\n", - // my_progname, remaining_argv[1]); - // unireg_abort(1); - // } + /* keep strict argv checking in non-library mode (see my_getopt_skip_unknown above) */Would you like me to wire this to the exact CMake defines used by the PR (BUILD_ELOQSQL_AS_LIBRARY)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (90)
concourse/scripts/pr.ent.bash(2 hunks)concourse/scripts/tag.sh(1 hunks)data_substrate(1 hunks)mysql-test/include/eloq_kv_dss.cnf(0 hunks)mysql-test/include/eloq_kv_keyspace.cnf(0 hunks)mysql-test/include/eloq_kv_storage.cnf(0 hunks)sql/CMakeLists.txt(10 hunks)sql/main.cc(2 hunks)sql/mysqld.cc(10 hunks)storage/eloq/eloq_db_dl.h(1 hunks)storage/eloq/eloq_system_handler.h(1 hunks)storage/eloq/eloqsql_catalog_factory.cpp(3 hunks)storage/eloq/eloqsql_catalog_factory.h(2 hunks)storage/eloq/ha_eloq.cc(16 hunks)storage/eloq/mysql-test/mono_basic/data_substrate.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/include/have_range_partition.inc(0 hunks)storage/eloq/mysql-test/mono_basic/my.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/alter_table.test(0 hunks)storage/eloq/mysql-test/mono_basic/t/alter_table_recovery.test(0 hunks)storage/eloq/mysql-test/mono_basic/t/create_drop_index.test(0 hunks)storage/eloq/mysql-test/mono_basic/t/data_store_error.test(0 hunks)storage/eloq/mysql-test/mono_basic/t/deadlock_handling.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/flush_undo_while_ckpt.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/flush_undo_while_ckpt_ds.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/isolation_level_for_locking.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/isolation_level_for_mvcc.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/isolation_level_for_occ.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/key_cache.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/key_cache_ds.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_archives_recycle.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_read_after_flushing_deleted_record.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_read_unique_sk_after_flushing_deleted_record.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_scan.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_scan_after_flushing_deleted_record.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_scan_sk.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_scan_sk_rc_isolvl.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/mvcc_unique_sk_lookup.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/poor_performance.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/poor_performance.test(0 hunks)storage/eloq/mysql-test/mono_basic/t/poor_performance_ds.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/range_split_keycache.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/range_split_keycache.test(0 hunks)storage/eloq/mysql-test/mono_basic/t/select_for_update_and_share.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/select_for_update_and_share_occ.cnf(0 hunks)storage/eloq/mysql-test/mono_basic/t/single_dead_lock_check.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/single_dead_lock_check_ds.cnf(1 hunks)storage/eloq/mysql-test/mono_basic/t/single_node.cnf(0 hunks)storage/eloq/mysql-test/mono_main/data_substrate.cnf(1 hunks)storage/eloq/mysql-test/mono_main/my.cnf(1 hunks)storage/eloq/mysql-test/mono_main/r/system_mysql_db_error_log.result(1 hunks)storage/eloq/mysql-test/mono_main/t/commit.opt(1 hunks)storage/eloq/mysql-test/mono_main/t/connect-abstract.cnf(1 hunks)storage/eloq/mysql-test/mono_main/t/connect.cnf(1 hunks)storage/eloq/mysql-test/mono_main/t/connect2.cnf(1 hunks)storage/eloq/mysql-test/mono_main/t/connect2.opt(1 hunks)storage/eloq/mysql-test/mono_main/t/consistent_snapshot.opt(1 hunks)storage/eloq/mysql-test/mono_main/t/pool_of_threads.cnf(1 hunks)storage/eloq/mysql-test/mono_main/t/pool_of_threads.opt(1 hunks)storage/eloq/mysql-test/mono_main/t/sp_notembedded.opt(1 hunks)storage/eloq/mysql-test/mono_main/t/subselect_exists2in_costmat.cnf(1 hunks)storage/eloq/mysql-test/mono_main/t/subselect_mat_cost.cnf(1 hunks)storage/eloq/mysql-test/mono_main/t/system_mysql_db_error_log.test(1 hunks)storage/eloq/mysql-test/mono_multi/data_substrate1.cnf(1 hunks)storage/eloq/mysql-test/mono_multi/data_substrate2.cnf(1 hunks)storage/eloq/mysql-test/mono_multi/data_substrate3.cnf(1 hunks)storage/eloq/mysql-test/mono_multi/my.cnf(1 hunks)storage/eloq/mysql-test/mono_multi/t/alter_schema_term_changed.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/check_blocked_request.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/check_blocked_request.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/create_drop_index.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/fault_create_drop_index.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk1.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk1.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk2.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk2.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/multi_dead_lock_check.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/multi_dead_lock_check.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/range_split.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_keycache.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_keycache.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_keycache.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_recovery.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_recovery.opt(1 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_recovery.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/range_split_write_forward.test(0 hunks)storage/eloq/mysql-test/mono_multi/t/tx_timeout.cnf(0 hunks)storage/eloq/mysql-test/mono_multi/t/tx_timeout.opt(1 hunks)
💤 Files with no reviewable changes (27)
- mysql-test/include/eloq_kv_keyspace.cnf
- storage/eloq/mysql-test/mono_basic/t/alter_table_recovery.test
- storage/eloq/mysql-test/mono_multi/t/fault_create_drop_index.test
- storage/eloq/mysql-test/mono_basic/t/alter_table.test
- storage/eloq/mysql-test/mono_multi/t/range_split_write_forward.test
- storage/eloq/mysql-test/mono_multi/t/check_blocked_request.cnf
- storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk1.cnf
- storage/eloq/mysql-test/mono_basic/t/select_for_update_and_share_occ.cnf
- storage/eloq/mysql-test/mono_basic/t/create_drop_index.test
- storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk2.cnf
- storage/eloq/mysql-test/mono_basic/t/poor_performance.test
- storage/eloq/mysql-test/mono_multi/t/alter_schema_term_changed.test
- storage/eloq/mysql-test/mono_basic/t/single_node.cnf
- mysql-test/include/eloq_kv_storage.cnf
- storage/eloq/mysql-test/mono_multi/t/tx_timeout.cnf
- storage/eloq/mysql-test/mono_basic/t/range_split_keycache.test
- storage/eloq/mysql-test/mono_multi/t/range_split.test
- storage/eloq/mysql-test/mono_multi/t/create_drop_index.test
- storage/eloq/mysql-test/mono_multi/t/range_split_keycache.cnf
- storage/eloq/mysql-test/mono_multi/t/multi_dead_lock_check.cnf
- storage/eloq/mysql-test/mono_basic/include/have_range_partition.inc
- storage/eloq/mysql-test/mono_basic/t/data_store_error.test
- storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo.cnf
- storage/eloq/mysql-test/mono_multi/t/range_split_recovery.cnf
- mysql-test/include/eloq_kv_dss.cnf
- storage/eloq/mysql-test/mono_multi/t/range_split_recovery.test
- storage/eloq/mysql-test/mono_multi/t/range_split_keycache.test
✅ Files skipped from review due to trivial changes (1)
- storage/eloq/mysql-test/mono_basic/t/key_cache_ds.cnf
🚧 Files skipped from review as they are similar to previous changes (29)
- storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk1.opt
- concourse/scripts/tag.sh
- storage/eloq/mysql-test/mono_multi/data_substrate2.cnf
- storage/eloq/mysql-test/mono_multi/t/check_blocked_request.opt
- storage/eloq/mysql-test/mono_multi/t/range_split_recovery.opt
- storage/eloq/mysql-test/mono_basic/data_substrate.cnf
- data_substrate
- storage/eloq/mysql-test/mono_multi/my.cnf
- storage/eloq/mysql-test/mono_main/data_substrate.cnf
- storage/eloq/mysql-test/mono_basic/t/poor_performance_ds.cnf
- sql/main.cc
- storage/eloq/mysql-test/mono_main/t/pool_of_threads.opt
- storage/eloq/mysql-test/mono_multi/t/multi_dead_lock_check.opt
- storage/eloq/mysql-test/mono_multi/t/tx_timeout.opt
- storage/eloq/mysql-test/mono_basic/t/single_dead_lock_check_ds.cnf
- storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo_sk2.opt
- storage/eloq/mysql-test/mono_multi/data_substrate3.cnf
- storage/eloq/mysql-test/mono_main/t/pool_of_threads.cnf
- storage/eloq/mysql-test/mono_main/t/sp_notembedded.opt
- storage/eloq/eloqsql_catalog_factory.cpp
- storage/eloq/mysql-test/mono_basic/t/isolation_level_for_occ.cnf
- storage/eloq/mysql-test/mono_basic/t/single_dead_lock_check.cnf
- storage/eloq/mysql-test/mono_main/t/subselect_mat_cost.cnf
- storage/eloq/mysql-test/mono_basic/t/flush_undo_while_ckpt_ds.cnf
- storage/eloq/mysql-test/mono_multi/t/range_split_keycache.opt
- storage/eloq/mysql-test/mono_basic/t/select_for_update_and_share.cnf
- storage/eloq/mysql-test/mono_main/t/connect2.opt
- storage/eloq/mysql-test/mono_multi/data_substrate1.cnf
- storage/eloq/mysql-test/mono_multi/t/improve_ckpt_flush_undo.opt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
📚 Learning: 2025-09-25T11:41:40.111Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.
Applied to files:
storage/eloq/eloqsql_catalog_factory.hstorage/eloq/mysql-test/mono_basic/t/mvcc_scan.cnfsql/CMakeLists.txtstorage/eloq/mysql-test/mono_main/my.cnfconcourse/scripts/pr.ent.bashstorage/eloq/ha_eloq.cc
🧬 Code graph analysis (1)
sql/mysqld.cc (1)
sql/sql_parse.cc (2)
bootstrap(982-1112)bootstrap(982-982)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (29)
concourse/scripts/pr.ent.bash (2)
37-46: Data substrate directory layout correctly refactored.The change from
storage/eloq/todata_substrate/is properly applied. The symlink creation foreloq_log_service(line 38) occurs before entering the directory, and PR branch handling (lines 40-45) follows a consistent pattern with the existingtx_servicelogic below. The ordering is correct.
185-199: Build step paths correctly updated for refactored structure.The
dss_serverandlog_serverbuild steps now correctly reference their new locations underdata_substrate/. The cmake configuration flags and relative path resolution (../) are appropriate for the directory structure. Both builds follow the expected mkdir-cmake-build-install pattern.storage/eloq/mysql-test/mono_basic/t/flush_undo_while_ckpt.cnf (1)
13-13: The referenced configuration file exists and the path resolves correctly. The fileflush_undo_while_ckpt_ds.cnfis present atstorage/eloq/mysql-test/mono_basic/t/as expected, confirming the relative path construction is valid.storage/eloq/mysql-test/mono_basic/my.cnf (2)
28-28: Good addition for consistency.Adding
MONO1_SOCKalongsideMONO1_PORTprovides a consistent interface for test scripts to access both the port and socket path for mysqld.1.
21-21: Path resolves correctly and follows established codebase pattern.The path in line 21 uses a consistent convention found throughout all eloq test configurations. The file
data_substrate.cnfexists at the expected location, and the@ENV.MYSQLTEST_VARDIR/../../../pattern is the standard approach used across 13+ test config files in mono_basic, mono_main, and mono_multi test suites. The path construction is correct and verified.storage/eloq/mysql-test/mono_main/t/consistent_snapshot.opt (1)
1-2: Naming inconsistency appears intentional based on codebase design patterns.After verifying the codebase, the flag naming is actually consistent with the design:
--enable_mvcc=trueis a general/core flag (category shared with--thread_pool_size,--core_number,--checkpointer_delay_seconds) — these have noeloq_prefix--eloq_cc_protocolis an Eloq-specific MySQL system variable (category shared with--eloq_config,--eloq_hm_bin_path) — these retain theeloq_prefixAll three verification points pass:
- Backend correctly handles
--enable_mvcc(code reads asGetCoreConfig().enable_mvccin ha_eloq.cc)- Value
trueis accepted (multiple test files successfully useenable_mvcc=true/falseformat; oldeloq_enable_mvcc=onis being migrated away)- Prefix inconsistency is intentional—it reflects flag categorization (core vs. Eloq-specific)
The change from
--eloq_enable_mvcc=onto--enable_mvcc=trueis an active migration pattern across the codebase, with both value formats working correctly.storage/eloq/mysql-test/mono_basic/t/mvcc_read_unique_sk_after_flushing_deleted_record.cnf (1)
14-14: LGTM! Configuration centralization is consistent.The changes align with the centralized configuration pattern, replacing per-instance settings with eloq_config reference and adding MONO1_SOCK to the environment.
Also applies to: 20-20
storage/eloq/mysql-test/mono_basic/t/isolation_level_for_mvcc.cnf (1)
12-12: LGTM! Configuration centralization is consistent.Also applies to: 18-18
storage/eloq/mysql-test/mono_basic/t/mvcc_archives_recycle.cnf (1)
15-15: LGTM! Configuration centralization is consistent.Also applies to: 21-21
storage/eloq/mysql-test/mono_basic/t/poor_performance.cnf (1)
18-18: No action required.The referenced configuration file
poor_performance_ds.cnfexists at the expected location:storage/eloq/mysql-test/mono_basic/t/poor_performance_ds.cnf. The path resolution in line 18 is correct.Likely an incorrect or invalid review comment.
storage/eloq/mysql-test/mono_basic/t/isolation_level_for_locking.cnf (1)
13-13: File exists and is correctly configured for locking-based isolation testing.Verification confirms
data_substrate.cnfexists and containsenable_mvcc=false, which is appropriate for locking-based (non-MVCC) isolation level testing. The configuration clearly differentiates from MVCC tests (which usemvcc_ds.cnfwithenable_mvcc=true). The deadlock detection settings also align with the locking-based protocol. No issues found.storage/eloq/mysql-test/mono_basic/t/mvcc_unique_sk_lookup.cnf (1)
14-14: No issues identified with the eloq_config path.The file
mvcc_ds.cnfexists atstorage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf. The relative path pattern@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnfis used consistently across multiple configuration files in the codebase (includingmono_main/my.cnfandmono_basic/my.cnf), indicating it's the standard and verified path resolution for this test framework setup.storage/eloq/mysql-test/mono_basic/t/key_cache.cnf (1)
21-21: ****The path construction is not a maintainability concern. The
@ENV.MYSQLTEST_VARDIR/../../../pattern is consistently used across 30+ CNF files throughout the Eloq test suite, and no alternative source/binary directory variables are used in any CNF file. This is an intentional, standardized approach across the test infrastructure, not a fragile one-off pattern. The referenced file exists and the implementation is consistent with established test configuration practices in the codebase.Likely an incorrect or invalid review comment.
sql/CMakeLists.txt (4)
121-127: Directory path standardization looks good.The consistent migration from CMAKE_{SOURCE,BINARY}DIR to ELOQSQL{SOURCE,BINARY}_DIR across INCLUDE_DIRECTORIES is well executed and aligns with the converged build refactoring.
388-399: Library export section for converged mode is well structured.The new conditional export of ELOQSQL_SQL_LIBRARY, ELOQSQL_SQL_BUILTINS_LIBRARY, and ELOQSQL_INCLUDE_DIRS correctly gates library-mode exports and provides the expected parent-level visibility for converged builds.
401-451: Conditional mariadbd executable wrapper is appropriate.The wrapping of the mariadbd executable target in
if(NOT BUILD_ELOQSQL_AS_LIBRARY)correctly handles the divergent build modes: library mode skips executable creation while standalone mode retains it.
215-215: The review concern is not applicable—header inclusion in SQL_SOURCE is established codebase practice.The
circular_queue.hinclusion follows the same pattern as other headers already in the SQL_SOURCE list (ddl_log.h,sql_sequence.h,ha_sequence.h,sql_tvc.h,rowid_filter.h). The file is a template class with inline implementation, which correctly requires a header-only model. Template classes must be instantiated at compile time in translation units that use them, making this fully compatible with static library compilation. Listing headers in CMakeLists.txt source lists is the established pattern in this codebase for dependency tracking and IDE visibility.Likely an incorrect or invalid review comment.
sql/mysqld.cc (2)
5528-5531: LGTM: shutdown hook is correct and minimal.Thin wrapper over break_connect_loop() is appropriate for library mode.
6006-6008: Pair allocations with frees (follow‑up to argv fix).If you adopt strdup for --defaults-file, remember to free it here.
See the diff in the argv construction comment adding:
+#ifdef WITH_GFLAGS + if (defaults_file_cstr) free(defaults_file_cstr); +#endifstorage/eloq/mysql-test/mono_main/my.cnf (1)
22-22: Centralized substrate config path looks good.Line 22 cleanly funnels the suite through the shared
data_substrate.cnf, which should simplify future tuning.storage/eloq/mysql-test/mono_main/t/system_mysql_db_error_log.test (1)
107-116: Updated WARNING patterns align with server output.The uppercase tokens in Lines 107-116 mirror the log severity format shift, keeping the test assertions in sync.
storage/eloq/mysql-test/mono_main/r/system_mysql_db_error_log.result (1)
115-118: Result expectations updated appropriately.Lines 115-118 now reflect the uppercase
WARNINGtokens, so the recorded output matches the revised test regexes.storage/eloq/mysql-test/mono_main/t/connect2.cnf (1)
12-24: Nice consistency with mono_main base config.Routing Line 12 through the shared
data_substrate.cnfand exportingMONO1_*keeps the connect2 scenario aligned with the new substrate setup.storage/eloq/eloqsql_catalog_factory.h (1)
27-35: DataSubstrate handoff is wired correctly.Lines 27-35 and 120-123 swap in the renamed headers and delegate catalog deserialization through
DataSubstrate::GetGlobal(), matching the converged ownership model.Also applies to: 120-123
storage/eloq/mysql-test/mono_main/t/connect-abstract.cnf (1)
12-25: Abstract socket and thread-pool config verified as correct.ABSTRACT_SOCKET is properly defined as
@mtr-test-abstract-socket-@OPT.portand correctly used inconnect-abstract.testwith$ABSTRACT_SOCKETsubstitution. MONO1_PORT and MONO1_SOCK are consistently applied across all test files. Thread pool sizing (size=2, oversubscribe=10) is appropriate for testing.storage/eloq/mysql-test/mono_main/t/subselect_exists2in_costmat.cnf (1)
17-30: Path is correct and follows established codebase pattern; review concern unsupported.The relative path
@ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_main/data_substrate.cnfis not brittle. This exact pattern is used consistently throughout the test suite (e.g.,storage/eloq/mysql-test/mono_multi/my.cnflines 23, 26, 29) and resolves correctly. The target file exists and the relative depth aligns with MTR's vardir conventions. No path failures or layout shifts are evident in the codebase. The secondary concerns aboutMONO1_SOCKandmysqldumpskip options lack specific evidence of coverage gaps.Likely an incorrect or invalid review comment.
storage/eloq/mysql-test/mono_main/t/connect.cnf (1)
11-17: Thread pool and config path are consistent with test suite patterns.The verification confirms:
thread_pool_size=2withthread_pool_oversubscribe=10is the standard baseline acrossmono_main,mono_basic, and other primary configs, with selective per-test overrides to size=1 intentional for specific tests.- The
eloq_configpath exists atstorage/eloq/mysql-test/mono_main/data_substrate.cnfand follows the same@ENV.MYSQLTEST_VARDIRpattern used consistently throughout the test suite.No issues identified.
storage/eloq/ha_eloq.cc (2)
1478-1485: Unable to verify InitializeGlobal signature—definition not found in repository; request manual verification.The review comment suggests adding error checking for
DataSubstrate::InitializeGlobal(eloq_config)at line 1484 in the plugin initialization functioneloq_init_func(). I found that:
eloq_configis declared asstatic char *eloq_config= nullptr;and can legitimately be nullptr- The function uses the
DBUG_RETURN()error-handling pattern witheloq_init_abort()returning 1 on failure- The context is plugin initialization, which should handle initialization failures
However, I could not locate the definition of
DataSubstrate::InitializeGlobal()in the repository through extensive searching, suggesting it resides in an external library or is conditionally compiled. Without access to the actual method signature, I cannot definitively confirm:
- Whether it returns a value that should be checked (bool vs void)
- Whether null
eloq_configis an actual error condition or valid input- Whether the suggested error-handling pattern aligns with the method's contract
The suggested fix is reasonable defensive programming, but verification requires examining the DataSubstrate library definition and confirming the initialization expectations.
1350-1356: Uncomment the MariaSystemHandler shutdown call to ensure proper cleanup ordering.The suggested fix is correct. The Shutdown() implementation (eloq_system_handler.cpp:89-99) guards against double-execution with
if (!shutdown_.load(...)), so calling it early in eloq_pre_shutdown() and again via the destructor is safe. Early shutdown ensures it runs while global_system_variables.table_plugin is still valid, before plugin_shutdown() tears it down. Leaving it commented risks the destructor executing after globals are invalidated, which could trigger the THD creation issue described in the comment.Apply the diff as suggested.
| ${SSL_LIBRARIES} | ||
| ${LIBSYSTEMD} | ||
| ${LIBSYSTEMD} | ||
| ${DATA_SUBSTRATE_LIB} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify DATA_SUBSTRATE_LIB variable definition.
The new DATA_SUBSTRATE_LIB variable is added to SQL_DEP_LIBS but its definition needs verification to ensure it's properly scoped and defined in all build modes (standalone vs. library).
🌐 Web query:
Where is DATA_SUBSTRATE_LIB defined in the CMake configuration?
💡 Result:
Which project or repository are you looking at? I need the CMakeLists.txt or repo name/path to locate where DATA_SUBSTRATE_LIB is defined.
I need to search the repository's CMakeLists.txt files to locate where DATA_SUBSTRATE_LIB is defined. Let me do that now.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for DATA_SUBSTRATE_LIB definition in CMakeLists.txt files
rg -n 'DATA_SUBSTRATE_LIB' --type cmake
Length of output: 46
The variable is referenced but not defined anywhere. Let me examine the context around line 282 in sql/CMakeLists.txt to understand the scope better.
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract context around line 282 in sql/CMakeLists.txt
sed -n '270,295p' sql/CMakeLists.txt
Length of output: 695
Let me search the entire repository for where DATA_SUBSTRATE_LIB might be defined, including parent directories and CMake modules.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search entire repo for DATA_SUBSTRATE_LIB set/definition
rg -n 'set\s*\(\s*DATA_SUBSTRATE_LIB' --type cmake
Length of output: 0
Let me check the root CMakeLists.txt and any included modules to see if DATA_SUBSTRATE_LIB might be defined there.
---
🏁 Script executed:
```shell
#!/bin/bash
# Look for root CMakeLists.txt and check for includes/modules
head -50 CMakeLists.txt
echo "---"
# Search for any DATA_SUBSTRATE references (broader search)
rg -i 'data_substrate\|data.substrate' --type cmake -n
Length of output: 1999
Let me search for where similar optional variables like LIBWRAP and LIBCRYPT are defined to understand the pattern.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where LIBWRAP or LIBCRYPT are defined
rg -n 'set\s*\(\s*LIBWRAP|set\s*\(\s*LIBCRYPT' --type cmake -A2
echo "---"
# Also check cmake modules directory structure
fd . cmake --type d 2>/dev/null | head -20
Length of output: 79
Let me search more broadly for LIBWRAP across all files to understand where platform variables come from, and look for any Find modules related to DATA_SUBSTRATE.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search broadly for LIBWRAP
rg -n 'LIBWRAP' --type cmake -C2 | head -30
echo "---"
# Look for Find modules related to data substrate or similar
fd 'Find.*cmake$' cmake --type f
echo "---"
# Check if DATA_SUBSTRATE_LIB appears in any recent git history
git log --oneline --all -S DATA_SUBSTRATE_LIB -- sql/CMakeLists.txt 2>/dev/null | head -5
Length of output: 1691
Found a relevant git commit! Let me examine it to see how DATA_SUBSTRATE is initialized.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the git commit that references data substrate
git show e0265af1c8 --stat 2>/dev/null | head -20
echo "---"
# Search for DATA_SUBSTRATE in all files
rg -n 'DATA_SUBSTRATE' --type-list | head -10
echo "---"
rg -n 'DATA_SUBSTRATE' -C3 2>/dev/null | head -50
Length of output: 1727
Let me search configure.cmake and the data_substrate build files for where DATA_SUBSTRATE_LIB should be defined.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search configure.cmake for DATA_SUBSTRATE
rg -n 'DATA_SUBSTRATE\|data.substrate\|data_substrate' configure.cmake -i -C3
echo "---"
# Check if there's a CMakeLists in data_substrate directory
find . -path '*/data_substrate*/CMakeLists.txt' -type f
echo "---"
# Check storage/eloq/CMakeLists.txt
if [ -f storage/eloq/CMakeLists.txt ]; then
grep -n 'DATA_SUBSTRATE_LIB\|DATA_SUBSTRATE' storage/eloq/CMakeLists.txt || echo "Not found in eloq"
fi
Length of output: 25
DATA_SUBSTRATE_LIB is undefined and must be properly initialized.
The variable is referenced at sql/CMakeLists.txt:282 but is not defined anywhere in the codebase. Following the pattern of similar optional dependencies like LIBWRAP (defined in configure.cmake:172), DATA_SUBSTRATE_LIB should be conditionally set during configuration. Without this definition, the CMake variable expands to an empty string, leaving the dependency list incomplete.
Define DATA_SUBSTRATE_LIB in the appropriate CMake configuration file, ensuring it's set based on the build configuration (standalone vs. library mode) similar to how LIBWRAP is initialized.
| #include <gflags/gflags.h> | ||
| #include "unireg.h" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard gflags/glog includes and include to fix portability/compile errors.
Wrap third‑party headers so non‑gflags/glog builds don’t break, and add for std::string usage.
-#include <gflags/gflags.h>
+#ifdef WITH_GFLAGS
+#include <gflags/gflags.h>
+#endif
+/* Needed for std::string used below */
+#include <string>
@@
-#include "glog/logging.h"
+#ifdef WITH_GLOG
+#include "glog/logging.h"
+#endifAlso applies to: 117-118
🤖 Prompt for AI Agents
In sql/mysqld.cc around lines 19-20 (and similarly at 117-118), the third-party
headers gflags/glog should be conditionally included so builds that don't enable
those libraries won't fail, and <string> must be included for std::string usage;
wrap the gflags/glog includes in preprocessor guards (e.g., #if
defined(HAVE_GFLAGS) / #endif or a project-specific ENABLE_GFLAGS/ENABLE_GLOG
macro) and add #include <string> unconditionally near the top of the file so
std::string is available regardless of whether gflags/glog are present.
| // Declare gflags | ||
| DEFINE_string(eloqsql_config, "", | ||
| "Path to MySQL configuration file. All MySQL options must be in this config file."); | ||
| DECLARE_bool(bootstrap); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Gate global gflags symbols behind WITH_GFLAGS.
Prevents undefined macro errors when gflags is not enabled.
-// Declare gflags
-DEFINE_string(eloqsql_config, "",
- "Path to MySQL configuration file. All MySQL options must be in this config file.");
-DECLARE_bool(bootstrap);
+#ifdef WITH_GFLAGS
+// gflags flags
+DEFINE_string(eloqsql_config, "",
+ "Path to server configuration file. All server options must be in this file.");
+DECLARE_bool(bootstrap);
+#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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Declare gflags | |
| DEFINE_string(eloqsql_config, "", | |
| "Path to MySQL configuration file. All MySQL options must be in this config file."); | |
| DECLARE_bool(bootstrap); | |
| #ifdef WITH_GFLAGS | |
| // gflags flags | |
| DEFINE_string(eloqsql_config, "", | |
| "Path to server configuration file. All server options must be in this file."); | |
| DECLARE_bool(bootstrap); | |
| #endif |
🤖 Prompt for AI Agents
In sql/mysqld.cc around lines 645 to 649, the global gflags symbols
(DEFINE_string and DECLARE_bool) must be compiled only when gflags is enabled;
wrap those lines with an #ifdef WITH_GFLAGS / #endif guard so the macros are not
emitted when WITH_GFLAGS is not defined (and ensure any necessary gflags headers
remain included under the same guard).
| ~MariaSystemHandler() | ||
| { | ||
| Shutdown(); | ||
| if (thd_.joinable()) | ||
| { | ||
| thd_.join(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Destructor-triggered Shutdown can race plugin teardown; guard and make idempotent.
Call Shutdown() earlier (see ha_eloq.cc pre_shutdown) and make the dtor a no-op if already shut down. Also mark dtor noexcept to avoid unwinding on teardown.
Apply this diff for safety in the header:
- ~MariaSystemHandler()
- {
- Shutdown();
- if (thd_.joinable())
- {
- thd_.join();
- }
- }
+ ~MariaSystemHandler() noexcept
+ {
+ // Ensure idempotence and avoid doing heavy work at DSO unload.
+ if (!shutdown_.load(std::memory_order_acquire)) {
+ Shutdown(); // expected to set shutdown_ true internally
+ }
+ if (thd_.joinable()) {
+ thd_.join();
+ }
+ }And ensure Shutdown() sets shutdown_ to true on first call.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ~MariaSystemHandler() | |
| { | |
| Shutdown(); | |
| if (thd_.joinable()) | |
| { | |
| thd_.join(); | |
| } | |
| } | |
| ~MariaSystemHandler() noexcept | |
| { | |
| // Ensure idempotence and avoid doing heavy work at DSO unload. | |
| if (!shutdown_.load(std::memory_order_acquire)) { | |
| Shutdown(); // expected to set shutdown_ true internally | |
| } | |
| if (thd_.joinable()) { | |
| thd_.join(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In storage/eloq/eloq_system_handler.h around lines 47 to 54, the destructor
currently calls Shutdown() which can race plugin teardown; change behavior so
the destructor is noexcept and becomes a no-op when already shut down: remove
the unconditional Shutdown() call and instead check a shutdown_ flag and only
join the thread if still joinable; ensure Shutdown() itself is made idempotent
by setting shutdown_ = true on first entry (guarded by a mutex/atomic) and
returning immediately on subsequent calls. Also ensure pre_shutdown (ha_eloq.cc)
invokes Shutdown() earlier in teardown so the dtor doesn’t need to perform
shutdown work.
| tx_service= DataSubstrate::GetGlobal()->GetTxService(); | ||
| txlog_server= DataSubstrate::GetGlobal()->GetLogServer(); | ||
| storage_hd= DataSubstrate::GetGlobal()->GetStoreHandler(); | ||
|
|
||
| if (idx == std::string::npos) | ||
| { | ||
| local_ip= local_ip_str; | ||
| local_port= 8000; | ||
| local_ip_str.append(":8000"); | ||
| } | ||
| else | ||
| { | ||
| local_ip= local_ip_str.substr(0, idx); | ||
| local_port= std::stoi(local_ip_str.substr(idx + 1)); | ||
| } | ||
|
|
||
| sql_print_information("MariaDB Node address: %s", local_ip_str.c_str()); | ||
| node_id= DataSubstrate::GetGlobal()->GetNetworkConfig().node_id; | ||
| #ifdef EXT_TX_PROC_ENABLED |
There was a problem hiding this comment.
Missing null-checks on DataSubstrate::GetGlobal().
If converged main failed or config is invalid, these derefs crash. Guard and abort init.
Apply this diff:
- tx_service= DataSubstrate::GetGlobal()->GetTxService();
- txlog_server= DataSubstrate::GetGlobal()->GetLogServer();
- storage_hd= DataSubstrate::GetGlobal()->GetStoreHandler();
+ auto* ds = DataSubstrate::GetGlobal();
+ if (ds == nullptr) {
+ LOG(ERROR) << "DataSubstrate global is null.";
+ DBUG_RETURN(eloq_init_abort());
+ }
+ tx_service = ds->GetTxService();
+ txlog_server = ds->GetLogServer();
+ storage_hd = ds->GetStoreHandler();
+ if (!tx_service || !storage_hd) {
+ LOG(ERROR) << "DataSubstrate components not initialized.";
+ DBUG_RETURN(eloq_init_abort());
+ }Also guard later uses, e.g., before GetCoreConfig() and GetNetworkConfig().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In storage/eloq/ha_eloq.cc around lines 1511-1516, the code dereferences
DataSubstrate::GetGlobal() without null-checks which can crash if initialization
failed or config is invalid; add a single DataSubstrate* ds =
DataSubstrate::GetGlobal(); check immediately after retrieving it, log a clear
error if ds is null, and abort/return from the init routine (or set appropriate
error state) to stop further initialization; replace direct chained calls
(GetTxService(), GetLogServer(), GetStoreHandler(), GetCoreConfig(),
GetNetworkConfig(), GetNetworkConfig().node_id, etc.) with uses of the guarded
ds pointer and ensure any later uses also check ds (or keep it in a local
variable) before calling GetCoreConfig()/GetNetworkConfig(), so nothing
dereferences a null pointer.
| transaction-isolation=READ-COMMITTED | ||
| eloq_deadlock_interval_sec=10 | ||
| eloq_hm_bin_path = @ENV.MYSQLTEST_VARDIR/../../storage/eloq/host_manager | ||
| eloq_config = @ENV.MYSQLTEST_VARDIR/../../../storage/eloq/mysql-test/mono_basic/data_substrate.cnf |
There was a problem hiding this comment.
Fix eloq_config path resolution
MYSQLTEST_VARDIR defaults to ./var under the mysql-test directory, so @ENV.MYSQLTEST_VARDIR/../../../… resolves to the parent of the source tree when you run the suite straight from the repo. In that case the server looks for storage/eloq/... one level too high and fails to load the substrate config. Please anchor the path to a stable source-root variable instead (e.g., @ENV.MYSQL_TEST_DIR/@ENV.ELOQSQL_SOURCE_DIR, or copy the config into the vardir) so the test works for both in-source and out-of-source runs.(mariadb.com)
🤖 Prompt for AI Agents
In storage/eloq/mysql-test/mono_basic/t/deadlock_handling.cnf around line 14,
the eloq_config uses a relative path via @ENV.MYSQLTEST_VARDIR/../../../ which
resolves outside the source tree for in-repo runs and causes the substrate
config to be looked up one level too high; replace this relative traversal with
a stable source-root environment variable (e.g., use @ENV.MYSQL_TEST_DIR or
@ENV.ELOQSQL_SOURCE_DIR) to point directly to
storage/eloq/mysql-test/mono_basic/data_substrate.cnf, or alternatively copy the
config into the test vardir and reference it from @ENV.MYSQLTEST_VARDIR to
ensure the test works for both in-source and out-of-source runs.
| enable_mvcc=true | ||
| deadlock_check_interval_seconds=10 | ||
| txlog_rocksdb_scan_threads=3 | ||
| hm_bin_path=../../../../data_substrate/host_manager No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the hm_bin_path relative path resolves correctly.
The path ../../../../data_substrate/host_manager goes up 4 levels from storage/eloq/mysql-test/mono_basic/t/, which would resolve to storage/data_substrate/host_manager. However, the PR summary indicates data_substrate is introduced as a submodule at the repository root. If data_substrate is at the root, the path should be ../../../../../data_substrate/host_manager (5 levels up) instead.
🏁 Script executed:
#!/bin/bash
# Verify data_substrate location in the repository
fd -t d "^data_substrate$" . --max-depth 2
# Check if host_manager exists under data_substrate
fd -t f "host_manager" data_substrate --max-depth 2Length of output: 17
Need to verify the structure of data_substrate to determine the correct path:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check directory structure within data_substrate
find ./data_substrate -maxdepth 2 -type d | head -20
Length of output: 51
---
🏁 Script executed:
```shell
#!/bin/bash
# Check directory structure within data_substrate
find ./data_substrate -maxdepth 3 -type d | sort
Length of output: 16
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if data_substrate is a git submodule and list all files in it
git -C ./data_substrate config --file .gitmodules --get-regexp path || echo "Not a submodule at root level"
find ./data_substrate -type f
# Also check .gitmodules at root
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"
Length of output: 1268
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for host_manager in the repository
find . -name "*host_manager*" -type d 2>/dev/null
find . -name "*host_manager*" -type f 2>/dev/null
Length of output: 0
Fix the relative path: use ../../../../../data_substrate/host_manager instead of ../../../../data_substrate/host_manager.
The path ../../../../data_substrate/host_manager goes up 4 levels from storage/eloq/mysql-test/mono_basic/t/, which resolves to storage/data_substrate/host_manager. Since data_substrate is a git submodule at the repository root, you need 5 levels up to reach it: ../../../../../data_substrate/host_manager.
🤖 Prompt for AI Agents
In storage/eloq/mysql-test/mono_basic/t/mvcc_ds.cnf around line 6, the relative
path hm_bin_path currently points to ../../../../data_substrate/host_manager
which climbs only four levels and resolves to
storage/data_substrate/host_manager; update the path to
../../../../../data_substrate/host_manager (five levels up) so it correctly
resolves to the data_substrate submodule at the repository root; replace the
existing hm_bin_path value with the corrected relative path.
| --enable_mvcc=true | ||
| --eloq_cc_protocol=OccRead |
There was a problem hiding this comment.
🧩 Analysis chain
Verify flag naming consistency and backend compatibility.
This file exhibits the same pattern as consistent_snapshot.opt: the MVCC flag changed from --eloq_enable_mvcc=on to --enable_mvcc=true (removing the eloq_ prefix and changing the value), while --eloq_cc_protocol retains its prefix. This naming inconsistency appears across multiple test configuration files.
Please verify that:
- All test configurations using the new
--enable_mvcc=trueflag are supported by the backend - The prefix inconsistency is intentional and doesn't indicate incomplete refactoring
- No other test files still reference the old
--eloq_enable_mvccflag that should also be updated
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to the old flag format
# to ensure the migration is complete
echo "=== Finding all .opt files that might need updating ==="
fd -e opt -x cat {} \; -x echo "File: {}" | rg -C1 'eloq_enable_mvcc|enable_mvcc'
echo -e "\n=== Checking for mixed usage patterns ==="
fd -e opt -x grep -l 'eloq_enable_mvcc'Length of output: 1285
I'll search for more context about the flag naming to understand if this is an intentional, completed migration and verify backend compatibility.The web search results are about generic MVCC concepts and H2 database, but don't specifically address the "eloq" backend being used in the repository. Let me search specifically for the eloq backend and its flag configurations.
</script>🏁 Script executed:
#!/bin/bash
# Search for documentation, comments, or backend code that explains the flag naming
echo "=== Looking for documentation or comments about the flags ==="
rg -i 'enable_mvcc|eloq_enable_mvcc' --type=md --type=txt --type=sql -C2
echo -e "\n=== Searching for flag parsing or configuration handling ==="
rg -i 'enable_mvcc' --type=cc --type=h --type=cpp -C2
echo -e "\n=== Checking for changelog or release notes mentioning the flag change ==="
fd -e md -e txt -i 'change|release|readme|migrat' | head -20 | xargs grep -l 'mvcc\|enable_mvcc' 2>/dev/null || echo "No matches found"
echo -e "\n=== Total count of .opt files with enable_mvcc (new format) ==="
fd -e opt -x grep -l 'enable_mvcc' | wc -l
echo -e "\n=== Total count of .opt files with eloq_cc_protocol ==="
fd -e opt -x grep -l 'eloq_cc_protocol' | wc -lLength of output: 369
🏁 Script executed:
#!/bin/bash
# Search for backend configuration, documentation, and flag definitions
echo "=== Looking for backend flag parsing or configuration ==="
rg -i 'eloq_cc_protocol|enable_mvcc' --type=cc --type=h --type=cpp --type=sql -C2 2>/dev/null | head -50
echo -e "\n=== Searching for CMakeLists, build files, or config with flag info ==="
fd -e cmake -e cfg -e conf -e ini | xargs grep -l 'mvcc\|eloq' 2>/dev/null || echo "No config files found"
echo -e "\n=== Look for driver or backend initialization code ==="
rg -i 'eloq_enable_mvcc|enable_mvcc.*=|OccRead|Locking' --type=cc --type=h -C2 2>/dev/null | head -40
echo -e "\n=== Counting total references to each flag format ==="
echo "Old format (eloq_enable_mvcc): $(rg 'eloq_enable_mvcc' | wc -l)"
echo "New format (enable_mvcc=true): $(rg 'enable_mvcc=true' | wc -l)"
echo "Files with eloq_cc_protocol: $(rg 'eloq_cc_protocol' | wc -l)"Length of output: 372
🏁 Script executed:
#!/bin/bash
# Locate the remaining references to the old flag format
echo "=== Finding exact locations of remaining eloq_enable_mvcc references ==="
rg 'eloq_enable_mvcc' -n -C3
echo -e "\n=== File names containing old flag ==="
rg 'eloq_enable_mvcc' -lLength of output: 1256
🏁 Script executed:
#!/bin/bash
# Check if any .cnf files have been updated to the new format
echo "=== Checking all .cnf files for either flag format ==="
echo "Files with old format (eloq_enable_mvcc):"
fd -e cnf -x grep -l 'eloq_enable_mvcc' | sort
echo ""
echo "Files with new format (enable_mvcc=true):"
fd -e cnf -x grep -l 'enable_mvcc=true' | sort
echo ""
echo "=== Total .cnf files in storage/eloq ==="
fd -path '*/storage/eloq/**' -e cnf | wc -l
echo -e "\n=== Check if migrate.opt or other .opt files exist that haven't been updated ==="
fd -e opt -x grep -l 'eloq_enable_mvcc'Length of output: 652
Complete the MVCC flag migration in remaining configuration files.
The migration of --eloq_enable_mvcc → --enable_mvcc=true is incomplete across the codebase. While all .opt test files have been successfully updated (including commit.opt), the following .cnf configuration files still reference the old flag format and should be updated for consistency:
storage/eloq/mysql-test/mono_basic/t/mvcc_scan_sk_rc_isolvl.cnf(line 16):eloq_enable_mvcc=on→enable_mvcc=trueconcourse/scripts/my_range.cnf(line 23):eloq_enable_mvcc=off→enable_mvcc=false
Other .cnf files have already been migrated (flush_undo_while_ckpt_ds.cnf and mvcc_ds.cnf), confirming the pattern is intentional. The naming inconsistency (where eloq_cc_protocol retains the prefix while enable_mvcc does not) appears deliberate—protocol-specific settings keep the prefix, while generic MVCC control does not.
🤖 Prompt for AI Agents
In storage/eloq/mysql-test/mono_basic/t/mvcc_scan_sk_rc_isolvl.cnf around line
16 and concourse/scripts/my_range.cnf around line 23 the old flag name
`eloq_enable_mvcc` is still used; replace `eloq_enable_mvcc=on` with
`enable_mvcc=true` and `eloq_enable_mvcc=off` with `enable_mvcc=false`
respectively so the `.cnf` files match the migrated `.opt` naming; keep
`eloq_cc_protocol` unchanged.
1 participant
Summary by CodeRabbit
New Features
Chores
Note
Integrates the new data_substrate across server and storage, adds a library build mode, and updates build/test/scripts to the new paths and config flow.
BUILD_ELOQSQL_AS_LIBRARYto build eloqsql as a library; exportsql/sql_builtinsand makemariadbdoptional.${CMAKE_SOURCE_DIR}usages with${ELOQSQL_SOURCE_DIR}/${ELOQSQL_BINARY_DIR}; update install/CPack, ABI, dtrace, RPM/DEB packaging, tags, and tests.WITH_DYNAMO_DBoption.storage/eloq/*submodules withdata_substrate/*submodules; adddata_substratesubmodule.sqlwithdata_substrate; addDATA_SUBSTRATE_LIBto deps.DataSubstrateinmysqld/storage; introduce--eloqsql_configand gflags-based parsing.DataSubstrateAPIs; rename internal headers toeloqsql_*.sql/circular_queue.h.data_substratepaths and centralized config files; remove obsolete eloq-specific test includes..cursor/to.gitignore.Written by Cursor Bugbot for commit f5fb5aa. This will update automatically on new commits. Configure here.