refactor: Correct the behavior of manual checkpoint for AP and TP

[zhanglei1949]

Fix #651

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Refactors checkpoint execution to correct manual checkpoint behavior in both embedded (AP) and service (TP) modes, addressing #651 by centralizing checkpoint orchestration and ensuring WAL/timestamp timeline handling and plan-cache correctness.

Changes:

• Introduces CheckpointCoordinator + CheckpointCreation to own the durable checkpoint build/publish + in-place reopen protocol.
• Reworks TP checkpoint semantics: drained update guard, WAL rotation, timestamp timeline reset, and cache invalidation hooks.
• Adds/updates extensive C++ + Python regression tests and updates API/docs around checkpoint-on-close and CHECKPOINT access-mode rules.

Reviewed changes

Copilot reviewed 66 out of 66 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tools/python_bind/tests/test_tp_service.py	Adds TP checkpoint regression tests for WAL rotation, timeline reset, and cached-plan invalidation.
tools/python_bind/tests/test_persistence.py	Adds AP checkpoint regression tests for PK lookups across COPY+CHECKPOINT and access-mode / explain-profile behavior.
tools/python_bind/src/py_database.h	Makes PyDatabase destructor noexcept (out-of-line) to avoid throwing from destructors.
tools/python_bind/src/py_database.cc	Implements PyDatabase destructor with exception suppression + improves close() docstring in bindings.
tools/python_bind/neug/database.py	Updates Python API docs for Database.close() checkpoint-on-close semantics and idempotence.
tests/transaction/test_wal_replay.cc	Adds unit tests for WAL writer reopen on new timeline and new VersionManager/update-guard behaviors.
tests/transaction/test_update_transaction.cc	Removes obsolete checkpoint test that called removed storage-level checkpoint API.
tests/transaction/test_transaction_release_order.cc	Updates test mock to new IVersionManager update-release API (drain + mode).
tests/transaction/test_compact_transaction.cc	Fixes compact blocking test helper to return timestamps and pass correct release values/modes.
tests/transaction/test_acid.cc	Updates comments to reflect renamed VM state concepts (checkpoint/update semantics).
tests/storage/test_graph_snapshot_store_concurrency.cc	Adds maintenance precondition test ensuring checkpoint maintenance rejects pinned readers.
tests/storage/test_checkpoint.cc	Updates tests for new checkpoint creation API and adds manual/tp routing + failure-mode coverage.
tests/compiler/flag_test.cpp	Switches execution flag from transaction to checkpoint + adds access-mode inference test.
src/utils/exception/exception.cc	Adds ServiceUnavailableException implementation.
src/transaction/wal/local_wal_writer.cc	Adds noexcept destructor, changes open() signature to accept uri, hardens close() fd handling.
src/transaction/wal/dummy_wal_writer.cc	Updates dummy WAL writer to new open(const std::string&) signature.
src/transaction/version_manager.cc	Adds explicit reader draining, stronger begin_commit validation, and timeline reset via update-release mode.
src/transaction/update_transaction.cc	Switches to UpdateTimestampGuard, refines commit ordering, and removes storage-level checkpoint behavior.
src/transaction/update_timestamp_guard.cc	Implements new move-only RAII guard for update timestamps and commit phases.
src/transaction/insert_transaction.cc	Renames WAL writer member usage consistent with new interface.
src/transaction/compact_transaction.cc	Renames WAL writer member usage consistent with new interface.
src/storages/graph/graph_snapshot_store.cc	Adds checkpoint maintenance handle + strict precondition checks for in-place reopen.
src/storages/graph/graph_interface.cc	Removes AP storage-level CreateCheckpoint() implementation.
src/storages/graph/checkpoint_session.cc	Replaces checkpoint session with CheckpointCreation implementation.
src/storages/graph/checkpoint_manager.cc	Makes staging creation exception-safe and adds TargetPublishedPath() helper.
src/server/session_pool.cc	Adds WAL rotation + cache invalidation helper used as checkpoint install hook.
src/server/neug_db_session.cc	Moves CHECKPOINT execution out of operator pipeline into coordinator; adjusts commit ordering for safe materialization.
src/server/neug_db_service.cc	Wires coordinator into service + registers/unregisters install hook for WAL rotation/caches.
src/server/brpc_service_mgr.cc	Adds HTTP mapping for service-unavailable and routes GetSchema through sessions.
src/main/query_processor.cc	Centralizes access-mode resolution, adds CHECKPOINT special-case executor in AP mode, and adds get_schema().
src/main/neug_db.cc	Introduces coordinator ownership in DB lifecycle; refactors recovery/shutdown checkpoint paths; improves destructor safety.
src/main/connection.cc	Routes GetSchema via QueryProcessor and maps errors to typed exceptions.
src/main/checkpoint_coordinator.cc	Adds new coordinator implementing checkpoint build/publish, in-place reopen, allocator rotation, and hook invocation.
src/execution/execute/ops/admin/checkpoint.cc	Disables runtime execution of Checkpoint operator (must be executed by checkpoint executor path).
include/neug/utils/result.h	Maps ServiceUnavailableException to ERR_SERVICE_UNAVAILABLE.
include/neug/utils/exception/exception.h	Declares ServiceUnavailableException and adds throw macro.
include/neug/transaction/wal/wal_builder.h	Removes checkpoint-only WAL API/docs and updates finalize contract.
include/neug/transaction/wal/wal.h	Changes WAL writer interface to open(const std::string&).
include/neug/transaction/wal/local_wal_writer.h	Updates interface + noexcept destructor.
include/neug/transaction/wal/dummy_wal_writer.h	Updates interface to new open signature.
include/neug/transaction/version_manager.h	Adds UpdateReleaseMode + reader draining API; documents timeline reset semantics.
include/neug/transaction/update_transaction.h	Switches UpdateTransaction to RAII update guard ownership and removes CreateCheckpoint from interface.
include/neug/transaction/update_timestamp_guard.h	Declares new update timestamp RAII guard.
include/neug/transaction/insert_transaction.h	Renames WAL writer parameter/member and updates concurrency docs wording.
include/neug/transaction/compact_transaction.h	Renames WAL writer parameter/member.
include/neug/transaction/README.md	Updates transaction state-machine documentation to new naming/semantics.
include/neug/storages/graph_snapshot_store.h	Declares maintenance handle API and updates comments.
include/neug/storages/graph/graph_interface.h	Removes CreateCheckpoint() from storage update interfaces.
include/neug/storages/checkpoint_session.h	Deletes obsolete CheckpointSession API.
include/neug/storages/checkpoint_manager.h	Adds TargetPublishedPath() and clarifies “repair primitive” semantics for restore/cleanup APIs.
include/neug/storages/checkpoint_creation.h	Introduces new RAII checkpoint builder/publisher API.
include/neug/storages/checkpoint.h	Adds shared allocator_prefix() helper.
include/neug/storages/allocators.h	Adds SwapState() to support in-place allocator rotation and deletes copy operations.
include/neug/server/session_pool.h	Refactors session pool to accept snapshot store + coordinator, adds alignment constants, and opens WAL with uri.
include/neug/server/neug_db_session.h	Refactors session to use snapshot store + coordinator and adds schema + cache invalidation helpers.
include/neug/server/neug_db_service.h	Tracks checkpoint hook token for safe unregistration.
include/neug/main/query_processor.h	Adds coordinator dependency, schema API, and refactors pipeline retrieval signature.
include/neug/main/neug_db.h	Adds coordinator accessor + updates lifecycle semantics (reopen allowed; close is best-effort checkpoint).
include/neug/main/checkpoint_coordinator.h	Declares new checkpoint coordinator API and hook mechanism.
include/neug/compiler/planner/graph_planner.h	Updates comment on access-mode inference capabilities.
include/neug/compiler/gopt/g_physical_convertor.h	Maps analyzer’s checkpoint flag to protobuf execution flags.
include/neug/compiler/gopt/g_physical_analyzer.h	Renames transaction flag to checkpoint and sets it only for CHECKPOINT action.
doc/source/transaction/transaction.md	Updates user-facing docs for checkpoint exclusivity, access-mode restrictions, and failure semantics.
doc/source/reference/python_api/database.md	Updates Python close() docs including checkpoint-on-close semantics.
doc/source/reference/nodejs_api/database.md	Updates Node.js close() docs including checkpoint-on-close semantics.
doc/source/reference/cpp_api/neug_db.md	Updates C++ docs for checkpoint-generation directory layout and reopen/close semantics.

Comments suppressed due to low confidence (4)

src/main/neug_db.cc:1

• NeugDB::Open(const NeugDBConfig&) returns bool but now throws on “already open”. That’s a behavioral/API mismatch for callers that treat false as the failure channel. Consider keeping the method non-throwing (return false with a log) or changing the public API to a status/exception-based return consistently (e.g., result<void> / Status) so callers have one clear error-handling model.
  src/transaction/wal/local_wal_writer.cc:1
• The error message concatenation is missing a separator, producing messages like "Failed to close fileBad file descriptor". Consider adding ": " (and ideally the fd/path) so the message is readable and actionable, e.g. "Failed to close file: " + std::string(strerror(errno)).
  src/transaction/version_manager.cc:1
• This is an unbounded tight spin loop with no backoff/yield, which can peg a CPU core under sustained readers (or in pathological cases where a reader stalls). Consider adding a small backoff (e.g., std::this_thread::yield() / pause instruction / exponential backoff) after some iterations to reduce CPU burn while still keeping low-latency behavior.
  src/main/checkpoint_coordinator.cc:1
• executeWithNewMaintenance catches only std::exception, while ExecuteTpManual also catches ... and returns a stable “unknown preparation failure” status. For consistency and to avoid unexpected exception propagation/termination, consider adding a catch (...) branch here too (returning an appropriate generic ERR_INTERNAL_ERROR message).

---

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

[Copilot]

Pull request overview

Copilot reviewed 66 out of 66 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/transaction/version_manager.cc:1

• The drain loop is a pure busy-spin with no pause/yield, which can peg a core at 100% while waiting for readers to finish (especially under contention or if a reader is slow). Consider adding a lightweight backoff (e.g., std::this_thread::yield() or a platform pause instruction) after some iterations, or switching to a wait/notify mechanism if you already have a place to signal reader release. This keeps latency reasonable while avoiding CPU burn.
  src/storages/graph/graph_snapshot_store.cc:1
• Maintenance rejection due to active pins/readers is a normal transient condition (client can retry), but this currently throws an internal error. Since this PR introduces ServiceUnavailableException/ERR_SERVICE_UNAVAILABLE (and maps it to HTTP 503), consider throwing THROW_SERVICE_UNAVAILABLE(...) here (and for the other maintenance precondition failures) so callers get a retryable signal instead of an internal error.
  src/server/session_pool.cc:1
• IWalWriter::close() / open() can throw (e.g., I/O errors). If an exception escapes this hook, the coordinator will treat it as a destructive-phase failure and abort, but without making it obvious that the failure was during WAL rotation (and potentially after some writers have been switched and others not). Consider catching exceptions inside this function and converting them into an explicit fail-stop with a clear message, or make this hook noexcept and handle failures deterministically (e.g., log + std::abort()), since partial WAL rotation is not a recoverable runtime state.
  tests/transaction/test_wal_replay.cc:1
• The new concurrency tests rely on very small timing thresholds (e.g., 20ms) to assert that a thread is blocked. This can be flaky on slow/loaded CI machines. Prefer a deterministic synchronization point (e.g., a latch/barrier to confirm the reader thread is parked in acquire_read_timestamp()), or increase timeouts with a larger margin to reduce spurious failures.

[Copilot]

Pull request overview

Copilot reviewed 66 out of 66 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/server/neug_db_session.cc:1

• ExecutePreparedPipeline() no longer copies result_schema into the response, but the insert-mode branch also doesn’t copy it afterwards (unlike read/update/checkpoint branches). This can lead to responses (including EXPLAIN/PROFILE results) missing response->schema(). Fix by copying cache_value->result_schema into response->mutable_schema() in the insert branch as well (and, if insert-mode queries can produce rows, ensure results are materialized consistently before committing).
  src/transaction/version_manager.cc:1
• drain_readers() busy-spins without any backoff/yield. If a reader is delayed (e.g., OS scheduling, long-running scan), this can burn CPU at 100% on the draining thread/core and impact latency for unrelated work. Consider adding a yield/backoff strategy (e.g., std::this_thread::yield() in the loop, exponential backoff, or atomic::wait where available) while still keeping the “minimal latency” intent for short waits.
  src/server/neug_db_session.cc:1
• EXPLAIN CHECKPOINT currently acquires an UpdateTimestampGuard (closing the insert/update gate and potentially waiting for inserters to drain) even though the operation is non-mutating and does not run checkpoint maintenance. This can unnecessarily reduce write concurrency and increase tail latency under load. Consider moving getUpdateTimestampGuard() so it’s only acquired for PROFILE CHECKPOINT / actual checkpoint execution, and using a read-style lease/pin (or no VersionManager lease if safe) for the EXPLAIN-only path.
  src/main/checkpoint_coordinator.cc:1
• executeWithNewMaintenance() catches only std::exception. If any non-std::exception escapes from AcquireCheckpointMaintenance() or related code paths, it would propagate out of the coordinator (potentially terminating the process depending on caller). The TP path (ExecuteTpManual) already has a catch (...) that returns a structured “unknown preparation failure” status; consider adding a similar catch (...) here for consistency and safer error reporting.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (4)

src/transaction/insert_transaction.cc:1

• InsertTransaction::clear() no longer re-initializes arc_ to contain a WalHeader (the prior arc_.Resize(sizeof(WalHeader)) was removed). If Commit()/Abort() are designed to be idempotent or if the transaction object can be observed/reused after clear(), later code that assumes a header-sized buffer (e.g., writing the WAL header on commit) can dereference an empty buffer and cause undefined behavior. Restore the header resize in clear() (and ensure any other reset paths also keep arc_ in a valid “header allocated” state).
  src/transaction/version_manager.cc:1
• The new drain_readers() uses an unbounded tight spin loop. If a reader is slow (or stuck), this can pin a CPU core at 100% for the entire duration of a checkpoint/maintenance window. Consider adding a bounded backoff (e.g., std::this_thread::yield() after N iterations, or a short sleep) to reduce CPU burn while still keeping latency low for the common case.
  src/transaction/version_manager.cc:1
• reset_timeline_after_checkpoint() is noexcept and relies solely on assert(...) for correctness preconditions. In release builds those checks compile out, so an accidental/buggy call could silently reset timestamps while transactions are active, which would be a high-impact concurrency/correctness failure. Consider replacing these with non-throwing runtime enforcement (e.g., CHECK(...) / LOG(FATAL)), or returning a status/error code instead of noexcept so violations cannot proceed silently in production.
  src/main/connection.cc:1
• The new error messages concatenate without a separator (e.g., "Failed to get schema." + <error>), which can produce hard-to-read output. Consider adding a delimiter like ": " (and possibly including whether YAML conversion or YAML→JSON conversion failed) to make the error actionable.

[Copilot]

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (6)

src/transaction/version_manager.cc:1

• The drain loop is a pure busy-wait with no backoff/yield, which can peg a CPU core during long-running readers and harm overall system responsiveness. Consider adding a small backoff strategy (e.g., pause/yield after N spins, or a short sleep) so checkpoint/maintenance doesn’t create sustained CPU pressure under load.
  src/transaction/insert_transaction.cc:1
• Previously clear() re-established the WAL buffer invariant by resizing to sizeof(WalHeader). With that removed, any later code path that assumes a header-sized buffer exists (e.g., writing WalHeader into arc_.GetBuffer()) can dereference a null/empty buffer. If the transaction object is ever reused after Commit()/Abort(), this becomes a correctness issue. Consider restoring arc_.Resize(sizeof(WalHeader)); in clear() (or otherwise ensuring the buffer is always header-sized whenever the transaction is active).
  src/transaction/wal/local_wal_writer.cc:1
• The thrown message concatenates without a separator ("Failed to close file" + strerror), producing hard-to-read errors (e.g., ...fileBad file descriptor). Add punctuation/whitespace such as \": \" before the strerror text to improve debuggability.
  src/main/checkpoint_coordinator.cc:1
• Preparation failures are currently collapsed into ERR_INTERNAL_ERROR even when the underlying exception is an I/O failure (e.g., staging checkpoint creation). This makes it harder for callers to distinguish transient filesystem issues from logic errors. Consider catching exception::IOException (and any checkpoint-specific exceptions you have) in the preparation path and returning ERR_IO_ERROR (or a more specific code) consistently with the destructive-phase error mapping.
  src/main/checkpoint_coordinator.cc:1
• Preparation failures are currently collapsed into ERR_INTERNAL_ERROR even when the underlying exception is an I/O failure (e.g., staging checkpoint creation). This makes it harder for callers to distinguish transient filesystem issues from logic errors. Consider catching exception::IOException (and any checkpoint-specific exceptions you have) in the preparation path and returning ERR_IO_ERROR (or a more specific code) consistently with the destructive-phase error mapping.
  src/main/connection.cc:1
• The error strings are missing a separator before the embedded status text ("schema." + status), which reduces readability. Consider changing to something like \"Failed to get schema: \" + ... so logs and exception messages are clearer.

[liulx20]

如果仅是Explain，可以考虑不做这个getUpdateTimestamp吗

[zhanglei1949]

修改为先获取read txn来获取 plan，如果为explain，则直接执行explain，不获取update timestamp；如果不是explain，则commit read txn + 获取update timestamp。

[Copilot]

Pull request overview

Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 65 out of 65 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/transaction/wal/local_wal_writer.cc:86

• Error message is missing a separator, producing messages like "Failed to close fileBad file descriptor". Add ": " to make the exception readable.

[Copilot]

Pull request overview

Copilot reviewed 76 out of 76 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/main/query_processor.cc:107

• Same as the update-lock branch above: unexpected parameter keys are silently ignored here. For consistency and better client feedback, validate parameter keys against cache_value->params_type and return ERR_INVALID_ARGUMENT on unknown keys before calling build_params_map.
  src/transaction/wal/local_wal_writer.cc:87
• The thrown close() error message concatenates "file" and strerror() without a separator, producing messages like "Failed to close fileNo such file or directory". Adding a ": " (or at least a space) makes the error actionable and easier to read.
  src/main/connection.cc:34
• This error message concatenates a sentence and the underlying error without whitespace ("schema." + error). Add a separator so the resulting exception string is readable.
  src/main/connection.cc:38
• Same formatting issue as above: the error string is built without a separator ("schema." + error). Add ": " to improve readability and log parsing.

[Copilot]

Pull request overview

Copilot reviewed 67 out of 67 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/main/connection.cc:34

• The thrown error message concatenates the prefix and the underlying error without a separator, producing messages like "Failed to get schema.". Adding a separator (e.g., ": ") makes the message clearer.
  src/main/connection.cc:38
• Same message formatting issue here: the error is concatenated without a separator, which makes the exception harder to read and parse.

[luoxiaojian]

实现放到.cc里