Group slots: group-safe WAL retention for Spock multi-master clusters.#546
Group slots: group-safe WAL retention for Spock multi-master clusters.#546ibrarahmad wants to merge 5 commits into
Conversation
Introduce the group-slot mechanism (spock_group_slot.c) wired through the manager and worker, with the 6.0.0--6.0.1 catalog upgrade and a TAP test. Also document large-transaction apply and failover slots.
The remove path now freezes via begin_part before slot teardown and resumes via complete_part, and join captures safe_horizon as the base point, so no defined group-slot function is left uncalled. Per-member confirmed_lsn is set NULL since it is not attributable from local catalogs. Comments are cut to the repo's minimal style.
The group replication slots branch and main independently inserted lines at the same locations in src/spock_manager.c and tests/tap/schedule, which produced merge conflicts. Reorder the additions so this branch matches main at those points. In spock_manager.c, keep utils/lsyscache.h immediately before utils/memutils.h and move utils/guc.h after it. In tests/tap/schedule, add the 032_lolor_largeobject_repset, 033_zodan_lolor_add_node, and 105_sub_disable_retransmit_after_disconnect entries and move the group replication slots test ahead of the regression tests section. This only reorders #include and test-schedule lines; there is no change in behavior.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesThe PR adds opt-in group replication slots with durable SQL state, safe advancement and repair controls, background workers, node lifecycle integration, documentation, and TAP coverage. It also adds large-transaction guidance, Zodan failover-slot documentation, and updates the Spock version to 6.0.1. Group replication slots
Large transaction apply guidance
Zodan failover slot documentation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 34 complexity · 0 duplication
Metric Results Complexity 34 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
sql/spock--6.0.0--6.0.1.sql (3)
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead exclusion clause.
spkgrp_…can never matchLIKE 'spk\_%'(the escaped_requires a literal underscore at position 4), so theslot_name <> COALESCE(spock.local_group_slot_name(), '')guard is unreachable. Harmless, but it costs alocal_group_slot_name()call per row and obscures intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sql/spock--6.0.0--6.0.1.sql` around lines 68 - 72, Remove the unreachable slot_name <> COALESCE(spock.local_group_slot_name(), '') predicate from the logical replication slot query, leaving the database, plugin, slot_type, and slot_name LIKE filters unchanged.
11-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider marking the new state tables as extension config so they survive
pg_dump.Without
pg_catalog.pg_extension_config_dump(),group_slot_state,group_slot_membership, andgroup_slot_member_progressare recreated empty on restore, losing membership generation and freeze state. Other Spock catalog tables use this pattern.♻️ Suggested addition after the table definitions
SELECT pg_catalog.pg_extension_config_dump('spock.group_slot_state', ''); SELECT pg_catalog.pg_extension_config_dump('spock.group_slot_membership', '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sql/spock--6.0.0--6.0.1.sql` around lines 11 - 54, Mark all three new state tables—spock.group_slot_state, spock.group_slot_membership, and spock.group_slot_member_progress—as extension configuration tables by adding pg_catalog.pg_extension_config_dump calls after their definitions, so their data is included in pg_dump and restored.
781-788: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
group_slot_refresh_members()is left executable by PUBLIC.It is an internal mutator of the group-slot catalog tables; revoke it alongside the other maintenance functions for a consistent surface.
🔒️ Proposed addition
REVOKE ALL ON FUNCTION spock.group_slot_complete_part(name) FROM PUBLIC; +REVOKE ALL ON FUNCTION spock.group_slot_refresh_members(bigint, interval) FROM PUBLIC;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sql/spock--6.0.0--6.0.1.sql` around lines 781 - 788, Add a REVOKE ALL statement for spock.group_slot_refresh_members() FROM PUBLIC alongside the existing group-slot maintenance function revocations, preserving the same privilege-restriction pattern.docs/managing/group_slots.md (1)
58-72: 🩺 Stability & Availability | 🔵 TrivialAdd WAL-retention guidance for a persistently blocked horizon.
While
stale_progress(or a join/part) holds, the slot never advances andpg_walgrows without bound. Worth documenting monitoring ofspock.group_slot_status()/pg_replication_slots.restart_lsnplus the interaction withmax_slot_wal_keep_size(which can invalidate the group slot and requirespock.repair_group_slot('recreate')).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/managing/group_slots.md` around lines 58 - 72, The “When advancement is refused” section should add WAL-retention guidance for persistently blocked horizons, especially stale_progress, join, and part states. Document monitoring spock.group_slot_status() and pg_replication_slots.restart_lsn, and explain that max_slot_wal_keep_size may invalidate the group slot, requiring spock.repair_group_slot('recreate').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/large_transaction_apply.md`:
- Around line 14-15: Update the compound modifier in the reproduction
description to hyphenate “600,000-row INSERT,” preserving the surrounding
wording and meaning.
- Around line 102-125: Revise the “Turn off spock.check_all_uc_indexes” section
to state that ALTER SYSTEM changes persistent subscriber-wide server
configuration, affecting all relevant apply workers rather than only the load
session. Instruct operators to record and explicitly restore the previous
setting after the load, and warn that an interrupted load may leave
secondary-unique conflict checking disabled.
- Around line 127-136: Update the unique-index conflict explanation in the
TRANSDISCARD section to identify TRANSDISCARD as the default
spock.exception_behaviour. Describe that failed apply rows are skipped and
logged, and only state that the subscription is disabled when configuration
explicitly sets sub_disable; remove the claim that sub_disable is the default.
- Around line 64-76: Update the transaction batching example in
docs/large_transaction_apply.md to state that the DO block must execute
top-level with auto-commit enabled, not inside an existing transaction. Document
that transaction-wrapped scripts or framework migrations should use client-side
batching instead.
In `@docs/managing/group_slots.md`:
- Around line 48-51: Update the safe LSN description to state that
spock.group_slot_min_downstream_lsn() considers the minimum confirmed_flush_lsn
across every spk_* logical slot in the database, including leftover or slow
subscription slots. Add spock.group_slot_safe_horizon() and the relevant
join/part functions to the Functions section where they are referenced.
In `@docs/modify/zodan/failover_slots.md`:
- Around line 115-118: Update the explanatory comment above primary_conninfo to
remove the blanket claim that pg_basebackup -R does not include dbname, and
accurately state that PostgreSQL 17+ records dbname only when it was explicitly
provided through the connection string or environment.
- Around line 8-15: Update the introductory mechanism statement in
failover_slots.md to say that PostgreSQL introduced native failover-slot
synchronization in PG17 and retained sync_replication_slots in PG18, then
separately identify the PG18 Spock worker registration change. Keep the numbered
PG15/16 and PG17/18 scope descriptions unchanged.
- Around line 210-218: Update the standby slot readiness query to exclude
temporary slots, evaluate only rows whose invalidation_reason is null, and
require both synced and failover to be true for the remaining logical slots.
Ensure the PG17/18 readiness check fails when any eligible slot violates these
conditions.
In `@samples/Z0DAN/zodremove.sql`:
- Around line 490-492: Update the RAISE NOTICE statement in the group-slot
completion flow to generate the recovery command with a valid SQL-literal
argument. Replace the unsupported %L placeholder usage by pre-formatting or
quote-escaping target_node_name with format(... %L ...) or quote_literal(),
while preserving the existing notice text and command.
In `@sql/spock--6.0.0--6.0.1.sql`:
- Around line 751-753: Update the generation-handling flow around the
group_slot_state lookup and v_newgen assignment to explicitly detect when no
metadata row exists, matching the guard and error behavior of
group_slot_begin_part(). Raise that error before evaluating v_gen + 1, while
preserving the existing increment path when metadata is present.
- Around line 476-499: Guard against a NULL v_target after assigning
COALESCE(target_lsn, v_safe) in the slot-advance flow. Before the
pg_replication_slots lookup and spock.group_slot_state UPDATE, raise a clear
exception when both target_lsn and the evaluated safe_lsn are NULL, preserving
the existing advance and update behavior for non-NULL targets.
- Around line 624-632: Update group_slot_begin_join so its missing-metadata
branch does not call spock.group_slot_worker_tick(). Seed the required
group_slot_state row directly using the existing metadata initialization path,
or raise the established directive error, then reread membership_generation
before computing v_newgen. Preserve the existing generation increment behavior.
In `@src/spock_group_slot.c`:
- Around line 161-176: Update the teardown SQL in the SPI_execute block so
group_slot_state rows are deleted only for replication slots successfully
selected and dropped by the NOT s.active cleanup query. Preserve metadata for
active slots and emit a warning when any slot remains active or otherwise was
not dropped, preventing orphaned slots from losing their cleanup metadata.
- Around line 133-140: Guard the metadata seed INSERT in the surrounding
initialization path before calling SPI_execute_with_args, using a to_regclass()
check for spock.group_slot_state. If the catalog table is absent, skip the
INSERT and emit only a warning so spock.create_node() continues; preserve the
existing result-code warning for executed INSERTs.
---
Nitpick comments:
In `@docs/managing/group_slots.md`:
- Around line 58-72: The “When advancement is refused” section should add
WAL-retention guidance for persistently blocked horizons, especially
stale_progress, join, and part states. Document monitoring
spock.group_slot_status() and pg_replication_slots.restart_lsn, and explain that
max_slot_wal_keep_size may invalidate the group slot, requiring
spock.repair_group_slot('recreate').
In `@sql/spock--6.0.0--6.0.1.sql`:
- Around line 68-72: Remove the unreachable slot_name <>
COALESCE(spock.local_group_slot_name(), '') predicate from the logical
replication slot query, leaving the database, plugin, slot_type, and slot_name
LIKE filters unchanged.
- Around line 11-54: Mark all three new state tables—spock.group_slot_state,
spock.group_slot_membership, and spock.group_slot_member_progress—as extension
configuration tables by adding pg_catalog.pg_extension_config_dump calls after
their definitions, so their data is included in pg_dump and restored.
- Around line 781-788: Add a REVOKE ALL statement for
spock.group_slot_refresh_members() FROM PUBLIC alongside the existing group-slot
maintenance function revocations, preserving the same privilege-restriction
pattern.
🪄 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 Plus
Run ID: 8dd116ca-d70d-4222-ad11-ff3477c37afa
📒 Files selected for processing (18)
docs/large_transaction_apply.mddocs/managing/group_slots.mddocs/managing/index.mddocs/modify/zodan/failover_slots.mddocs/spock_release_notes.mdinclude/spock.hinclude/spock_group_slot.hinclude/spock_worker.hsamples/Z0DAN/zodan.sqlsamples/Z0DAN/zodremove.sqlsql/spock--6.0.0--6.0.1.sqlsrc/spock.csrc/spock_functions.csrc/spock_group_slot.csrc/spock_manager.csrc/spock_worker.ctests/tap/scheduletests/tap/t/105_group_slots.pl
| ```sql | ||
| -- instead of one big INSERT | ||
| DO $$ | ||
| DECLARE | ||
| batch_size int := 5000; | ||
| start_id int; | ||
| BEGIN | ||
| FOR start_id IN 1 .. 60000 BY batch_size LOOP | ||
| INSERT INTO my_table SELECT ... FROM ... WHERE id BETWEEN start_id | ||
| AND start_id + batch_size - 1; | ||
| COMMIT; | ||
| END LOOP; | ||
| END $$; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files around target:\n'
git ls-files | rg '(^|/)large_transaction_apply\.md$|docs/large_transaction_apply\.md' || true
printf '\nTarget document excerpt:\n'
sed -n '1,120p' docs/large_transaction_apply.md 2>/dev/null | cat -n
printf '\nSearch for transaction/top-level mentions:\n'
rg -n -i 'transaction|top[- ]level|autocommit|commit|nested|DO \$\$|block' docs -S || true
printf '\nPostgreSQL relevant behavior from installed psql if available:\n'
if command -v psql >/dev/null 2>&1; then
psql --version || true
cat >/tmp/postgres_commit_in_do.sql <<'SQL'
\echo 'outside transaction 1'
DO $$
BEGIN
RAISE NOTICE 'block executed';
END $$;
\echo 'outside transaction 2'
BEGIN;
DO $$
BEGIN
RAISE NOTICE 'inside transaction';
END $$;
ROLLBACK;
SQL
psql -h localhost -U postgres -f /tmp/postgres_commit_in_do.sql 2>&1 || true
else
echo 'psql not available'
fiRepository: pgEdge/spock
Length of output: 50370
🌐 Web query:
PostgreSQL DO block COMMIT transaction control top-level query invalid transaction context ERROR
💡 Result:
The PostgreSQL error "invalid transaction context" (SQLSTATE 2D000) when using COMMIT or ROLLBACK within a DO block occurs because transaction control commands are only permitted when the DO block is executed in its own transaction [1][2]. If a DO block is executed inside an existing transaction block (e.g., if you have issued BEGIN or are running within a wrapper that starts a transaction), the code inside the DO block cannot commit or roll back that transaction [1][2]. To resolve this: 1. Execute the DO block at the top level: Ensure the DO command is not wrapped in an explicit BEGIN...COMMIT block or any other surrounding transaction [1][2]. 2. Avoid nested calls: Transaction control is only possible in DO or CALL invocations from the top level, or within nested CALL/DO chains that do not have any other intervening commands (such as SELECT or DML statements) [2][3]. 3. Check your client/environment: Some interfaces or GUI tools (like certain modes in pgAdmin or transaction-wrapped scripts) automatically start a transaction, which triggers this error [4]. Running the command directly in psql or ensuring the transaction is not auto-started is often necessary [4]. Essentially, COMMIT/ROLLBACK in a DO block is only valid if PostgreSQL can manage the transaction boundaries for that block itself [1][5]. If the block is already part of a larger, client-initiated transaction, it lacks the authority to terminate it [1][6].
Citations:
- 1: https://www.postgresql.org/docs/18/sql-do.html
- 2: https://www.postgresql.org/docs/18/plpgsql-transactions.html
- 3: https://www.postgresql.org/docs/14/plpgsql-transactions.html
- 4: https://stackoverflow.com/questions/78198667/i-cannot-commit-inside-a-procedure
- 5: https://www.postgresql.org/message-id/YzEgH26TCMdAsdPQ%40depesz.com
- 6: https://philipmcclarence.com/how-to-diagnose-and-fix-the-2d000-invalid_transaction_termination-error-code-in-postgres/
Document the top-level execution requirement.
COMMIT inside this DO block fails when the statement is run inside an existing BEGIN/transaction block, which is common in scripts or framework migrations. State that the example requires auto-commit/top-level execution and that transaction-wrapped environments need client-side batching instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/large_transaction_apply.md` around lines 64 - 76, Update the transaction
batching example in docs/large_transaction_apply.md to state that the DO block
must execute top-level with auto-commit enabled, not inside an existing
transaction. Document that transaction-wrapped scripts or framework migrations
should use client-side batching instead.
| **Important: you lose unique-index conflict protection while it is | ||
| off.** With the GUC on, Spock catches the case where an incoming row | ||
| collides with an existing local row on a secondary unique index (not | ||
| the primary key or replica identity) and resolves it through the normal | ||
| conflict path. With the GUC off, that detection does not happen. If | ||
| a colliding row arrives, the apply worker still tries the INSERT, the | ||
| heap rejects it with a duplicate-key violation, and (because the | ||
| default `spock.exception_behaviour` is `sub_disable`) the subscription | ||
| is disabled until an operator inspects `spock.exception_log` and | ||
| re-enables it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)large_transaction_apply\.md$|docs/'
echo "== target section =="
if [ -f docs/large_transaction_apply.md ]; then
sed -n '110,155p' docs/large_transaction_apply.md | nl -ba -v110
else
echo "docs/large_transaction_apply.md not found"
fi
echo "== search exception_behaviour mentions =="
rg -n --hidden --glob '!/.git' 'spock\.exception_behaviour|exception_behaviour|TRANSDISCARD|sub_disable|exception_log' . | head -200Repository: pgEdge/spock
Length of output: 4233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target section =="
python3 - <<'PY'
from pathlib import Path
p = Path('docs/large_transaction_apply.md')
lines = p.read_text().splitlines()
for i in range(120, 145+1):
print(f"{i}: {lines[i-1]}")
PY
echo "== exception_behaviour/sub_disable/TRANSDISCARD/exception_log =="
rg -n --hidden --glob '!/.git' 'spock\.exception_behaviour|exception_behaviour|TRANSDISCARD|sub_disable|exception_log' ./src ./spock ./pg 2>/dev/null || true
echo "== likely source files near exception_behaviour =="
rg -n --hidden --glob '!/.git' 'spock_exception_behaviour|sub_disable|TRANSDISCARD' . 2>/dev/null | head -300Repository: pgEdge/spock
Length of output: 33078
Don’t describe TRANSDISCARD as sub_disable.
spock.exception_behaviour defaults to TRANSDISCARD; in that mode failed apply rows are skipped and logged, not the whole transaction discarded from PostgreSQL, and the apply worker does not reach sub_disable unless configuration explicitly sets it. Update this section to state the actual default and avoid calling it sub_disable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/large_transaction_apply.md` around lines 127 - 136, Update the
unique-index conflict explanation in the TRANSDISCARD section to identify
TRANSDISCARD as the default spock.exception_behaviour. Describe that failed
apply rows are skipped and logged, and only state that the subscription is
disabled when configuration explicitly sets sub_disable; remove the claim that
sub_disable is the default.
| * A per-database background worker periodically computes the **safe LSN** as the | ||
| minimum confirmed flush/replay LSN across all active and relevant downstream | ||
| members, and advances the group slot to that horizon — but only when it is | ||
| safe to do so. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Horizon description is looser than the implementation.
spock.group_slot_min_downstream_lsn() takes the minimum confirmed_flush_lsn over every spk_* logical slot in the database, not only slots for active/relevant members — any leftover or slow subscription slot pins the horizon. Also, spock.group_slot_safe_horizon() and the join/part functions are referenced elsewhere in this page but absent from the Functions section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/managing/group_slots.md` around lines 48 - 51, Update the safe LSN
description to state that spock.group_slot_min_downstream_lsn() considers the
minimum confirmed_flush_lsn across every spk_* logical slot in the database,
including leftover or slow subscription slots. Add
spock.group_slot_safe_horizon() and the relevant join/part functions to the
Functions section where they are referenced.
Add the missing utils/snapmgr.h include so the extension builds under -Werror=implicit-function-declaration (this was failing every CI job). Also fix the review findings: keep group-slot metadata for a still-active slot instead of orphaning it, guard the metadata seed against a missing catalog during version skew, raise clear errors when group-slot metadata is absent (advance, begin_join, complete_part) instead of NOT NULL violations or creating a slot inside a write transaction, quote the recovery command in zodremove, and tighten the docs.
get_database_name() moved from commands/dbcommands.h to utils/lsyscache.h in PG19. Without it the PG19 build fails with an implicit-declaration error (and silently truncates the returned pointer under older compilers).
1 participant
This change adds group replication slots to Spock: one internally managed, permanently inactive logical slot per Spock database that pins WAL at the group-safe position for a multi-master mesh. It protects WAL retention across membership changes so a node can rejoin and catch up after downtime without WAL being removed too early, while avoiding over-retention when a downstream is merely slow. The feature is off by default (spock.group_slots_enabled) and adds no overhead unless enabled. A per-database group-slot background worker is started and stopped by the Spock manager based on the toggle, and the manager reloads configuration on SIGHUP so the setting takes effect without a restart.
Catalog state tracks group membership, membership generations, and per-member replay progress, and explicit node lifecycle handling (active, joining, parting) ensures membership churn does not lose or strand WAL. Testing adds the TAP test 105_group_slots and runs against the existing suite. The branch also includes a small commit that aligns include ordering in src/spock_manager.c and the entries in tests/tap/schedule with main, so it merges into main with no conflicts.