Rework read-only mode: use Postgres native DefaultXactReadOnly#376
Open
Rework read-only mode: use Postgres native DefaultXactReadOnly#376
Conversation
📝 WalkthroughWalkthroughThe patch renames the enum value READONLY_USER to READONLY_LOCAL and adjusts associated option mappings. It elevates failure severity for replication read-only detection, adds interrupt and config reload handling plus feedback in the apply loop, and simplifies read-only enforcement by globally setting transaction read-only state based on mode and superuser status. Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/spock.c (1)
116-120: Consider adding alocalGUC literal and keepinguseras a hidden compatibility alias.The enum was renamed to
READONLY_LOCAL, but the exposed value is still"user". Accepting"local"improves clarity and keeps backward compatibility.♻️ Suggested adjustment
static const struct config_enum_entry readonly_options[] = { {"off", READONLY_OFF, false}, - {"user", READONLY_LOCAL, false}, + {"local", READONLY_LOCAL, false}, + {"user", READONLY_LOCAL, true}, {"all", READONLY_ALL, false}, {NULL, 0, false} };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock.c` around lines 116 - 120, The readonly_options enum currently exposes "user" while the internal enum is READONLY_LOCAL; update the GUC literals to add "local" as the primary visible option and keep "user" as a hidden/compatibility alias in the readonly_options array that is returned by the config system; specifically modify the readonly_options initializer (refer to readonly_options and READONLY_LOCAL) to include an entry for "local" mapping to READONLY_LOCAL and mark the existing "user" entry as hidden/alias so both parse to READONLY_LOCAL while preserving backward compatibility.
🤖 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 2887-2899: In the READONLY_ALL branch, replacing the inner
"continue" (when xact_had_exception is true) with a break ensures the worker
exits the loop to run the outer post-loop cleanup/exception/subscription
handling; update the code in spock_apply.c so that after
maybe_send_feedback(...) you do "if (xact_had_exception) break;" (or set a local
flag and break) instead of "continue", preserving any per-iteration cleanup and
allowing the outer post-loop handling to execute.
In `@src/spock_readonly.c`:
- Around line 65-68: spock_ropost_parse_analyze() and spock_roExecutorStart()
currently unconditionally reset DefaultXactReadOnly to false; instead, before
you change DefaultXactReadOnly save its current value into a local variable
(e.g., prev_DefaultXactReadOnly) and record whether Spock actually enforced
readonly (e.g., did_force_readonly = (spock_readonly >= READONLY_LOCAL &&
!superuser())); only set DefaultXactReadOnly when did_force_readonly is true,
and on exit restore DefaultXactReadOnly from prev_DefaultXactReadOnly only if
did_force_readonly was true; apply this save/restore pattern in both
spock_ropost_parse_analyze() and spock_roExecutorStart(), referencing the
DefaultXactReadOnly, spock_readonly, READONLY_LOCAL, and superuser() symbols to
locate the places to change.
---
Nitpick comments:
In `@src/spock.c`:
- Around line 116-120: The readonly_options enum currently exposes "user" while
the internal enum is READONLY_LOCAL; update the GUC literals to add "local" as
the primary visible option and keep "user" as a hidden/compatibility alias in
the readonly_options array that is returned by the config system; specifically
modify the readonly_options initializer (refer to readonly_options and
READONLY_LOCAL) to include an entry for "local" mapping to READONLY_LOCAL and
mark the existing "user" entry as hidden/alias so both parse to READONLY_LOCAL
while preserving backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0080300-bc1e-4d74-ade7-87ba2b6fbdb5
📒 Files selected for processing (4)
include/spock_readonly.hsrc/spock.csrc/spock_apply.csrc/spock_readonly.c
danolivo
force-pushed
the
readonly-refactoring
branch
from
5f69140 to
e60b508
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/spock_apply.c (1)
2901-2910:⚠️ Potential issue | 🟠 MajorREADONLY_ALL exception path can still spin and misses postmaster-death handling.
In this branch,
WaitLatchOrSocket(... WL_SOCKET_READABLE ...)+continue(Line 2903 onward) can return immediately on a perpetually readable socket, causing tight-loop churn; it also skips checkingWL_POSTMASTER_DEATHin this path. Prefer a latch/timeout wait here and preserve the emergency bailout.💡 Suggested patch
if (xact_had_exception) { - rc = WaitLatchOrSocket(&MyProc->procLatch, - WL_SOCKET_READABLE | WL_LATCH_SET | - WL_TIMEOUT | WL_POSTMASTER_DEATH, - fd, 1000L); - - ResetLatch(&MyProc->procLatch); + rc = WaitLatch(&MyProc->procLatch, + WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH, + 1000L, + WAIT_EVENT_LOGICAL_APPLY_MAIN); + if (rc & WL_POSTMASTER_DEATH) + proc_exit(1); + ResetLatch(&MyProc->procLatch); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_apply.c` around lines 2901 - 2910, In the xact_had_exception branch (around the WaitLatchOrSocket call), don't call WaitLatchOrSocket with WL_SOCKET_READABLE and then continue because a perpetually readable socket can cause a tight spin and skip postmaster-death handling; replace that call with WaitLatch(&MyProc->procLatch, WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH, timeout) (i.e., use WaitLatch instead of WaitLatchOrSocket), keep ResetLatch(&MyProc->procLatch) after the wait, and explicitly handle the WL_POSTMASTER_DEATH case (or call proc_exit/emergency bailout) so the emergency bailout path is preserved instead of looping continuously. Ensure you reference the xact_had_exception branch, MyProc->procLatch, WaitLatchOrSocket (old) and WaitLatch (new), ResetLatch, and WL_POSTMASTER_DEATH in your changes.
🤖 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.c`:
- Around line 116-119: The readonly_options array currently only maps the old
name "user" to READONLY_LOCAL, so setting spock.readonly = 'local' is rejected;
update the readonly_options entries to include "local" as the canonical string
mapping to READONLY_LOCAL and keep "user" as a backward-compatible alias (both
mapping to READONLY_LOCAL) so the parser accepts either name; modify the static
const struct config_enum_entry readonly_options[] (and any related
parsing/validation that references READONLY_LOCAL) accordingly.
---
Duplicate comments:
In `@src/spock_apply.c`:
- Around line 2901-2910: In the xact_had_exception branch (around the
WaitLatchOrSocket call), don't call WaitLatchOrSocket with WL_SOCKET_READABLE
and then continue because a perpetually readable socket can cause a tight spin
and skip postmaster-death handling; replace that call with
WaitLatch(&MyProc->procLatch, WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
timeout) (i.e., use WaitLatch instead of WaitLatchOrSocket), keep
ResetLatch(&MyProc->procLatch) after the wait, and explicitly handle the
WL_POSTMASTER_DEATH case (or call proc_exit/emergency bailout) so the emergency
bailout path is preserved instead of looping continuously. Ensure you reference
the xact_had_exception branch, MyProc->procLatch, WaitLatchOrSocket (old) and
WaitLatch (new), ResetLatch, and WL_POSTMASTER_DEATH in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5595b87b-7ef4-4477-90de-6e1d4fe06adb
📒 Files selected for processing (4)
include/spock_readonly.hsrc/spock.csrc/spock_apply.csrc/spock_readonly.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spock_readonly.c
Instead of manually filtering command types in post_parse_analyze and ExecutorStart hooks, set XactReadOnly directly for non-superuser sessions. This leverages Postgres's built-in read-only transaction enforcement instead of reimplementing it. To distinguish Spock-imposed read-only from user/GUC-imposed read-only, compare XactReadOnly against the transaction_read_only GUC value. Rename READONLY_USER to READONLY_LOCAL to better reflect that the mode restricts local (non-replication) connections. In the apply worker, move CHECK_FOR_INTERRUPTS and config reload to the top of the replay loop so read-only mode changes are detected before applying the next transaction. When READONLY_ALL is active, the worker sends keepalive feedback but skips applying new transactions. Escalate the READONLY_ALL violation in replication_handler from ERROR to PANIC.
danolivo
force-pushed
the
readonly-refactoring
branch
from
e60b508 to
fdb7dd1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/spock_apply.c (1)
2887-2912:⚠️ Potential issue | 🔴 CriticalREADONLY_ALL exception path can trap the worker in the inner loop and skip finalization flow.
With
xact_had_exception == true,continueat Line 2909 keeps control inside the inner loop, so the post-loop exception/subscription handling block is never reached. This can stall apply progress indefinitely while in READONLY_ALL.🐛 Minimal fix
- if (spock_readonly == READONLY_ALL) + if (spock_readonly == READONLY_ALL) { /* * Send feedback to keep walsender alive - we may avoid it * with introduction of TCP keepalive approach. */ maybe_send_feedback(applyconn, last_received, &last_receive_timestamp); - - /* - * In case of an exception we can't break out of the loop - * because exception processing code may also modify the - * database. Wait briefly and continue to the next iteration. - */ - if (xact_had_exception) - { - rc = WaitLatchOrSocket(&MyProc->procLatch, - WL_SOCKET_READABLE | WL_LATCH_SET | - WL_TIMEOUT | WL_POSTMASTER_DEATH, - fd, 1000L); - - ResetLatch(&MyProc->procLatch); - continue; - } break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_apply.c` around lines 2887 - 2912, The READONLY_ALL branch currently does a continue when xact_had_exception is true, which keeps execution inside the inner loop and skips the post-loop exception/subscription finalization; change the control flow so that after maybe_send_feedback and the WaitLatchOrSocket call the code breaks out of the inner loop (not continue), ensuring the subsequent post-loop exception/subscription handling runs (update the block around spock_readonly == READONLY_ALL, referencing xact_had_exception, maybe_send_feedback and WaitLatchOrSocket to use break instead of continue).
🧹 Nitpick comments (1)
src/spock_readonly.c (1)
68-89: Extract duplicated read-only sync logic into one helper.
spock_ropost_parse_analyze()andspock_roExecutorStart()now carry identical state-sync logic. A shared static helper would reduce drift risk between hooks.♻️ Suggested refactor
+static inline void +spock_sync_xact_readonly(void) +{ + if (spock_readonly >= READONLY_LOCAL && !superuser()) + XactReadOnly = true; + else if (XactReadOnly) + { + const char *value = GetConfigOption("transaction_read_only", false, false); + if (strcmp(value, "off") == 0) + XactReadOnly = false; + } +} + void spock_ropost_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) { - ... + spock_sync_xact_readonly(); } void spock_roExecutorStart(QueryDesc *queryDesc, int eflags) { - ... + spock_sync_xact_readonly(); }Also applies to: 95-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_readonly.c` around lines 68 - 89, Both spock_ropost_parse_analyze and spock_roExecutorStart contain duplicated logic that syncs spock_readonly with the Postgres XactReadOnly state; extract that logic into a single static helper (e.g., sync_spock_readonly_state) and have both functions call it. The helper should encapsulate the current conditional flow that checks spock_readonly >= READONLY_LOCAL, superuser(), sets XactReadOnly, and restores it by reading transaction_read_only via GetConfigOption and strcmp(value, "off") to detect and revert Spock-imposed changes. Replace the duplicated blocks in spock_ropost_parse_analyze and spock_roExecutorStart with a call to the new static function to avoid drift.
🤖 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 2887-2912: The READONLY_ALL branch currently does a continue when
xact_had_exception is true, which keeps execution inside the inner loop and
skips the post-loop exception/subscription finalization; change the control flow
so that after maybe_send_feedback and the WaitLatchOrSocket call the code breaks
out of the inner loop (not continue), ensuring the subsequent post-loop
exception/subscription handling runs (update the block around spock_readonly ==
READONLY_ALL, referencing xact_had_exception, maybe_send_feedback and
WaitLatchOrSocket to use break instead of continue).
---
Nitpick comments:
In `@src/spock_readonly.c`:
- Around line 68-89: Both spock_ropost_parse_analyze and spock_roExecutorStart
contain duplicated logic that syncs spock_readonly with the Postgres
XactReadOnly state; extract that logic into a single static helper (e.g.,
sync_spock_readonly_state) and have both functions call it. The helper should
encapsulate the current conditional flow that checks spock_readonly >=
READONLY_LOCAL, superuser(), sets XactReadOnly, and restores it by reading
transaction_read_only via GetConfigOption and strcmp(value, "off") to detect and
revert Spock-imposed changes. Replace the duplicated blocks in
spock_ropost_parse_analyze and spock_roExecutorStart with a call to the new
static function to avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1824a4f-54be-4418-93b9-88195988dc05
📒 Files selected for processing (4)
include/spock_readonly.hsrc/spock.csrc/spock_apply.csrc/spock_readonly.c
🚧 Files skipped from review as they are similar to previous changes (2)
- src/spock.c
- include/spock_readonly.h
Contributor
Author
I think it is not good or necessary change. So, it seems we can keep it as is. |
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
1 participant
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.
post_parse_analyzeandExecutorStarthooks with settingXactReadOnlydirectly for non-superusersessions, leveraging Postgres's built-in read-only transaction enforcement
comparing
XactReadOnlyagainst thetransaction_read_onlyGUC valueREADONLY_USERtoREADONLY_LOCALto better reflect that the moderestricts local (non-replication) connections
CHECK_FOR_INTERRUPTSand config reload to thetop of the replay loop so read-only mode changes are detected before applying
the next transaction
READONLY_ALLis active, the apply worker sends keepalive feedback butskips applying new transactions
READONLY_ALLviolation inreplication_handlerfromERRORto
PANIC