Skip to content

remove schema modification in clean_ccm_op_#236

Merged
MrGuin merged 4 commits into
mainfrom
fix_update_schema
Dec 4, 2025
Merged

remove schema modification in clean_ccm_op_#236
MrGuin merged 4 commits into
mainfrom
fix_update_schema

Conversation

@MrGuin

@MrGuin MrGuin commented Nov 21, 2025

Copy link
Copy Markdown
Collaborator
  • Fix schema modification for flushdb, update ccmap schema in PostWriteAllCc when releasing the write lock;
  • Standby txns reading catalog skips read_cnt fast path.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of standby transactions during catalog access to ensure correct processing paths.
    • Removed a conditional schema-update step during catalog entry processing.
    • Ensure successful catalog reads are recorded before continuing multi-object operations.
  • Chores

    • Checkpoint log messages now include the node group identifier for clearer diagnostics.
    • Added additional debug logging during backfill processing to aid troubleshooting.

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

@coderabbitai

coderabbitai Bot commented Nov 21, 2025

Copy link
Copy Markdown

Walkthrough

Removed a dirty-schema CCM update path during KickoutCcEntryCc, added a BackFill debug log, changed checkpoint logs to include node-group IDs, introduced an early standby transaction path that uses read_catalog_op_, and set catalog_read_success_ after successful catalog reads.

Changes

Cohort / File(s) Summary
Kickout / CCM schema
tx_service/include/cc/cc_request.h
Deleted the conditional that called UpdateCcmSchema(...) when a dirty_schema was present during KickoutCcEntryCc.
Object CC diagnostics
tx_service/include/cc/object_cc_map.h
Added a debug log in BackFill when CCE payload is unknown; logs key, status (as int), and commit_ts.
Checkpoint logging
tx_service/src/checkpointer.cpp
Adjusted checkpoint log text to include node group: now logs "Begin checkpoint of node group #Y with timestamp: X".
Standby transaction catalog flow
tx_service/src/tx_execution.cpp
For standby Tx (via IsStandbyTx(TxTerm())), ObjectCommandOp and MultiObjectCommandOp now mark the command not running, initialize and push read_catalog_op_ to read the Catalog entry, process it immediately, and return early (bypassing normal ReadCatalog path).
Catalog-read success flagging
tx_service/src/tx_operation.cpp
Set catalog_read_success_ = true in ObjectCommandOp::Forward and MultiObjectCommandOp::Forward after successful catalog reads.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Cmd as Object/MultiObjectCommandOp
participant RCO as read_catalog_op_
participant Cat as Catalog table
participant Main as Normal Flow
Note over Cmd,RCO: New standby transaction path
Cmd->>RCO: reset/init & push read_catalog_op_ (mark Cmd not running)
RCO->>Cat: Read Catalog entry (sync)
Cat-->>RCO: Catalog entry result
RCO-->>Cmd: Callback with catalog result (sets catalog_read_success_)
Cmd->>Main: Resume processing (continuing operation)
Note over Cmd,Main: Non-standby still follows existing ReadCatalog flow

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25–30 minutes

  • Review standby early-exit control flow in tx_execution.cpp for state consistency (running flags, read_catalog_op_ init) across success/failure.
  • Confirm removal in cc_request.h doesn't break callers expecting dirty-schema CCM updates.
  • Verify catalog_read_success_ is used correctly across both standby and normal paths.

Possibly related PRs

Suggested reviewers

  • thweetkomputer
  • lzxddz
  • liunyl

Poem

🐇 I nibble logs and hop through code,

Standby reads now take their gentle road.
Dirty schemas hush and step away,
Catalog flags blink to show the way.
Checkpoints call the node-group day.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'remove schema modification in clean_ccm_op_' does not accurately reflect the main changes described in the PR objectives and summaries. Revise the title to reflect both key changes: removing schema modification in clean_ccm_op and adding standby transaction catalog reading optimization, or focus on the primary objective if only one is paramount.
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.
Description check ❓ Inconclusive The PR description is vague and lacks detail about what specific issues are being fixed and why these changes are necessary. Expand the description to include: the specific problem being solved, why the schema modification removal is needed, and clarify the standby transaction catalog optimization rationale.
✨ 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 fix_update_schema

📜 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 9a0600f and 280f5c7.

📒 Files selected for processing (6)
  • tx_service/include/cc/catalog_cc_map.h (0 hunks)
  • tx_service/include/cc/cc_request.h (0 hunks)
  • tx_service/include/cc/object_cc_map.h (1 hunks)
  • tx_service/src/checkpointer.cpp (1 hunks)
  • tx_service/src/tx_execution.cpp (2 hunks)
  • tx_service/src/tx_operation.cpp (2 hunks)
💤 Files with no reviewable changes (2)
  • tx_service/include/cc/cc_request.h
  • tx_service/include/cc/catalog_cc_map.h
🚧 Files skipped from review as they are similar to previous changes (4)
  • tx_service/src/tx_operation.cpp
  • tx_service/src/tx_execution.cpp
  • tx_service/src/checkpointer.cpp
  • tx_service/include/cc/object_cc_map.h

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

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

@MrGuin MrGuin force-pushed the fix_update_schema branch from 9a0600f to 280f5c7 Compare December 4, 2025 06:06
@MrGuin MrGuin merged commit 8ae2546 into main Dec 4, 2025
4 checks passed
@MrGuin MrGuin deleted the fix_update_schema branch December 4, 2025 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants