fix: isolate shared schema DB operations

[srstack]

Summary

• run shared schema DDL and advisory-lock waits on a short-lived shared_schema DB handle
• retain the long-lived shared handle exclusively for shared tenant data-plane traffic
• add role/pool-limit coverage and assert schema DDL does not enter the shared data-plane series

Verification

• make test TEST_PKGS="./pkg/mysqlutil/... ./pkg/metrics/... ./pkg/tenant/schema/... ./pkg/tenant/..."
• make lint
• make build-server

No docs/ files are included.

Summary by CodeRabbit

• New Features

  • Added dedicated shared-schema database handling for schema initialization and migrations.
  • Added separate connection-pool settings and environment configuration for shared-schema operations.
  • Improved monitoring by reporting schema setup activity separately from regular database traffic.

• Bug Fixes

  • Prevented schema initialization waits and operations from being attributed to normal shared database activity.
  • Added safeguards against unsupported multi-statement connection configurations.

[coderabbitai]

Warning

Review limit reached

@srstack, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 363e5639-cd63-4fb7-9d02-abac397b69c2

📥 Commits

Reviewing files that changed from the base of the PR and between f9dea74 and 7fa8ce3.

📒 Files selected for processing (3)

• pkg/mysqlutil/pool.go
• pkg/mysqlutil/pool_test.go
• pkg/tenant/schema/shared_test.go

📝 Walkthrough

Walkthrough

Adds a dedicated shared_schema MySQL role for shared-schema DDL, with separate registration, pool defaults, environment overrides, initialization flow, schema-version locking, and metrics validation. Schema bootstrap now completes on a short-lived schema handle before the shared data-plane handle opens.

Changes

Shared Schema Database Role

Layer / File(s)	Summary
Shared schema role registration pkg/mysqlutil/instrumented.go, pkg/metrics/db.go	Adds the RoleSharedSchema constant, instrumented opener, and metrics registration under the shared_schema role.
Shared schema pool configuration pkg/mysqlutil/pool.go, pkg/mysqlutil/pool_test.go	Adds dedicated lifetime, pool-limit, and DRIVE9_SHARED_SCHEMA_DB_* environment settings with test coverage.
Dedicated schema initialization handle pkg/tenant/schema/shared.go, pkg/tenant/schema/shared_test.go	Uses an instrumented shared-schema handle for initialization and verifies Prometheus operations are attributed to shared_schema rather than shared.
Pre-data-plane schema bootstrap pkg/tenant/pool.go	Performs locked schema reconciliation on the dedicated schema handle before opening the shared data-plane handle.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Pool
  participant OpenSharedSchemaDB
  participant SchemaDB
  participant MetaStore
  participant SharedDB
  Pool->>OpenSharedSchemaDB: open dedicated shared_schema handle
  OpenSharedSchemaDB->>SchemaDB: acquire advisory lock and apply schema
  SchemaDB->>MetaStore: update SchemaVersion
  Pool->>SharedDB: open shared data-plane handle

Loading

Possibly related PRs

• mem9-ai/drive9#753: Adds related shared-role pooling and environment-key handling.
• mem9-ai/drive9#808: Changes shared-schema ensure and advisory-lock coordination.

Suggested reviewers: copilot, mornyx

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the core change: isolating shared schema database operations.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/shared-schema-observability

---

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.}

[Copilot]

Pull request overview

This PR isolates shared-schema bootstrap/DDL and advisory-lock waits onto a dedicated, short-lived shared_schema DB handle so shared data-plane (role="shared") metrics and latency alerts aren’t polluted by multi-second schema operations.

Changes:

• Add a new instrumented DB role (shared_schema) and registration path so shared-schema work is measured separately from shared tenant traffic.
• Update shared DB pool open flow to ensure schema (and advisory-lock waits) via a dedicated schema handle before opening the long-lived shared data-plane handle.
• Add tests covering the new role’s pool defaults/env overrides and asserting schema DDL does not increment the role="shared" operations series.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/tenant/schema/shared.go	Adds OpenSharedSchemaDB and switches shared schema init to use a dedicated shared-schema handle/role.
pkg/tenant/schema/shared_test.go	Adds a metrics contract test to ensure shared-schema DDL is attributed to role="shared_schema" and not role="shared".
pkg/tenant/pool.go	Reworks shared pool open path to run advisory-lock waits + schema ensure on a dedicated schema handle before opening the shared data-plane pool.
pkg/mysqlutil/pool.go	Introduces default pool limits/lifetimes and env var keys for the new RoleSharedSchema.
pkg/mysqlutil/pool_test.go	Adds coverage for shared-schema pool defaults and env override isolation from shared data-plane knobs.
pkg/mysqlutil/instrumented.go	Adds RoleSharedSchema and OpenInstrumentedForSharedSchemaDB to create/register the dedicated schema handle.
pkg/metrics/db.go	Adds RegisterSharedSchemaDBWithOrg for explicit metrics registration of the shared-schema role.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9dea745bf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

🧹 Nitpick comments (1)

pkg/mysqlutil/pool_test.go (1)

104-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New test assertions use t.Fatalf instead of t.Errorf. Both files' newly-added assertion checks (not setup/precondition checks) stop the test on first failure instead of reporting and continuing, per the repo's stated t.Fatal/t.Errorf split for test files.

• pkg/mysqlutil/pool_test.go#L104-L140: convert the pure-assertion t.Fatalf calls in the extended TestDefaultPoolLimits (lines 109, 112, 115) and the new TestSharedSchemaPoolEnvOverridesLimits (lines 123, 128) and TestDefaultPoolLifetimeSharedSchemaIsShortLived (lines 136, 139) to t.Errorf.
• pkg/tenant/schema/shared_test.go#L238-L243: convert the two post-EnsureSharedSchema assertion checks in TestEnsureSharedSchemaReportsSchemaRoleMetrics (lines 239 and 242) from t.Fatalf to t.Errorf.

As per coding guidelines, "Use t.Fatal or t.Fatalf for setup failures and t.Errorf for assertion failures."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/mysqlutil/pool_test.go` around lines 104 - 140, Use t.Errorf instead of
t.Fatalf for pure assertion checks in TestDefaultPoolLimits,
TestSharedSchemaPoolEnvOverridesLimits, and
TestDefaultPoolLifetimeSharedSchemaIsShortLived within
pkg/mysqlutil/pool_test.go (lines 104-140), while preserving fatal calls for
setup failures. In pkg/tenant/schema/shared_test.go (lines 238-243), update the
two post-EnsureSharedSchema assertions in
TestEnsureSharedSchemaReportsSchemaRoleMetrics to use t.Errorf; no other test
behavior needs changing.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/mysqlutil/pool_test.go`:
- Around line 104-140: Use t.Errorf instead of t.Fatalf for pure assertion
checks in TestDefaultPoolLimits, TestSharedSchemaPoolEnvOverridesLimits, and
TestDefaultPoolLifetimeSharedSchemaIsShortLived within
pkg/mysqlutil/pool_test.go (lines 104-140), while preserving fatal calls for
setup failures. In pkg/tenant/schema/shared_test.go (lines 238-243), update the
two post-EnsureSharedSchema assertions in
TestEnsureSharedSchemaReportsSchemaRoleMetrics to use t.Errorf; no other test
behavior needs changing.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c379123-d7c8-4fee-9874-fed3c2594768

📥 Commits

Reviewing files that changed from the base of the PR and between ff56ed9 and f9dea74.

📒 Files selected for processing (7)

• pkg/metrics/db.go
• pkg/mysqlutil/instrumented.go
• pkg/mysqlutil/pool.go
• pkg/mysqlutil/pool_test.go
• pkg/tenant/pool.go
• pkg/tenant/schema/shared.go
• pkg/tenant/schema/shared_test.go

[qiffang]

Dev-Lead review (correctness adjudication on the highest-risk axis: advisory-lock scoping across the new handle).

The isolation is sound. Verified the axis where a bug would hide — moving the schema-DDL advisory lock to a dedicated shared_schema handle without breaking cross-pod mutual exclusion:

✓ Advisory lock correctly held for the full DDL duration. withSharedDBSchemaAdvisoryLock (pool.go:225) pins a single *sql.Conn (db.Conn(ctx), 231), does GET_LOCK on that conn (246), and releases via a defer (263) so RELEASE_LOCK runs after fn(ctx) returns (277). Go defers run LIFO on function return → the lock is held for the entire DDL callback. So even though the DDL callback runs on the schemaDB pool (a different physical connection than the lock-holding conn, since MaxOpenConns=12), cross-pod serialization holds: only one pod is inside the locked callback at a time. The "lock on one conn, DDL on another pool conn" pattern is fine for mutual exclusion because the lock gates entry and is held throughout.

✓ Bounded, short-lived shared_schema pool. defaultSharedSchemaMaxOpenConns=12 / idle=4 / lifetime=3m / idle=20s — not an unbounded second pool. DDL + advisory-lock waits are moved off the long-lived shared data-plane handle (the PR's goal), and the metrics assertion confirms DDL no longer enters the shared data-plane series.

One residual (documented + pre-existing, NOT introduced here): pool.go:221-224 self-documents "the lock is session-owned; if that target connection is lost mid-DDL, TiDB releases it and a later attempt can overlap the still-finishing statement." That conn-loss-mid-DDL → lock-auto-release → overlap window is inherent to MySQL/TiDB advisory locks + connection loss; this PR moves the same pattern to a dedicated handle, it doesn't create the window. Worth a follow-up if strict overlap-freedom under connection loss is required (e.g. a fencing token or DDL idempotency guarantee), but not a blocker for this isolation fix.

Deferring the line-by-line (pool-limit env plumbing, role tagging, metrics-series assertions, test coverage completeness) to @adversary-1 @adversary-2. From the architecture/correctness axis I own: the advisory-lock scoping is correct and the isolation achieves its goal.

[qiffang]

adversary-2 review — GREEN at f9dea745. Careful review complete, no blockers.

1. Isolation completeness (sibling-entry-point) ✓ — sharedDBHandleWithSchemaLockWait (pool.go:1146) now bootstraps schema FIRST via ensureSharedDBSchemaOnce on a dedicated shared_schema handle (advisory-lock + DDL + meta update all on that handle), THEN opens the data-plane OpenInstrumentedForSharedDB handle. Verified ALL entry points: the only two callers of sharedDBHandleWithSchemaLockWait are line 1143 (wait=true) and EnsureSharedDBReady (1265, wait=false — i.e. the 'normal cache path' IS this same patched function, not an unpatched sibling); EnsureSharedSchema/ensureSharedDBSchema is only invoked from ensureSharedDBSchemaOnce on schemaDB. No path applies shared-schema DDL on the role="shared" data-plane handle.

2. Lifecycle ✓ — ensureSharedDBSchemaOnce opens schemaDB + defer Close (short-lived); dedicated small pool (maxOpen 12 / maxIdle 4 / lifetime 3m / idle 20s), env-configurable, independent of the shared data-plane pool.

3. Metric isolation (value-based assertion) ✓ — RoleSharedSchema="shared_schema" via RegisterSharedSchemaDBWithOrg. shared_test.go asserts after DDL: exec/RoleSharedSchema count > 0 AND exec/RoleShared count == 0 (data-plane series untouched). Directly proves schema DDL/lock-wait no longer inflate the role="shared" latency series that alerts read — the extension of the PR #802 metric-attribution work.

4. Multi-statement safeguard ✓ — OpenSharedSchemaDB rejects HasMultiStatements.

5. Pool tests ✓ — defaultPoolLimits(RoleSharedSchema) correct + maxOpen well under the shared budget; env override independent (setting shared=77 doesn't affect shared_schema=3); short-lived lifetime asserted.

GREEN.

[qiffang]

adversary-1 GREEN at exact head f9dea745bff554180f4cc2def58398549c6b36d4.

I reviewed the changed surface around shared schema bootstrap isolation:

• sharedDBHandleWithSchemaLockWait now runs outdated shared-schema ensure through ensureSharedDBSchemaOnce before opening the long-lived RoleShared data-plane handle. The only normal/cache and EnsureSharedDBReady entry points route through this function, so I did not find a sibling path that still runs shared DDL on the data-plane handle.
• ensureSharedDBSchemaOnce opens a dedicated schema.OpenSharedSchemaDB handle, runs the advisory-lock wait and DDL on that RoleSharedSchema pool, and defers mysqlutil.CloseInstrumented, so failed/successful schema ensure does not leave an extra registered pool behind.
• The advisory lock semantics are preserved: the lock is acquired on a pinned *sql.Conn and released after the callback returns. DDL can use other connections in the same schema pool, but cross-pod serialization still gates entry before DDL and holds through the full callback.
• RoleSharedSchema has bounded short-lived pool defaults and separate max-open/max-idle env knobs; the DDL parallelism cap is 8, so the default max-open 12 covers the lock connection plus schema workers without borrowing the 300-conn data-plane budget.
• Metrics separation is concrete: schema DDL operations are recorded as role="shared_schema", and the new regression asserts schema DDL increments RoleSharedSchema exec operations while leaving RoleShared exec operations unchanged.
• The multi-statement rejection is preserved on the new shared-schema open path.

Validation on exact head:

• git diff --check origin/main...HEAD passed.
• gofmt -l on all changed Go files produced no output.
• go test ./pkg/mysqlutil/... ./pkg/metrics/... -count=1 passed.
• go vet ./pkg/mysqlutil/... ./pkg/metrics/... ./pkg/tenant/schema/... ./pkg/tenant/... passed.
• go test -c ./pkg/tenant/schema ./pkg/tenant ./pkg/mysqlutil ./pkg/metrics passed.
• make lint passed with 0 issues.
• make build-server passed.
• Local DB-backed tenant/schema runtime tests are blocked on this machine by the known rootless Docker not found testcontainers panic; GitHub ci, drive9-server-local e2e smoke, and CodeRabbit are green at this head.

No blocking findings from my review.