initial commit #1
Conversation
|
Warning Rate limit exceeded@Agent-Hellboy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a new PostgreSQL extension pg_retry (C implementation, control and SQL install script), PGXS Makefile and regression tests, multi-version CI workflow, documentation and debug guides, a revised license, and .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PostgreSQL
participant pg_retry
participant SPI
User->>PostgreSQL: SELECT retry.retry(sql, max_tries, ...)
PostgreSQL->>pg_retry: pg_retry_retry()
Note over pg_retry: Validate inputs (sql non-null, single statement,\nmax_tries ≥1, delays valid, parse retry SQLSTATEs)
rect rgb(230,240,255)
Note over pg_retry: Retry loop (attempt 1..max_tries)
pg_retry->>SPI: SPI_connect()
pg_retry->>pg_retry: BeginInternalSubTransaction()
pg_retry->>SPI: SPI_execute(sql)
end
alt Success
SPI-->>pg_retry: rows processed
pg_retry->>pg_retry: ReleaseCurrentSubTransaction()
pg_retry->>PostgreSQL: Return row count
PostgreSQL-->>User: Result
else Retryable error (SQLSTATE matches)
SPI-->>pg_retry: Error
pg_retry->>pg_retry: RollbackAndReleaseCurrentSubTransaction()
rect rgb(255,250,230)
Note over pg_retry: Compute delay = base*(2^(attempt-1)) ±20% jitter, clamp to [1ms, max_delay]
end
pg_retry->>PostgreSQL: Log warning & sleep(delay)
pg_retry->>pg_retry: next attempt
else Non-retryable error or exhausted
SPI-->>pg_retry: Error
pg_retry->>pg_retry: RollbackAndReleaseCurrentSubTransaction()
pg_retry->>SPI: SPI_finish()
pg_retry->>PostgreSQL: Rethrow error
PostgreSQL-->>User: Error
end
SPI->>SPI: SPI_finish()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.gitignore (1)
25-30: Optional: Improve section organization.The
results/entry is test output rather than a shared object. Consider moving it to a dedicated test artifacts section at the end of the file for semantic clarity, though the current placement is functionally correct.# Shared objects (inc. Windows DLLs) *.dll *.so *.so.* *.dylib -results/ # ExecutablesThen add a new section at the end:
# Kernel Module Compile Results *.mod* *.cmd .tmp_versions/ modules.order Module.symvers Mkfile.old dkms.conf # debug information files *.dwo + +# Test artifacts +results/README.md (2)
7-7: Add language specifier to code block.The code block would benefit from a language specifier for better syntax highlighting.
Apply this diff:
-``` +```text pg_retry--- `28-40`: **Add language specifier to bash code block.** The bash code block should have a language specifier for syntax highlighting. Apply this diff: ```diff -```bash +```bash # Clone or download the extensionNote: The code block at line 28 already has the
bashidentifier, which is correct. However, ensure consistency throughout the document.sql/pg_retry--1.0.sql (1)
21-23: Consider security implications of PUBLIC grants.The function is granted to PUBLIC, allowing any user to call
retry.retry(). This is generally acceptable because:
- The function validates and restricts input SQL
- Execution occurs within the caller's permissions
- No privilege escalation is possible
However, for security-conscious deployments, consider documenting that administrators can revoke PUBLIC access and grant selectively:
REVOKE EXECUTE ON FUNCTION retry.retry(TEXT, INT, INT, INT, TEXT[]) FROM PUBLIC; GRANT EXECUTE ON FUNCTION retry.retry(TEXT, INT, INT, INT, TEXT[]) TO specific_role;debug.md (1)
1-55: Excellent comprehensive debug guide with platform-specific instructions.The compilation and installation sections are detailed and practical. However, the guide is heavily macOS/Homebrew-focused. Consider adding a note at the beginning about platform-specific paths and including Linux examples for broader applicability.
Consider adding a platform note at the beginning:
# pg_retry Extension - Debug and Testing Guide > **Note**: This guide includes platform-specific paths for macOS with Homebrew. > For Linux systems, replace paths like `/opt/homebrew/Cellar/postgresql@18/` > with your system's PostgreSQL installation directory (commonly `/usr/lib/postgresql/18/` > or `/usr/pgsql-18/`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/c-cpp.yml(2 hunks).gitignore(1 hunks)LICENSE(1 hunks)Makefile(1 hunks)README.md(1 hunks)debug.md(1 hunks)pg_retry.c(1 hunks)pg_retry.control(1 hunks)sql/pg_retry--1.0.sql(1 hunks)sql/pg_retry.sql(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Clang (14.0.6)
pg_retry.c
[error] 1-1: 'postgres.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
debug.md
[style] ~317-~317: Consider an alternative adverb to strengthen your wording.
Context: ...actions isolate retries - [ ] Memory is properly cleaned up
(PROPERLY_THOROUGHLY)
🪛 markdownlint-cli2 (0.18.1)
README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
.gitignore (1)
30-30: Appropriate addition for PostgreSQL extension test artifacts.The
results/pattern correctly ignores regression test output generated by PGXS-based tests, which aligns well with the new PostgreSQL extension.pg_retry.control (1)
1-5: LGTM! Extension control file is well-structured.The control file follows PostgreSQL extension conventions with appropriate metadata. The
relocatable = trueflag is correct for this type of extension.LICENSE (1)
1-24: LGTM! License follows PostgreSQL conventions.The license appropriately combines a permissive license with reference to PostgreSQL's own license terms, which is standard practice for PostgreSQL extensions.
README.md (1)
1-224: Excellent comprehensive documentation.The README provides thorough coverage of installation, usage, configuration, safety considerations, and limitations. The examples are clear and practical.
sql/pg_retry.sql (1)
1-44: Good test coverage for validation and basic functionality.The test suite effectively covers:
- Basic CRUD operations
- Input validation (parameter bounds)
- SQL safety checks (multi-statement, transaction control)
- Custom parameter usage
However, the tests don't validate actual retry behavior on transient errors. Consider adding tests that simulate serialization failures or deadlocks to verify retry logic.
Would you like me to suggest additional test cases for retry behavior with transient errors?
.github/workflows/c-cpp.yml (3)
20-29: Verify PostgreSQL service accessibility.The PostgreSQL service is configured with hostname
postgres, and the workflow steps usePGHOST=postgres. In GitHub Actions, services are accessible vialocalhoston mapped ports, not by service name when running directly on the runner (not in a container).Verify that the PostgreSQL service is accessible with this configuration by checking if the workflow runs successfully. If connection issues occur, you may need to either:
- Run steps in a container to use service name resolution, OR
- Use
localhostwith port mappingCheck the CI run results to confirm connectivity.
43-48: Good build configuration with proper PostgreSQL version isolation.The build step correctly sets PATH and PG_CONFIG for the specific PostgreSQL version in the matrix, ensuring clean builds for each version.
69-77: Test artifact upload is well-configured.The workflow appropriately uploads test results on failure with version-specific naming, which will help debugging issues across different PostgreSQL versions.
Makefile (1)
1-14: LGTM! Standard PGXS-based Makefile is correctly configured.The Makefile follows PostgreSQL extension conventions:
- Proper variable definitions for extension components
- Regression test configuration with
--load-extensionoption- PG_CONFIG override support for CI flexibility
- PGXS inclusion provides standard targets (all, clean, install, installcheck)
Note: The static analysis warnings about missing phony targets are false positives - PGXS provides these targets automatically.
sql/pg_retry--1.0.sql (2)
7-19: Function definition is correct and well-documented.The function is properly configured:
VOLATILE: Appropriate for functions that modify database statePARALLEL RESTRICTED: Correct for functions using SPI and subtransactions- Default parameters are sensible and well-documented
- C function binding is standard
25-28: Good documentation with inline comments.The COMMENT statements provide clear, concise descriptions that will appear in PostgreSQL's system catalogs and be visible to users querying the extension.
debug.md (3)
97-109: Excellent concurrent testing example.The concurrent workload test provides a practical way to simulate and verify retry behavior on serialization conflicts. This is valuable for manual validation of the core retry functionality.
169-243: Comprehensive troubleshooting section with actionable solutions.The troubleshooting guide covers common issues systematically with clear error messages, root causes, and solutions. This will significantly help developers debugging installation and runtime issues.
303-317: Comprehensive testing checklist for release validation.The checklist covers all critical aspects of the extension including correctness, safety, performance, and memory management. This provides a good framework for verifying the extension before release.
Note: The LanguageTool suggestion about using a stronger adverb than "properly" is a minor style point that can be safely ignored.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/c-cpp.yml (3)
3-4: Clarify workflow description to match scope.Line 3 states the workflow "Tests" the extension, but the workflow also builds and installs it. The description should more accurately reflect the full scope.
-# Tests the pg_retry PostgreSQL extension across multiple PostgreSQL versions -# Builds the extension, installs it, and runs regression tests +# Builds, installs, and tests the pg_retry PostgreSQL extension across multiple PostgreSQL versions
34-41: Consider simplifying PostgreSQL dev headers installation.The current approach adds an external PostgreSQL APT repository for all versions, including those (e.g., PostgreSQL 14) that may be available in the standard Ubuntu repos. While this ensures newer versions are available, it increases the CI runtime and introduces an external supply chain dependency for all test runs.
Consider querying whether the PostgreSQL version is available in the default repos before adding the external repository, or document the rationale for always using the official PostgreSQL repository.
Alternatively, if the external repository is intentional (to ensure consistency across versions), consider caching the APT state to reduce setup time across workflow runs:
+ - name: Cache APT packages + uses: awalsh128/cache-apt-pkgs-action@latest + with: + packages: postgresql-server-dev-${{ matrix.postgresql }} build-essential + version: 1.0 + - name: Install PostgreSQL development headers run: | # Add PostgreSQL APT repository for newer versions
47-52: Add build artifact verification step.The build step runs
make cleanandmakebut does not verify that the extension binary or expected build artifacts were produced. Adding a verification step would catch silent build failures wheremakeexits with code 0 but fails to generate required files.- name: Build extension run: | export PATH="/usr/lib/postgresql/${{ matrix.postgresql }}/bin:$PATH" export PG_CONFIG="/usr/lib/postgresql/${{ matrix.postgresql }}/bin/pg_config" make clean make + + - name: Verify build artifacts + run: | + # Verify that the extension .so file was created + [ -f pg_retry.so ] || (echo "Build artifact pg_retry.so not found" && exit 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/c-cpp.yml(2 hunks)
🔇 Additional comments (2)
.github/workflows/c-cpp.yml (2)
63-72: Approve regression test configuration.The environment variables (PGHOST, PGUSER, PGPASSWORD, PGDATABASE) are correctly set to match the PostgreSQL service configuration, and
make installcheckis the standard approach for PostgreSQL extension regression testing.
73-81: Approve artifact upload strategy.Using per-version artifact naming with
test-results-pg${{ matrix.postgresql }}and conditional upload on failure (if: failure()) is appropriate. This preserves test output for debugging while avoiding unnecessary artifact storage when tests pass.
- Fix .gitignore to allow expected test output files - Add expected/pg_retry.out to repository - Fix critical SQLSTATE handling bug using unpack_sql_state() - Add FlushErrorState() for proper error cleanup - Update documentation for fixes
- Move variable declarations before executable statements - Fixes 'ISO C90 forbids mixed declarations and code' error - Required for strict CI compilation
- Use C11 standard to allow mixed declarations and code - Cleaner code without C90 declaration ordering constraints - Maintains compatibility with modern compilers
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
debug.md (1)
12-21: Consider making the path configuration more portable.The hardcoded PostgreSQL 18 path
/opt/homebrew/Cellar/postgresql@18/18.0_1/binis version-specific and will break for users on different PostgreSQL versions or installation methods. Consider documenting a more generic approach usingpg_config --bindirto discover the path dynamically.Example for the guide:
# Find PostgreSQL bin directory dynamically export PATH="$(pg_config --bindir):$PATH"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
expected/pg_retry.outis excluded by!**/*.out
📒 Files selected for processing (6)
.github/workflows/c-cpp.yml(1 hunks).gitignore(1 hunks)Makefile(1 hunks)debug.md(1 hunks)pg_retry.c(1 hunks)sql/pg_retry.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sql/pg_retry.sql
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Clang (14.0.6)
pg_retry.c
[error] 1-1: 'postgres.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: build
pg_retry.c
[error] 194-194: gcc: error: variable ‘last_error’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]. Warnings treated as errors. Compilation failed: exit code 2.
🪛 LanguageTool
debug.md
[style] ~329-~329: Consider an alternative adverb to strengthen your wording.
Context: ...actions isolate retries - [ ] Memory is properly cleaned up
(PROPERLY_THOROUGHLY)
🪛 markdownlint-cli2 (0.18.1)
debug.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
285-285: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
.gitignore (2)
30-30: LGTM!The
results/directory is correctly ignored to prevent committing PostgreSQL regression test output artifacts.
35-35: LGTM!The negation pattern correctly preserves expected test outputs in version control, which are needed for regression test validation.
debug.md (2)
77-158: LGTM!The testing examples are comprehensive and cover key scenarios: basic functionality, parameter validation, SQL safety checks, concurrent workloads, and GUC configuration. This will be helpful for users testing the extension.
224-228: LGTM!The documentation accurately describes the SQLSTATE handling fix using
unpack_sql_state()andFlushErrorState(), which was already addressed in the code implementation..github/workflows/c-cpp.yml (2)
32-34: LGTM!The strict compiler flags with version-specific conditionals are well-configured. The
-std=c11standard and strict warning flags (-Werror) will catch potential issues early in CI.
38-40: LGTM!The test execution with automatic diff printing on failure is well-configured. This will help diagnose test failures in CI.
Makefile (1)
1-19: LGTM!The PGXS-based Makefile is correctly configured for building and testing the PostgreSQL extension. The use of
?=forPG_CFLAGSallows CI to override with stricter flags, andREGRESS_OPTS = --load-extension=pg_retryensures proper test setup.pg_retry.c (7)
1-44: LGTM!The includes, GUC variable declarations, and function prototypes are well-organized. The default retryable SQLSTATEs (serialization_failure, deadlock_detected, lock_not_available, query_canceled) are appropriate choices for retry scenarios.
49-80: LGTM!The SQLSTATE checking logic correctly deconstructs the array and compares strings. Memory management is proper with cleanup on both early return and normal exit paths.
85-100: LGTM!The transaction control detection correctly uses AST-based parsing via
IsA(raw_stmt->stmt, TransactionStmt), which prevents false positives from keywords appearing in identifiers or string literals. This addresses the concerns from previous reviews.
105-119: LGTM!The single-statement validation correctly uses PostgreSQL's parser instead of naive semicolon counting. This properly handles semicolons in string literals, comments, and JSON values. This addresses the concerns from previous reviews.
163-176: LGTM!The exponential backoff with ±20% jitter is well-implemented. The jitter helps prevent thundering herd problems when multiple clients retry simultaneously, and the minimum 1ms delay ensures reasonable behavior.
240-322: LGTM!The retry loop implementation is well-structured:
- Correctly uses
SPI_OPT_NONATOMICfor subtransaction support- Properly calls
FlushErrorState()afterCopyErrorData()to clear the error context- Uses
unpack_sql_state()for correct SQLSTATE handling (addressing previous review concerns)- Implements exponential backoff with interrupt checking
- Properly manages subtransaction lifecycle
324-408: LGTM!The cleanup, error handling, and module initialization are well-implemented:
- Memory is properly freed with
pfree(sql)- Final error is correctly rethrown with
ReThrowError()- GUC variables are properly registered with
PGC_SUSETcontext, which is appropriate for retry configuration that affects query execution behavior- Default values are sensible and match the static variable declarations
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pg_retry.c (1)
194-194: Fix volatile keyword placement to resolve compilation error.The
volatilekeyword is incorrectly placed. Currently it marks the pointed-toErrorDataas volatile, but you need to mark the pointer variable itself as volatile to prevent it from being clobbered across thelongjmpinPG_TRY/PG_CATCH.Apply this diff:
- volatile ErrorData *last_error = NULL; + ErrorData *volatile last_error = NULL;This matches the correction suggested in the past review comments and will resolve the pipeline failure.
🧹 Nitpick comments (2)
pg_retry.c (2)
163-176: Consider seeding the random number generator.The jitter calculation uses
rand()(line 171), but there's no call tosrand()to initialize the seed. Without seeding, the jitter pattern will be identical across all backend processes, reducing its effectiveness in preventing retry storms.Consider adding
srand(time(NULL) ^ getpid())in_PG_init()to ensure each backend has a unique jitter pattern:void _PG_init(void) { /* Seed RNG for jitter calculation */ srand((unsigned int)(time(NULL) ^ getpid())); /* Register GUC variables */ DefineCustomIntVariable("pg_retry.default_max_tries", /* ... rest of init ... */
327-349: Consider freeing sql on all paths.The
sqlstring allocated on line 204 is freed on the success path (line 330) but not beforeReThrowError()on the error path (line 338). WhileReThrowError()doesn't return and PostgreSQL will clean up the transaction's memory context, explicitly freeing it before the rethrow would make the cleanup more obvious.Consider applying this diff:
else { + pfree(sql); /* Rethrow the last error */ if (last_error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)pg_retry.c(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Clang (14.0.6)
pg_retry.c
[error] 1-1: 'postgres.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: build
pg_retry.c
[error] 194-194: pg_retry.c:194:25: error: variable 'last_error' might be clobbered by 'longjmp' or 'vfork' [-Werror=clobbered]
Makefile
[error] 1-1: Command failed with exit code 2. Build step 'make' failed due to compilation error in pg_retry.c.
🔇 Additional comments (12)
Makefile (1)
1-18: LGTM! Well-structured PGXS configuration.The Makefile correctly follows PostgreSQL extension conventions. The strict compiler flags on line 14 are appropriate for catching issues early.
Note: The static analysis warnings about missing phony targets (
all,clean,test) are false positives—PGXS provides these targets automatically when you include it on line 18.pg_retry.c (11)
1-20: LGTM! Comprehensive includes.All necessary PostgreSQL headers are included, including the parser headers (lines 14-16) that enable proper AST-based SQL validation.
23-44: LGTM! Well-chosen defaults.The GUC defaults are sensible, and the default SQLSTATE list (lines 27-32) appropriately covers common transient errors: serialization failures, deadlocks, lock timeouts, and query cancellations.
49-80: LGTM! Proper array handling and memory management.The function correctly deconstructs the SQLSTATE array, performs comparisons, and frees allocated memory appropriately.
85-100: LGTM! AST-based transaction control detection.Using
IsA(raw_stmt->stmt, TransactionStmt)correctly identifies transaction control statements without false positives from string literals or identifiers.
105-119: LGTM! Parser-based statement counting.Using
pg_parse_querycorrectly handles SQL with semicolons in literals, comments, or JSON values. The parsed tree is stored for reuse in validation, which is efficient.
146-158: LGTM! Clear validation logic with good documentation.The validation function properly checks both statement count and transaction control, with clear error messages for each case.
198-220: LGTM! Comprehensive argument extraction with proper defaults.The function correctly extracts arguments, handles NULL values by using GUC defaults, and properly converts the default SQLSTATE string to an array when needed.
222-237: LGTM! Thorough input validation.All parameters are validated with clear error messages, including logical constraints like ensuring
base_delay_ms ≤ max_delay_ms.
240-244: LGTM! Correct SPI setup for subtransactions.Using
SPI_connect_ext(SPI_OPT_NONATOMIC)is essential for the retry logic to work within subtransactions.
246-322: LGTM! Well-structured retry loop with proper error handling.The retry loop correctly:
- Uses subtransactions to isolate each attempt (lines 252, 269, 278)
- Calls
FlushErrorState()afterCopyErrorData()(line 275) to clear error context- Uses
unpack_sql_state()(line 283) for proper SQLSTATE string conversion- Implements exponential backoff with jitter (line 312)
- Checks for interrupts (line 314) to allow query cancellation
Based on past review comments, the major issues (substring-based detection, SQLSTATE conversion, missing FlushErrorState) have been properly addressed.
355-408: LGTM! Proper GUC registration.All GUC variables are correctly registered with appropriate:
- Data types (int/string)
- Ranges (min/max values)
- Access levels (PGC_SUSET for superuser/session control)
- Default values
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
License