fix eloqdb compile

[liunyl]

Summary by CodeRabbit

• Chores

  • Updated internal submodule pointer and build configuration to support library-mode initialization and improved handling of configuration files when embedded.

• Bug Fixes

  • Explicitly set the default insert semantic to "insert" to clarify insertion behavior.

• Style

  • Various formatting and minor non-functional cleanups; no other behavior changes.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Updates include a data_substrate submodule pointer bump, build flag changes enabling MYSQLD_LIBRARY_MODE for library builds, conditional library-mode startup argv handling in sql/mysqld.cc, and several minor formatting and one default-value change in storage source files.

Changes

Cohort / File(s)	Summary
Submodule Update data_substrate	Submodule pointer updated from 4df2f155a074d86594c5fe8075407efe284cb014 to 2e1fb679e115452b2d92fd05fe06fe78e599ce45.
Build Configuration storage/eloq/CMakeLists.txt	Added conditional compile definition MYSQLD_LIBRARY_MODE when BUILD_ELOQSQL_AS_LIBRARY is enabled. Removed prior USE_ONE_DATA_STORE_SHARD and METRICS_LIB lines.
Library-mode startup logic sql/mysqld.cc	Changed DEFINE_string(eloqsql_config, ...) usage site to DECLARE_string(eloqsql_config). Added #ifdef MYSQLD_LIBRARY_MODE guards around assembly of load_default_argv to conditionally append --defaults-file (from FLAGS_eloqsql_config) and --bootstrap. Added null-check/error handling for malloc of load_default_argv.
Formatting-only edits storage/eloq/eloq_db.cpp, storage/eloq/eloq_db_dl.h, storage/eloq/eloqsql_catalog_factory.cpp	Rewrapping/line-break and comment formatting changes only; no behavioral or signature changes.
System variable default change storage/eloq/ha_eloq.cc	Added explicit default value "insert" to insert_semantic system variable declaration (behavior already treated non-"upsert" as insert).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Areas to focus on:
  • storage/eloq/CMakeLists.txt — ensure removed defines (USE_ONE_DATA_STORE_SHARD, METRICS_LIB) are not required by other targets.
  • sql/mysqld.cc — validate library-mode #ifdef logic and load_default_argv construction, and that non-library behavior remains unchanged.
  • Submodule update — confirm downstream compatibility with the new data_substrate commit.

Suggested reviewers

• lzxddz
• xiexiaoy

Poem

🐰
In burrows of code I hop and cheer,
A default set, a flag now near,
Submodule nudged, and library awakes,
Tiny format hops, no big mistakes,
I nibble bytes and celebrate these makes! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'fix eloqdb compile' is vague and generic, using non-descriptive terms that don't convey meaningful information about the changeset's specific improvements.	Replace with a more specific title describing the actual changes, such as 'Add MYSQLD_LIBRARY_MODE support and update compilation flags' or 'Refactor MySQL config handling for library mode'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch eloqdb

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aca72b6 and 26f81b0.

📒 Files selected for processing (4)

• storage/eloq/eloq_db.cpp (2 hunks)
• storage/eloq/eloq_db_dl.h (1 hunks)
• storage/eloq/eloqsql_catalog_factory.cpp (1 hunks)
• storage/eloq/ha_eloq.cc (3 hunks)

✅ Files skipped from review due to trivial changes (2)

• storage/eloq/eloqsql_catalog_factory.cpp
• storage/eloq/eloq_db_dl.h

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.

📚 Learning: 2025-11-07T07:10:40.633Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.

Applied to files:

• storage/eloq/eloq_db.cpp

🔇 Additional comments (3)

storage/eloq/eloq_db.cpp (1)

398-401: UpsertDatabase call reformatting is safe

Only line-wrapping changed; the argument list and control flow are identical, so there’s no behavioral or ABI impact here.

Also applies to: 441-444

storage/eloq/ha_eloq.cc (2)

284-287: Config sysvar definition and registration look correct; double‑check empty‑string semantics

Defining config as a MYSQL_SYSVAR_STR with PLUGIN_VAR_MEMALLOC and a default of "" aligns it with the other string sysvars (e.g., auto_increment), and adding MYSQL_SYSVAR(config) to eloq_system_variables is necessary so the variable is exposed and usable. No compile‑ or runtime‑safety issues evident.

One thing to confirm: eloq_config is passed to DataSubstrate::InitializeGlobal(eloq_config) later in eloq_init_func. If that API distinguishes between nullptr and "", please make sure that an empty default still triggers the intended “use default config” behavior rather than “use an explicitly empty path”.

Also applies to: 428-443

---

1447-1454: Standalone data substrate path comment matches behavior

The updated comment correctly documents that in standalone (non‑MYSQLD_LIBRARY_MODE) builds you set FLAGS_eloq_data_path to mysql_real_data_home_ptr before calling DataSubstrate::InitializeGlobal(eloq_config). This matches the code and clarifies the intended default pathing.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

sql/mysqld.cc (1)

5567-5608: Argv wrapper for load_defaults_or_exit looks correct; a couple of minor nits

The new load_default_argv construction preserves behavior for normal binaries and adds the desired library-mode injection of --defaults-file=... and --bootstrap before load_defaults_or_exit, with correct sizing and null termination. A few optional refinements:

• The comment “append --eloqsql_config and --bootstrap” is slightly misleading; the code actually appends --defaults-file=<eloqsql_config> and --bootstrap. Updating the comment would prevent confusion.
• defaults_file_arg.c_str() is passed via const_cast<char*>. Today load_defaults_or_exit only reads these strings, but if that ever changed to modify them in-place it would be undefined behavior. If you want extra safety, you could allocate a writable buffer (e.g. via my_strdup() or a small std::vector<char>) for that entry.

These are polish-level; functionally the block is fine.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3f68f5 and aca72b6.

📒 Files selected for processing (3)

• data_substrate (1 hunks)
• sql/mysqld.cc (4 hunks)
• storage/eloq/CMakeLists.txt (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.

📚 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/CMakeLists.txt

📚 Learning: 2025-11-07T07:10:40.633Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.

Applied to files:

• storage/eloq/CMakeLists.txt

📚 Learning: 2025-11-07T05:12:43.208Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2770-2781
Timestamp: 2025-11-07T05:12:43.208Z
Learning: In eloqdata/eloqsql, storage/eloq/ha_eloq.cc (KV_ELOQDS path): for DATA_STORE_TYPE_ELOQDSS_ROCKSDB (local RocksDB), each eloqsql instance owns its own RocksDB instance, and passing create_if_missing=true to DataStoreService::StartService is intentional and safe.

Applied to files:

• storage/eloq/CMakeLists.txt

🔇 Additional comments (3)

data_substrate (1)

1-1: Provide context for submodule pointer update and verify the related changes.

This file only shows a submodule pointer update. To properly review this change, I need:

1. Context on the actual changes: The AI summary mentions this PR also updates sql/mysqld.cc and storage/eloq/CMakeLists.txt to support MYSQLD_LIBRARY_MODE, but those files are not provided for review.

2. Verification of the submodule commit: Confirm that commit 2e1fb679e115452b2d92fd05fe06fe78e599ce45 in the data_substrate repository contains the intended changes that align with the PR objective ("fix eloqdb compile").

Please provide or clarify:

• The changes in sql/mysqld.cc and storage/eloq/CMakeLists.txt related to MYSQLD_LIBRARY_MODE support
• A summary of what was changed in the data_substrate submodule at the new commit to fix the compilation issue
• Any build logs or specific compilation errors that this PR resolves

storage/eloq/CMakeLists.txt (1)

24-26: Based on my verification:

1. BUILD_ELOQSQL_AS_LIBRARY is properly defined: It's declared as an option() in the root CMakeLists.txt with default OFF, and correctly propagates to subdirectories like storage/eloq/.

2. MYSQLD_LIBRARY_MODE usage is guarded: The define is consistently used with #ifdef MYSQLD_LIBRARY_MODE guards in source files (storage/eloq/ha_eloq.cc, sql/mysqld.cc, sql/main.cc), ensuring conditional compilation works correctly.

3. Redundant definition: The root CMakeLists.txt already defines MYSQLD_LIBRARY_MODE under the same condition. The definition in storage/eloq/CMakeLists.txt lines 24-26 is redundant but harmless.

The conditional logic is sound and will function correctly. The original review concern about verification is unnecessary—the variable is properly defined upstream and the implementation is correct.

sql/mysqld.cc (1)

645-648: Switching to DECLARE_string for eloqsql_config is appropriate

Using DECLARE_string(eloqsql_config) here (with a single DEFINE_string in another translation unit) is the right pattern for gflags flags in shared/library builds and avoids multiple-definition/link issues. No further changes needed around this declaration.