Skip to content

Implement SAVEPOINT support for nested transactions#592

Draft
Copilot wants to merge 6 commits into
masterfrom
copilot/fix-575
Draft

Implement SAVEPOINT support for nested transactions#592
Copilot wants to merge 6 commits into
masterfrom
copilot/fix-575

Conversation

Copilot AI commented Sep 11, 2025

Copy link
Copy Markdown
  • Identify missing AST imports in main Red.rakumod file
  • Fix TransactionContext Promise handling that could cause hanging
  • Add proper AST imports to test files
  • Fix test array access patterns for translate method results
  • Validate savepoint implementation for proper delegation
  • Verify tests pass without hanging
  • Address any remaining issues found during testing

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 4 commits September 11, 2025 02:14
Co-authored-by: FCO <99834+FCO@users.noreply.github.com>
Co-authored-by: FCO <99834+FCO@users.noreply.github.com>
…ests

Co-authored-by: FCO <99834+FCO@users.noreply.github.com>
Co-authored-by: FCO <99834+FCO@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement SAVEPOINT (and friends.) Implement SAVEPOINT support for nested transactions Sep 11, 2025
Copilot AI requested a review from FCO September 11, 2025 02:22
@FCO

FCO commented Sep 11, 2025

Copy link
Copy Markdown
Owner

It seems a file is missing to be added and there are tests hanging... @copilot could you take a look, please?

Co-authored-by: FCO <99834+FCO@users.noreply.github.com>

Copilot AI commented Sep 11, 2025

Copy link
Copy Markdown
Author

It seems a file is missing to be added and there are tests hanging... @copilot could you take a look, please?

I've addressed both issues:

Missing file: The savepoint AST classes needed to be imported in the main Red.rakumod file. Added imports for Red::AST::Savepoint, Red::AST::RollbackToSavepoint, and Red::AST::ReleaseSavepoint.

Hanging tests: Fixed several issues that could cause hanging:

  1. Removed unnecessary Promise handling in TransactionContext that could block
  2. Added missing AST imports to test files so they can access the classes
  3. Fixed test array access patterns - translate() returns a Pair, not an array

The changes are in commit 0136e13. The implementation should now work without hanging and with all required files properly included.

@hermes-fco

Copy link
Copy Markdown

Code Review — feat: Implement SAVEPOINT support for nested transactions

Verdict: Comment (0 critical, 2 warnings, 1 suggestion)

Adds full SAVEPOINT support for nested transactions via Red::Driver::TransactionContext (+756/-11 across 13 files). Implements SAVEPOINT/RELEASE SAVEPOINT/ROLLBACK TO SAVEPOINT SQL translation with SQLite-specific syntax handling.


✅ Looks Good

  • AST nodes — Clean, minimal Red::AST::Savepoint, Red::AST::ReleaseSavepoint, Red::AST::RollbackToSavepoint with does Red::AST. Each has a required $.name and standard interface methods.
  • Red::Driver::TransactionContext — Good nested-level design: level 1 starts a BEGIN TRANSACTION, level > 1 creates named savepoints (sp2, sp3, ...). commit/rollback correctly dispatch to savepoint release/rollback vs. full transaction commit/rollback based on level.
  • SQLite-specific savepoint syntax — Correctly overrides ROLLBACK TO SAVEPOINTROLLBACK TO and RELEASE SAVEPOINTRELEASE (SQLite omits "SAVEPOINT" keyword for release/rollback).
  • Test coverage — Two test files (t/90-savepoints.rakutest 194 lines, t/91-savepoints-basic.rakutest 69 lines) covering basic operations, nested transactions, rollback semantics, and release.
  • Circular dependency handling — Uses require in Driver.rakumod to break circular use between Red::Driver and Red::Driver::TransactionContext. Valid Raku pattern.

⚠️ Warnings

  • lib/Red/Driver/TransactionContext.rakumod:58-60 — The FALLBACK method silently delegates ALL unknown method calls to $!parent. This can mask typos and missing methods — e.g., calling $ctx.typooed-method() silently passes through without error. Consider logging a warning for unmatched methods in debug mode, or listing all explicitly delegated methods and removing FALLBACK.

    method FALLBACK($name, |c) {
        note "FALLBACK: $name delegated to parent" if $*RED-DEBUG;
        $!parent."$name"(|c)
    }
  • lib/Red/Driver/TransactionContext.rakumod:20,23,70,73,81,84 — Multiple $!parent.prepare(...).map: *.execute calls cast results to Seq but discard them. If any of these statements fail, the exception propagates but there's no explicit error handling. Ensure that prepareexecute failures in savepoint operations are properly caught and surfaced.

💡 Suggestions

  • lib/Red/Driver/TransactionContext.rakumod:30 — Savepoint names are auto-generated as sp{$level}. Consider allowing callers to pass a custom savepoint name for better debuggability in deeply nested transactions.
    method new-connection(Str :$name) {
        my $new-level = $!level + 1;
        my $savepoint-name = $name // "sp{$new-level}";
        ...
    }

📊 Stats

Metric Value
Files changed 13
Lines added +756
Lines removed −11
New tests 2 test files (t/90-savepoints, t/91-savepoints-basic)
Security scan Clean

Reviewed by Hermes Agent

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.

3 participants