Conversation
xxsc0529
self-requested a review
September 14, 2026 08:45
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical generated-Tcl injection risk and two moderate lifecycle/configuration issues must be addressed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an independent OceanBase category for MySQL-compatible tenants with mode dispatch, shared MySQL workloads, tenant configuration, counters, migration support, documentation, and tests.
Changes:
- Adds OceanBase CLI, GUI, configuration, and workload integration.
- Adds tenant-aware sessions, counters, adapters, and validation.
- Adds regression tests, documentation, and generated workflows.
Review findings:
- Critical —
src/oceanbase/mysql/adapter.tcl:3: Generated Tcl values require escaping or stricter validation. - Moderate —
src/oceanbase/obopt.tcl:35: Counter options do not wire generic transaction-counter settings. - Moderate —
scripts/tcl/oceanbase/mysql/config.tcl:13: Virtual users must be destroyed before propagating phase failures.
File summaries
| File | Reviewed change |
|---|---|
tests/oceanbase.tcl |
Tests dispatch, counters, login, and configuration. |
tests/oceanbase-generated.tcl |
Tests generated-driver behavior. |
src/oceanbase/obotc.tcl |
OceanBase transaction-counter entry point. |
src/oceanbase/obopt.tcl |
Backend registry, dispatch, and validation. |
src/oceanbase/oboltp.tcl |
OceanBase TPROC-C entry points. |
src/oceanbase/obolap.tcl |
OceanBase TPROC-H entry points. |
src/oceanbase/obmet.tcl |
Metrics scope documentation. |
src/oceanbase/obci.tcl |
CI and external-installation handling. |
src/oceanbase/mysql/adapter.tcl |
MySQL tenant adaptation and options. |
src/mysql/mysqlotc.tcl |
Tenant-aware counter and session support. |
src/mysql/mysqloltp.tcl |
TPROC-C initialization and fixes. |
src/mysql/mysqlolap.tcl |
TPROC-H date, schema, and session fixes. |
src/generic/geninitws.tcl |
Web-service registration and migration. |
src/generic/geninitcli.tcl |
CLI registration and migration. |
src/generic/geninit.tcl |
GUI registration and migration. |
src/generic/gengen.tcl |
OceanBase data-generation mapping. |
scripts/tcl/oceanbase/mysql/tproch.tcl |
TPROC-H CLI workflow. |
scripts/tcl/oceanbase/mysql/tprocc.tcl |
TPROC-C CLI workflow. |
scripts/tcl/oceanbase/mysql/config.tcl |
Shared CLI configuration and phase checks. |
modules/xml-1.1.tm |
Empty XML field parsing. |
modules/mysqlcommon-1.0.tm |
Shared runtime timeout and counter support. |
modules/jobs-1.0.tm |
OceanBase chart styling. |
DocBook/oceanbase-mysql.md |
Usage and validation documentation. |
config/oceanbase.xml |
OceanBase defaults. |
config/database.xml |
OceanBase category definition. |
Review details
Suppressed comments (2)
scripts/tcl/oceanbase/mysql/config.tcl:13
- If any virtual user reports
FINISH FAILED, thiserrorexits the helper before thevudestroyon line 15 runs. Every generated phase calls this helper, so a failed build/check/run can leave virtual-user threads and database connections alive after the script aborts. Collect the failure, destroy the VUs, and then raise the saved error.
if {$status ne "FINISH SUCCESS"} {error "$phase failed: virtual user $vu ($status)"}
src/oceanbase/obopt.tcl:35
countoptsroutes the GUI Transaction Counter Options action tocountobopts, but this implementation calls the workloadoptionsUI withall; that UI only renders OceanBase connection fields and savesconfigoceanbase, never reading or updating the generictransaction_countersettings (tc_refresh_rate, logging flags) that the counter actually uses. Consequently OceanBase users cannot configure the counter from the GUI and get a misleading options dialog. Please provide a counter-specific implementation (or adapt the existing MySQL counter dialog to the OceanBase configuration) and keep the generic counter settings wired through.
proc countobopts {bm} {oceanbase::dispatch options [expr {$bm eq "TPC-C" ? "tpcc" : "tpch"}] all}
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3
to
+5
| proc username {user tenant cluster} { | ||
| foreach part [list $user $tenant $cluster] { | ||
| if {[regexp {[@#\s]} $part]} {error "Use separate user, tenant and cluster fields without @, # or whitespace"} |
xxsc0529
approved these changes
Sep 14, 2026
jackysp
force-pushed
the
codex/oceanbase-mysql
branch
from
September 14, 2026 09:15
e29f573 to
5f1efa3
Compare
jackysp
pushed a commit
that referenced
this pull request
Sep 25, 2026
) vsqlmet aliased connect_to_mysql to the MySQL implementation, which reads configmysql + mysql_* connection vars + check_mysql_ssl. So the Metrics dialog showed vsql_* values but then connected using MySQL config (functional bug per PR TPC-Council#920 review point 1). Fix: exclude connect_to_mysql from the blanket alias and provide a VillageSQL version that reads configvillagesql, vsql_* vars and check_vsql_ssl, then hands off to the shared (provider-agnostic) thread/logon/ASH machinery via the public(...) array. All other metrics procs remain shared with MySQL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on the standalone MySQL bug fixes in TPC-Council/HammerDB #926. This PR is temporarily based on
codex/mysql-no-sp-fixesso those fixes are excluded from the OceanBase adaptation diff. Retarget tomasterafter the fixes are incorporated into the fork.Changes
Add an independent OceanBase database category for MySQL-compatible tenants, covering TPROC-C and TPROC-H while retaining the legacy OceanBase option under MySQL. The CLI selects the category with
dbset db ob.Category entry points dispatch generation, counters, options and validation through
ob_compatibility_mode. MySQL-specific configuration mapping, login construction and workload integration live in a separate adapter. Existing configurations migrate tomysql; unsupported modes, includingoracle, fail explicitly. A future Oracle adapter can reuse the Oracle generators without changing the category entry points.Reuse the existing MySQL workloads with isolated configuration and tenant-session query timeouts. Add tenant-local OceanBase transaction counters shared by timed TPROC-C and its chart. Adapt H abbreviated-month parsing and case-insensitive table lookup, and propagate schema-loader failures. Generic no-procedure transaction and TPROC-H scale-check fixes are reviewed separately in TPC-Council#926. Include CLI examples, migration support and regression tests. Empty optional fields are supplied by an OceanBase-only configuration normalizer; the shared XML parser is unchanged. Offline C/H data generation obtains its format from the selected tenant adapter through a prefix-based callback.
Validation
Tested with the v6.0 runtime on Linux x86-64 against OceanBase Enterprise 4.3.5.6 and OBProxy 4.4.1.0:
OB TPM includes other activity in the same tenant and is not directly equivalent to MySQL statement counters. These runs validate compatibility, not performance.
Offline data-generation regression also passed on AWS: one C warehouse (9 files) and H SF1 (8 files, 1,500,000 orders and 5,998,868 line items), with table row counts and MySQL date-time formatting checked. Fresh and persisted OceanBase defaults and rejection of unsupported Oracle data generation passed.
Remaining validation
Complete GUI startup, Windows execution, TLS, native MySQL server regression, large/multi-node schemas and schema deletion remain unverified. The Linux GUI test was blocked by the repackaged runtime's embedded console initialization. The bundled Linux MySQL client emitted a non-fatal character-set 45 warning during statistics collection; schema and consistency checks passed.
This PR targets
oceanbase/HammerDB:codex/mysql-no-sp-fixesfor review in our fork. It does not implement Oracle tenant support or claim complete cross-platform validation.