Replace wal_sender_timeout-based liveness with TCP keepalive.#373
Replace wal_sender_timeout-based liveness with TCP keepalive.#373mason-sharp merged 6 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an apply-worker idle-timeout GUC (header exported), removes the feedback-frequency config and helper logic, adjusts replication connection parameters and TCP keepalive defaults, and refactors apply-worker idle-timer and COPY receive handling to use the new idle-timeout. 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/spock.c (1)
345-356: Consider making TCP keepalive parameters configurable.The hardcoded values (idle=10s, interval=5s, count=3) result in ~25s dead connection detection. While reasonable for most deployments, high-latency or unreliable network environments may experience false-positive disconnects.
Consider exposing these as GUCs (e.g.,
spock.keepalives_idle,spock.keepalives_interval,spock.keepalives_count) to allow tuning without code changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock.c` around lines 345 - 356, Replace the hardcoded TCP keepalive literals in the keys/vals block with configurable GUC-backed values: define GUCs (e.g., spock.keepalives_idle, spock.keepalives_interval, spock.keepalives_count) as int variables (suggest names spock_keepalives_idle, spock_keepalives_interval, spock_keepalives_count) during module initialization (e.g., in _PG_init or the existing GUC registration area), register them with DefineCustomIntVariable, and then use those variables' stringified values when populating vals[] for the keys "keepalives_idle", "keepalives_interval", and "keepalives_count" in the code that sets keys[i]/vals[i]; keep the default values 10/5/3 and ensure bounds checking on the GUCs (positive integers) when registering.
🤖 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.c`:
- Around line 345-356: Replace the hardcoded TCP keepalive literals in the
keys/vals block with configurable GUC-backed values: define GUCs (e.g.,
spock.keepalives_idle, spock.keepalives_interval, spock.keepalives_count) as int
variables (suggest names spock_keepalives_idle, spock_keepalives_interval,
spock_keepalives_count) during module initialization (e.g., in _PG_init or the
existing GUC registration area), register them with DefineCustomIntVariable, and
then use those variables' stringified values when populating vals[] for the keys
"keepalives_idle", "keepalives_interval", and "keepalives_count" in the code
that sets keys[i]/vals[i]; keep the default values 10/5/3 and ensure bounds
checking on the GUCs (positive integers) when registering.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69aec04d-1d81-4de7-913f-14b9f06f8761
📒 Files selected for processing (3)
include/spock.hsrc/spock.csrc/spock_apply.c
| * kernel ACKs them, but no data is being sent. | ||
| */ | ||
| if (rc & WL_TIMEOUT) | ||
| if (rc & WL_TIMEOUT && spock_apply_idle_timeout > 0) |
There was a problem hiding this comment.
It seems like if walsender just doesn't have data to send for a long time, subscriber will restart. Am I wrong?
It would be better to modify walsender little: skip keepalive messages being busy and rely on TCP status. But send keepalive messages if no data arrives from the WAL. In this case we don't need any subscriber-side GUC at all.
There was a problem hiding this comment.
It does a slightly different task. We are relying on TCP_KEEPALIVE. The idle time is just for the guard; if the work is stuck at the application level, but the kernel TCP keep-alive will continue, there will be no way to restart the wal_sender.
|
@ibrarahmad Needs rebase |
mason-sharp
left a comment
mason-sharp
left a comment
There was a problem hiding this comment.
Added a comment about having a non-zero wal_sender_timeout.
Also, needs a rebase.
Also, could use that test file.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/spock_apply.c (1)
2852-2863:⚠️ Potential issue | 🟠 MajorThis can reconnect an idle-but-healthy upstream.
last_receive_timestamponly moves whenCopyDataarrives. Because the replication connection now runs withwal_sender_timeout = 0, PostgreSQL's walsender returns early fromWalSndKeepaliveIfNecessary()and won't send protocol keepalives, so a publisher that is simply caught up and idle can still hit this reconnect path afterspock.apply_idle_timeout. As written, this safety net can't distinguish “hung” from “idle”; it needs a sender heartbeat that surviveswal_sender_timeout = 0, or this GUC should default to0. (doxygen.postgresql.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_apply.c` around lines 2852 - 2863, The idle-timeout check using last_receive_timestamp can disconnect a healthy but caught-up publisher because CopyData isn't updated by walsender keepalives when wal_sender_timeout = 0; update the logic so spock.apply_idle_timeout does not trigger in that case: either change the GUC spock_apply_idle_timeout default to 0, or add a guard in the timeout branch (the block that sets MySpockWorker->worker_status = SPOCK_WORKER_STATUS_STOPPED and elog(ERROR, ...)) to skip reconnect when the upstream is only sending WAL keepalives (i.e., detect/update a separate last_keepalive_timestamp on WalSndKeepaliveIfNecessary/keepalive handling and use that for idle detection), or check wal_sender_timeout and treat spock_apply_idle_timeout as disabled when wal_sender_timeout == 0; update references to last_receive_timestamp, CopyData handling, and the reconnect path accordingly.
🤖 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_apply.c`:
- Around line 2852-2863: The idle-timeout check using last_receive_timestamp can
disconnect a healthy but caught-up publisher because CopyData isn't updated by
walsender keepalives when wal_sender_timeout = 0; update the logic so
spock.apply_idle_timeout does not trigger in that case: either change the GUC
spock_apply_idle_timeout default to 0, or add a guard in the timeout branch (the
block that sets MySpockWorker->worker_status = SPOCK_WORKER_STATUS_STOPPED and
elog(ERROR, ...)) to skip reconnect when the upstream is only sending WAL
keepalives (i.e., detect/update a separate last_keepalive_timestamp on
WalSndKeepaliveIfNecessary/keepalive handling and use that for idle detection),
or check wal_sender_timeout and treat spock_apply_idle_timeout as disabled when
wal_sender_timeout == 0; update references to last_receive_timestamp, CopyData
handling, and the reconnect path accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f499ec7-1af5-4d57-bcd9-56189b254fe8
📒 Files selected for processing (3)
include/spock.hsrc/spock.csrc/spock_apply.c
✅ Files skipped from review due to trivial changes (1)
- include/spock.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spock.c
|
Did a rebase + one additional commit |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/configuring.md (2)
194-209: Documentation looks good; consider adding language specifier to code block.The guidance to set
wal_sender_timeoutto a conservative value like5minis appropriate and aligns well with the new TCP keepalive-based liveness detection. The explanation of why PostgreSQL's default60sis problematic for busy apply workers is clear and helpful.📝 Optional: Add language specifier to code block
The code block at line 199 should specify a language for better rendering:
-``` +```conf wal_sender_timeout = '5min'This addresses the markdownlint warning and improves documentation quality. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/configuring.mdaround lines 194 - 209, Add a language specifier to the
fenced code block showing the postgresql.conf setting so the snippet renders
with proper highlighting; update the block that contains "wal_sender_timeout =
'5min'" to use a conf (or ini) code fence (e.g., ```conf) and ensure the snippet
still shows the postgresql.conf context and the wal_sender_timeout setting.</details> --- `210-220`: **Add language specifier to code block for better rendering.** The documentation accurately describes `spock.apply_idle_timeout` with the correct default value of `300` seconds. The explanation of the safety net mechanism and timer reset behavior is clear. Add a language specifier to the code example for improved rendering: <details> <summary>📝 Code block improvement</summary> ```diff -``` +```conf spock.apply_idle_timeout = 300 ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/configuring.mdaround lines 210 - 220, Update the fenced code block
that shows the spock.apply_idle_timeout example to include a language specifier
by changing the opening fence toconf so the snippet `spock.apply_idle_timeout = 300` is rendered with the "conf" language; locate the code block near the `spock.apply_idle_timeout` description and replace the current triple backtick opener withconf (keep the existing content and
closing backticks unchanged).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/configuring.md:
- Around line 194-209: Add a language specifier to the fenced code block showing
the postgresql.conf setting so the snippet renders with proper highlighting;
update the block that contains "wal_sender_timeout = '5min'" to use a conf (or
ini) code fence (e.g., ```conf) and ensure the snippet still shows the
postgresql.conf context and the wal_sender_timeout setting.- Around line 210-220: Update the fenced code block that shows the
spock.apply_idle_timeout example to include a language specifier by changing the
opening fence toconf so the snippet `spock.apply_idle_timeout = 300` is rendered with the "conf" language; locate the code block near the `spock.apply_idle_timeout` description and replace the current triple backtick opener withconf (keep the existing content and closing backticks unchanged).</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `ed326f14-e345-446b-9fff-bff123ef2a88` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ed05b18925556944bdfd17a423ec7e2b3d29cd9e and e43ab707b9c82ab025b1de254a6c81693489f6d9. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/configuring.md` * `src/spock.c` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/spock.c </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
The apply worker previously relied on wal_sender_timeout as both a server-side disconnect trigger and an indirect keepalive pressure on the subscriber. This caused spurious disconnects in two scenarios: a flood of 'w' messages keeping the subscriber too busy to send 'r' feedback in time, and large transactions whose apply time exceeded wal_sender_timeout. The workaround was maybe_send_feedback(), which force-sent 'r' after every 10 'w' messages or wal_sender_timeout/2, whichever came first. This was a fragile band-aid that coupled subscriber behavior to a server GUC it cannot control. Replace the entire mechanism with a clean two-layer model: - TCP keepalive (keepalives_idle=10, keepalives_interval=5, keepalives_count=3) is the primary liveness detector on both sides. A dead network or crashed host is detected in ~25 seconds. - wal_sender_timeout=0 is set on replication connections so the walsender never disconnects due to missing 'r' feedback. Liveness on the server side is now handled entirely by TCP keepalive. - spock.apply_idle_timeout (default 300s) is a subscriber-side safety net for a hung-but-connected walsender whose TCP keepalive probes are answered by the kernel but sends no data. Set to 0 to disable. Fix a bug in last_receive_timestamp handling: it was updated unconditionally after every PQgetCopyData call, including when r==0 (no data available). Each 1-second WL_TIMEOUT spin silently reset the timer, making apply_idle_timeout never fire. Move the update to after the r==0 guard so it reflects actual data receipt only. Remove maybe_send_feedback() as it is no longer needed.
Also document conservative wal_sender_timeout and apply_idle_timeout settings.
3 participants
The apply worker previously relied on wal_sender_timeout as both a server-side disconnect trigger and an indirect keepalive pressure on the subscriber. This caused spurious disconnects in two scenarios: a flood of 'w' messages keeping the subscriber too busy to send 'r' feedback in time, and large transactions whose apply time exceeded wal_sender_timeout.
The workaround was maybe_send_feedback(), which force-sent 'r' after every 10 'w' messages or wal_sender_timeout/2, whichever came first. This was a fragile band-aid that coupled subscriber behavior to a server GUC it cannot control.
Replace the entire mechanism with a clean two-layer model:
TCP keepalive (keepalives_idle=10, keepalives_interval=5, keepalives_count=3) is the primary liveness detector on both sides. A dead network or crashed host is detected in ~25 seconds.
wal_sender_timeout=0 is set on replication connections so the walsender never disconnects due to missing 'r' feedback. Liveness on the server side is now handled entirely by TCP keepalive.
spock.apply_idle_timeout (default 300s) is a subscriber-side safety net for a hung-but-connected walsender whose TCP keepalive probes are answered by the kernel but sends no data. Set to 0 to disable.
Fix a bug in last_receive_timestamp handling: it was updated unconditionally after every PQgetCopyData call, including when r==0 (no data available). Each 1-second WL_TIMEOUT spin silently reset the timer, making apply_idle_timeout never fire. Move the update to after the r==0 guard so it reflects actual data receipt only.
Remove maybe_send_feedback() as it is no longer needed.
SPOC-419