feat: add spock.resolutions_retention_days GUC and cleanup_resolutions()#412
Open
feat: add spock.resolutions_retention_days GUC and cleanup_resolutions()#412
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdd retention-based automatic deletion for Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/regress/sql/resolutions_retention.sql (1)
94-106: Consider resetting the provider-side retention GUC for completeness.The cleanup section resets
spock.resolutions_retention_dayson the subscriber but not on the provider. While the provider doesn't modify this GUC in the test, explicitly resetting both nodes would ensure a clean slate for subsequent tests.Proposed addition
\c :subscriber_dsn RESET spock.resolutions_retention_days; ALTER SYSTEM SET spock.save_resolutions = off; SELECT pg_reload_conf(); + +\c :provider_dsn +RESET spock.resolutions_retention_days;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regress/sql/resolutions_retention.sql` around lines 94 - 106, Add resetting of the provider-side GUC spock.resolutions_retention_days in the cleanup block so both nodes return to defaults: after switching to the provider connection (currently via "\c :provider_dsn") ensure you RESET spock.resolutions_retention_days and, if you disable resolution saving on the provider, also reapply ALTER SYSTEM SET spock.save_resolutions = off followed by SELECT pg_reload_conf() to mirror the subscriber cleanup; update the cleanup sequence around spock.repset_remove_table / spock.replicate_ddl and the subsequent ALTER SYSTEM/pg_reload_conf calls to include these provider-side resets.src/spock_conflict.c (1)
936-1001: Well-designed transaction management for background execution.The error handling correctly:
- Saves the caller's memory context before transaction start
- Copies error data to the surviving context before abort
- Uses
AbortCurrentTransaction()which handles SPI/snapshot cleanup automatically- Downgrades to WARNING to avoid disrupting replication
One minor defensive consideration:
edata->messagecould theoretically be NULL in unusual error scenarios.Defensive NULL check
ereport(WARNING, (errcode(edata->sqlerrcode), - errmsg("%s", edata->message))); + errmsg("%s", edata->message ? edata->message : "cleanup_resolutions failed"))); FreeErrorData(edata);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_conflict.c` around lines 936 - 1001, The code in spock_cleanup_resolutions uses edata->message in the ereport(WARNING ...) call but edata->message can be NULL; update the PG_CATCH block to defensively check edata->message (the ErrorData pointer returned by CopyErrorData()) and pass a safe fallback string (e.g. "no error message" or similar) to ereport so you never call errmsg with a NULL pointer; reference the PG_CATCH block, the edata variable, CopyErrorData(), and the ereport(WARNING...) invocation when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/spock_apply.c`:
- Around line 3057-3075: The spock_cleanup_resolutions() call runs unprotected
inside the apply loop and can throw into the replication-exception path; wrap
the call in a local exception block using PG_TRY / PG_CATCH around
spock_cleanup_resolutions() (located near last_cleanup_timestamp and
GetCurrentTimestamp checks) so cleanup failures are handled locally: on error,
log the failure (with error details) and clear the error with FlushErrorState()
/ ReThrowError? (do not re-raise), then continue the main loop and still update
last_cleanup_timestamp only on success (or decide to back off by leaving it
unchanged). Ensure no transaction state is assumed (check IsTransactionState
usage remains) and avoid propagating the exception to replication exception
handling.
---
Nitpick comments:
In `@src/spock_conflict.c`:
- Around line 936-1001: The code in spock_cleanup_resolutions uses
edata->message in the ereport(WARNING ...) call but edata->message can be NULL;
update the PG_CATCH block to defensively check edata->message (the ErrorData
pointer returned by CopyErrorData()) and pass a safe fallback string (e.g. "no
error message" or similar) to ereport so you never call errmsg with a NULL
pointer; reference the PG_CATCH block, the edata variable, CopyErrorData(), and
the ereport(WARNING...) invocation when making this change.
In `@tests/regress/sql/resolutions_retention.sql`:
- Around line 94-106: Add resetting of the provider-side GUC
spock.resolutions_retention_days in the cleanup block so both nodes return to
defaults: after switching to the provider connection (currently via "\c
:provider_dsn") ensure you RESET spock.resolutions_retention_days and, if you
disable resolution saving on the provider, also reapply ALTER SYSTEM SET
spock.save_resolutions = off followed by SELECT pg_reload_conf() to mirror the
subscriber cleanup; update the cleanup sequence around spock.repset_remove_table
/ spock.replicate_ddl and the subsequent ALTER SYSTEM/pg_reload_conf calls to
include these provider-side resets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f8490f1-c398-45d5-b8f8-42ed592f41ce
⛔ Files ignored due to path filters (1)
tests/regress/expected/resolutions_retention.outis excluded by!**/*.out
📒 Files selected for processing (10)
Makefiledocs/configuring.mddocs/spock_functions/functions/spock_cleanup_resolutions.mdinclude/spock_conflict.hsql/spock--5.0.6--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_apply.csrc/spock_conflict.ctests/regress/sql/resolutions_retention.sql
rasifr
force-pushed
the
feature/SPOC-266/resolutions-table-cleanup
branch
from
e83a5ef to
8247abd
Compare
Introduces spock.resolutions_retention_days (int, default 100, min 0) to control how long rows are kept in spock.resolutions. Rows older than the configured window are deleted automatically by the apply worker at most once per day. Setting the GUC to 0 disables automatic cleanup entirely. Also adds spock.cleanup_resolutions(), a superuser-only SQL function that returns the number of rows deleted, for manual invocation. A log_time index is added to both the fresh-install and upgrade scripts to keep the daily DELETE efficient on large resolutions tables. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ns() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rasifr
force-pushed
the
feature/SPOC-266/resolutions-table-cleanup
branch
from
8247abd to
b9adb1d
Compare
mason-sharp
requested changes
Apr 9, 2026
| * own transaction and error handling. | ||
| */ | ||
| if (!IsTransactionState() && | ||
| spock_resolutions_retention_days > 0) |
Member
There was a problem hiding this comment.
The docs mention it checks save_resolutions, too.
| NULL, | ||
| &spock_resolutions_retention_days, | ||
| 100, 0, INT_MAX, | ||
| PGC_SUSET, |
Member
There was a problem hiding this comment.
Should this use PGC_SIGHUP since it is used in the apply worker?
| #define is_skipping_changes() (unlikely(!XLogRecPtrIsInvalid(skip_xact_finish_lsn))) | ||
|
|
||
| /* How often the apply worker runs spock_cleanup_resolutions() (milliseconds). */ | ||
| #define RESOLUTIONS_CLEANUP_INTERVAL_MS (86400L * 1000L) |
Member
There was a problem hiding this comment.
We could leave this, but ms resolution seems like overkill when seconds would do?
| @@ -0,0 +1,41 @@ | |||
| ## NAME | |||
|
|
|||
| spock.cleanup_resolutions() | |||
Member
There was a problem hiding this comment.
well, since you created this, what might be nice is if takes an optional argument that overrides resolutions_retention_days. For example, maybe they set retention to 0, then periodically manually run something like SELECT spock.cleanup_resolutions(60) when they think it is getting too big.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
spock.resolutions_retention_daysGUC (int, default 100, min 0) to control how long rows are kept inspock.resolutions. The apply worker deletes rows older than the configured window at most once per day. Setting to0disables automatic cleanup entirely.Also adds
spock.cleanup_resolutions(), a superuser-only SQL function that returns the number of rows deleted, for manual invocation.A
log_timeindex is added to both the fresh-install and upgrade scripts to keep the dailyDELETEefficient on large tables.