SPOC-437: Add spock.sub_alter_options() for bulk subscription option changes#413
Merged
mason-sharp merged 3 commits intomainfrom Apr 10, 2026
Merged
SPOC-437: Add spock.sub_alter_options() for bulk subscription option changes#413mason-sharp merged 3 commits intomainfrom
spock.sub_alter_options() for bulk subscription option changes#413mason-sharp merged 3 commits intomainfrom
Conversation
Introduce a new SQL-callable function spock.sub_alter_options() that accepts a subscription name and a JSONB object to modify multiple subscription settings in a single call. Recognised keys are "forward_origins" (array of origin names), "apply_delay" (interval string), and "skip_schema" (array of schema names). Unrecognised keys raise an error. Only the keys present in the JSONB object are modified; omitted settings retain their current values. This avoids the need for callers to issue multiple sub_alter_* calls when changing several options at once, and provides an extensible interface for future options without adding new C functions. Add TAP test (018_forward_origins.pl) that validates the forward_origins use case across a 3-node chain (n1 -> n2 -> n3): confirms that with forwarding disabled, n1 inserts do not reach n3, then enables forwarding via sub_alter_options and verifies n1 inserts now propagate through.
📝 WalkthroughWalkthroughAdds a new C-backed SQL function 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/tap/t/018_forward_origins.pl (2)
109-129: Add one negative-path call forsub_alter_options().This file only exercises the
forward_originshappy path. Sincesrc/spock_functions.cnow hand-parses JSONB and is supposed to reject unknown keys and wrong types, one failing case here would lock down the most brittle branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/018_forward_origins.pl` around lines 109 - 129, Add a negative-path test that calls spock.sub_alter_options('sub_n2_n3', <bad jsonb>) and asserts it fails (throws/error) to exercise the JSONB parsing error branches in src/spock_functions.c; specifically invoke sub_alter_options with either an unknown key (e.g. '{"unknown_key": true}') or a wrong-type value for forward_origins (e.g. '{"forward_origins": "not_an_array"}') and use the test harness to expect a failure (pass when error is raised) before the succeeding happy-path checks so the code verifies rejection of unknown keys/types.
76-78: Assert these schema-sync waits instead of unconditionally passing.Both
wait_for_count(...)calls are ignored, so this block reports success even iftest_fwdnever appears on n2/n3. Failing here would make setup regressions much easier to diagnose.One way to tighten the assertion
-wait_for_count(2, "SELECT COUNT(*) FROM pg_tables WHERE schemaname='public' AND tablename='test_fwd'", '1', 30); -wait_for_count(3, "SELECT COUNT(*) FROM pg_tables WHERE schemaname='public' AND tablename='test_fwd'", '1', 30); -pass('Test table present on all nodes'); +ok(wait_for_count(2, "SELECT COUNT(*) FROM pg_tables WHERE schemaname='public' AND tablename='test_fwd'", '1', 30), + 'test_fwd is present on n2'); +ok(wait_for_count(3, "SELECT COUNT(*) FROM pg_tables WHERE schemaname='public' AND tablename='test_fwd'", '1', 30), + 'test_fwd is present on n3');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/018_forward_origins.pl` around lines 76 - 78, The two wait_for_count(...) calls are currently ignored and the test unconditionally calls pass('Test table present on all nodes'); instead, check the return value of the wait_for_count calls and only pass if both succeed: call wait_for_count(...) for node2 and node3, capture/verify their boolean/true results (e.g., via ok(...) or die/unless) and then call pass(...) or ok(...) once both checks succeed; update the block around wait_for_count and remove the unconditional pass so the test fails if either wait_for_count for 'test_fwd' on node2/node3 does not return success.
🤖 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_functions.c`:
- Around line 962-1030: The loop should treat an empty JSON options object as a
no-op by avoiding an unnecessary call to alter_subscription(sub); add a boolean
flag (e.g. bool any_option_seen = false) before the JsonbIteratorNext loop, set
any_option_seen = true inside each recognized-key branch (when handling
"forward_origins", "skip_schema", or "apply_delay"), and after the loop call
alter_subscription(sub) only if any_option_seen is true; reference symbols:
JsonbIteratorNext loop, keys "forward_origins"/"skip_schema"/"apply_delay",
sub->forward_origins, sub->skip_schema, sub->apply_delay, and
alter_subscription().
---
Nitpick comments:
In `@tests/tap/t/018_forward_origins.pl`:
- Around line 109-129: Add a negative-path test that calls
spock.sub_alter_options('sub_n2_n3', <bad jsonb>) and asserts it fails
(throws/error) to exercise the JSONB parsing error branches in
src/spock_functions.c; specifically invoke sub_alter_options with either an
unknown key (e.g. '{"unknown_key": true}') or a wrong-type value for
forward_origins (e.g. '{"forward_origins": "not_an_array"}') and use the test
harness to expect a failure (pass when error is raised) before the succeeding
happy-path checks so the code verifies rejection of unknown keys/types.
- Around line 76-78: The two wait_for_count(...) calls are currently ignored and
the test unconditionally calls pass('Test table present on all nodes'); instead,
check the return value of the wait_for_count calls and only pass if both
succeed: call wait_for_count(...) for node2 and node3, capture/verify their
boolean/true results (e.g., via ok(...) or die/unless) and then call pass(...)
or ok(...) once both checks succeed; update the block around wait_for_count and
remove the unconditional pass so the test fails if either wait_for_count for
'test_fwd' on node2/node3 does not return success.
🪄 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: aa264c13-0628-4b0c-bc9c-baa844f6761b
📒 Files selected for processing (5)
sql/spock--5.0.6--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock_functions.ctests/tap/scheduletests/tap/t/018_forward_origins.pl
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/spock_functions.c (1)
936-939: Update restart guidance in the function comment.The current text suggests manual disable/enable, but this function already triggers restart flow via
alter_subscription(sub)when options change. The comment should reflect automatic restart semantics to avoid confusion.Proposed comment fix
- * Note: options that affect the replication stream (forward_origins, - * apply_delay) take effect only after the apply worker reconnects to the - * publisher. Call sub_disable() followed by sub_enable() to force an - * immediate restart. + * Note: options that affect the replication stream (forward_origins, + * apply_delay) take effect on the next apply-worker reconnect + * (triggered automatically after alter_subscription() at commit).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_functions.c` around lines 936 - 939, Update the function comment to remove the suggestion to manually call sub_disable()/sub_enable() for options that affect the replication stream (forward_origins, apply_delay) and instead state that changing those options triggers the restart flow automatically via alter_subscription(sub); explicitly mention that alter_subscription(sub) will cause the apply worker to reconnect to the publisher so an immediate restart is performed without manual disable/enable. Reference the existing symbols alter_subscription(sub) and sub_disable()/sub_enable() so readers know the code path that performs the restart.tests/regress/sql/alter_options.sql (1)
23-27: Avoid tight-spin polling inwait_replicating().This loop continuously polls without yielding, which can unnecessarily burn CPU during regress runs. Add a short sleep inside the loop.
Proposed test helper improvement
CREATE OR REPLACE FUNCTION wait_replicating(sub_name name) RETURNS void AS $$ BEGIN WHILE EXISTS ( SELECT 1 FROM spock.sub_show_status() WHERE subscription_name = sub_name AND status != 'replicating' - ) LOOP END LOOP; + ) LOOP + PERFORM pg_sleep(0.05); + END LOOP; END; $$ LANGUAGE plpgsql;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regress/sql/alter_options.sql` around lines 23 - 27, The tight-spin loop in wait_replicating() that repeatedly queries spock.sub_show_status() (checking subscription_name = sub_name and status != 'replicating') needs a short sleep to yield CPU; modify the WHILE ... LOOP body to call a small sleep (e.g. pg_sleep or equivalent) each iteration before END LOOP so the loop pauses briefly (e.g. 0.1s) while polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/spock_functions.c`:
- Around line 936-939: Update the function comment to remove the suggestion to
manually call sub_disable()/sub_enable() for options that affect the replication
stream (forward_origins, apply_delay) and instead state that changing those
options triggers the restart flow automatically via alter_subscription(sub);
explicitly mention that alter_subscription(sub) will cause the apply worker to
reconnect to the publisher so an immediate restart is performed without manual
disable/enable. Reference the existing symbols alter_subscription(sub) and
sub_disable()/sub_enable() so readers know the code path that performs the
restart.
In `@tests/regress/sql/alter_options.sql`:
- Around line 23-27: The tight-spin loop in wait_replicating() that repeatedly
queries spock.sub_show_status() (checking subscription_name = sub_name and
status != 'replicating') needs a short sleep to yield CPU; modify the WHILE ...
LOOP body to call a small sleep (e.g. pg_sleep or equivalent) each iteration
before END LOOP so the loop pauses briefly (e.g. 0.1s) while polling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e54d715a-b94a-4252-9dd4-3f7d216a5829
📒 Files selected for processing (3)
Makefilesrc/spock_functions.ctests/regress/sql/alter_options.sql
…ression tests
Harden the sub_alter_options() implementation in three ways:
* No-op optimisation: track a `changed` flag through the JSONB parsing
loop. alter_subscription() — which sends SIGTERM to the apply worker
at commit — is only called when at least one option was actually
modified. An empty object '{}' now returns false without disturbing
the apply worker.
* Return value: the function now returns false when nothing changed and
true when at least one subscription field was updated, giving callers
a reliable signal about whether a worker restart is coming.
* Input validation: forward_origins elements are checked against the
only supported sentinel value "all". Any other string raises
INVALID_PARAMETER_VALUE with a hint, replacing the previous TODO
comment.
Add tests/regress/sql/alter_options.sql covering: baseline state
snapshot, no-op '{}', each option individually (forward_origins,
apply_delay, skip_schema), a multi-option call, baseline restore, and
all expected error paths (non-object JSON, unknown key, wrong value
types, invalid interval, non-existent subscription, invalid
forward_origins value).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/spock_functions.c (1)
936-939: Restart guidance in the function comment is stale.The note currently tells users to call disable/enable for immediate restart; this function already routes through
alter_subscription, so the comment should reflect the current behavior to avoid operator confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_functions.c` around lines 936 - 939, Update the stale function comment to reflect current behavior: replace the instruction to call sub_disable() followed by sub_enable() with guidance that changes now route through alter_subscription and will take effect when the apply worker reconnects via that call; reference the functions/commands alter_subscription, sub_disable, and sub_enable in the comment so readers understand that alter_subscription is the preferred/automatic path and sub_disable/sub_enable are no longer required for immediate restart.
🤖 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_functions.c`:
- Around line 1010-1015: The code is setting the flag changed = true whenever a
key is present rather than when its value actually changes; update the logic
around handling "forward_origins" and "skip_schema" (and the other similar
blocks that set changed after assigning result) to first compare the new value
(result) with the current value (e.g., sub->forward_origins or sub->skip_schema)
and only assign and set changed = true if they differ; ensure you perform an
equality check appropriate for the type (string pointer/content or boolean)
before mutating the field so no-op updates do not flip changed and trigger
restarts.
---
Nitpick comments:
In `@src/spock_functions.c`:
- Around line 936-939: Update the stale function comment to reflect current
behavior: replace the instruction to call sub_disable() followed by sub_enable()
with guidance that changes now route through alter_subscription and will take
effect when the apply worker reconnects via that call; reference the
functions/commands alter_subscription, sub_disable, and sub_enable in the
comment so readers understand that alter_subscription is the preferred/automatic
path and sub_disable/sub_enable are no longer required for immediate restart.
🪄 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: 9abe069e-8217-46bf-87da-de5d3a6ec84b
⛔ Files ignored due to path filters (1)
tests/regress/expected/alter_options.outis excluded by!**/*.out
📒 Files selected for processing (3)
Makefilesrc/spock_functions.ctests/regress/sql/alter_options.sql
✅ Files skipped from review due to trivial changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/regress/sql/alter_options.sql
mason-sharp
reviewed
Apr 9, 2026
Member
mason-sharp
left a comment
mason-sharp
left a comment
There was a problem hiding this comment.
Looking good to me, I just added one comment.
Other than that, can you update docs to add the new function?
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/spock_functions.c (1)
1009-1013:⚠️ Potential issue | 🟠 Major
changedstill flips on key presence, causing avoidable apply-worker restarts.At Line [1013] and Line [1031],
changedis set even when the incoming value equals current state, so Line [1044] can still triggeralter_subscription(sub)for effective no-ops.Proposed idempotent-change guard
- if (strcmp(key, "forward_origins") == 0) - sub->forward_origins = result; - else - sub->skip_schema = result; - changed = true; + if (strcmp(key, "forward_origins") == 0) + { + if (!spock_strlist_equal(sub->forward_origins, result)) + { + sub->forward_origins = result; + changed = true; + } + } + else + { + if (!spock_strlist_equal(sub->skip_schema, result)) + { + sub->skip_schema = result; + changed = true; + } + } @@ - sub->apply_delay = DatumGetIntervalP( + Interval *new_delay = DatumGetIntervalP( DirectFunctionCall3(interval_in, CStringGetDatum(delay_str), ObjectIdGetDatum(InvalidOid), Int32GetDatum(-1))); - changed = true; + if (sub->apply_delay == NULL || + !DatumGetBool(DirectFunctionCall2(interval_eq, + IntervalPGetDatum(sub->apply_delay), + IntervalPGetDatum(new_delay)))) + { + sub->apply_delay = new_delay; + changed = true; + }Also applies to: 1026-1032, 1043-1045
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_functions.c` around lines 1009 - 1013, The code unconditionally sets the flag changed after assigning sub->forward_origins or sub->skip_schema, causing alter_subscription(sub) to run even when the new value equals the existing one; update the logic in the block that handles keys "forward_origins" and "skip_schema" (and the similar blocks at the other mentioned ranges) to first compare the incoming result with the current sub field (e.g., compare strings for sub->forward_origins with strcmp or compare booleans/integers for sub->skip_schema), only assign and set changed = true when the value actually differs, and leave changed untouched for no-op updates so alter_subscription(sub) is not triggered unnecessarily.
🧹 Nitpick comments (1)
tests/regress/sql/alter_options.sql (1)
21-29: Avoid hot-spin polling inwait_replicating().Line [27] loops continuously with no backoff, which can waste CPU and make CI timing less stable. Add a small sleep in the loop body.
Suggested tweak
CREATE OR REPLACE FUNCTION wait_replicating(sub_name name) RETURNS void AS $$ BEGIN WHILE EXISTS ( SELECT 1 FROM spock.sub_show_status() WHERE subscription_name = sub_name AND status != 'replicating' - ) LOOP END LOOP; + ) LOOP + PERFORM pg_sleep(0.05); + END LOOP; END; $$ LANGUAGE plpgsql;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regress/sql/alter_options.sql` around lines 21 - 29, The wait_replicating function currently busy-loops; modify its loop body to yield with a short sleep to avoid hot-spin polling: inside the WHILE loop in function wait_replicating, call pg_sleep(...) (e.g., 0.1s) via PERFORM pg_sleep(...) so the loop checks spock.sub_show_status() less frequently and reduces CPU usage while waiting for status to become 'replicating'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/spock_functions.c`:
- Around line 1009-1013: The code unconditionally sets the flag changed after
assigning sub->forward_origins or sub->skip_schema, causing
alter_subscription(sub) to run even when the new value equals the existing one;
update the logic in the block that handles keys "forward_origins" and
"skip_schema" (and the similar blocks at the other mentioned ranges) to first
compare the incoming result with the current sub field (e.g., compare strings
for sub->forward_origins with strcmp or compare booleans/integers for
sub->skip_schema), only assign and set changed = true when the value actually
differs, and leave changed untouched for no-op updates so
alter_subscription(sub) is not triggered unnecessarily.
---
Nitpick comments:
In `@tests/regress/sql/alter_options.sql`:
- Around line 21-29: The wait_replicating function currently busy-loops; modify
its loop body to yield with a short sleep to avoid hot-spin polling: inside the
WHILE loop in function wait_replicating, call pg_sleep(...) (e.g., 0.1s) via
PERFORM pg_sleep(...) so the loop checks spock.sub_show_status() less frequently
and reduces CPU usage while waiting for status to become 'replicating'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4094ccb-337c-4104-9ddc-41848a31581c
⛔ Files ignored due to path filters (1)
tests/regress/expected/alter_options.outis excluded by!**/*.out
📒 Files selected for processing (2)
src/spock_functions.ctests/regress/sql/alter_options.sql
mason-sharp
approved these changes
Apr 10, 2026
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.
Introduce a new SQL-callable function that accepts a subscription name and a JSONB object to modify multiple subscription settings in a single call:
Recognised keys
"forward_origins"string[]["all"]to forward all origins)"apply_delay"stringinterval_in, e.g."500ms"or"1 minute""skip_schema"string[]Only keys present in the JSONB object are modified; omitted settings retain their current values. Any unrecognised key or wrong JSON type raises an error immediately.
Why JSONB rather than separate functions
The existing
sub_alter_*family requires one round-trip per option. When several settings need to change atomically (e.g.forward_origins+apply_delaytogether), callers had to issue multiple calls and the subscription catalog was updated — and the apply worker signalled — multiple times.sub_alter_optionsperforms a single catalog write and sends one SIGTERM, which the manager uses to restart the apply worker with the new settings.The JSONB interface is also forward-compatible: new options can be added without new C functions or SQL signatures.
Restart behaviour
alter_subscription()sends SIGTERM to the apply worker at transaction commit; the spock manager restarts it automatically. Options that affect the replication stream (forward_origins,apply_delay) therefore take effect on the nextSTART_REPLICATIONnegotiation without any manualsub_disable/sub_enablecycle.Files changed
src/spock_functions.c—spock_alter_subscription_optionsC implementation +PG_FUNCTION_INFO_V1declaration; added#include "utils/jsonb.h"sql/spock--6.0.0-devel.sql—CREATE FUNCTIONfor the fresh-install pathsql/spock--5.0.6--6.0.0-devel.sql—CREATE FUNCTIONfor the upgrade pathtests/tap/t/018_forward_origins.pl— TAP test: 3-node cascade (n1 → n2 → n3), confirms n1 inserts do not reach n3 while forwarding is disabled, then enables forwarding viasub_alter_optionsand verifies n1 inserts now propagate end-to-end; usesspock.sync_event/wait_for_sync_eventfor deterministic synchronisation instead of sleepstests/tap/schedule— registers the new test