SPOC-502: Replace opaque "unavailable" message in exception_log with informative discard context#424
Conversation
📝 WalkthroughWalkthroughThis PR changes internal exception logging and error-message handling across three C modules: synthesizing messages that include exception-behaviour names and action counters, asserting non-null error messages in the exception log path, and adding a conditional header include. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spock_apply.c (1)
2413-2435:⚠️ Potential issue | 🔴 CriticalCritical:
fallback_msgis used outside its scope — dangling stack pointer.
fallback_msgis declared inside the innerelse { ... }block (Line 2419), assigned toerror_msgat Line 2425, and then the block closes at Line 2426.error_msgis subsequently dereferenced at Line 2435 insideadd_entry_to_exception_log(...), at which point the storage duration offallback_msghas ended per C99 §6.2.4 — this is undefined behavior. Today it may appear to work because compilers typically don't reuse the stack slot before the function returns, but a future codegen / stack-protector / ASan build can turn this into garbage text (or worse) written tospock.exception_log.Hoist the buffer to the scope where
error_msglives, matching the pattern already used inlog_insert_exception(Line 1230).🐛 Proposed fix
if (should_log_exception(failed)) { char *error_msg; + char fallback_msg[128]; /* Just to be paranoid */ Assert(my_exception_log_index >= 0); if (failed) error_msg = edata->message; else if (xact_action_counter == exception_log_ptr[my_exception_log_index].failed_action && exception_log_ptr[my_exception_log_index].initial_error_message[0] != '\0') error_msg = exception_log_ptr[my_exception_log_index].initial_error_message; else { - char fallback_msg[128]; - snprintf(fallback_msg, sizeof(fallback_msg), "%s: tuple discarded due to exception at command_counter %u", get_exception_behaviour_name(), exception_log_ptr[my_exception_log_index].failed_action); error_msg = fallback_msg; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_apply.c` around lines 2413 - 2435, The code assigns error_msg to point at a locally scoped buffer fallback_msg inside the inner else, then passes error_msg to add_entry_to_exception_log after that buffer is out of scope; hoist the fallback_msg buffer to the same scope as error_msg (i.e., declare a char fallback_msg[128] alongside error_msg before the if/else), then use snprintf into that buffer in the else branch and set error_msg = fallback_msg so the pointer remains valid when calling add_entry_to_exception_log (follow the same scoping pattern used by log_insert_exception).
🧹 Nitpick comments (1)
src/spock_exception_handler.c (1)
191-194: Drop the now-dead"unavailable"fallback to make the contract unambiguous.With
Assert(error_message != NULL)the ternaryerror_message != NULL ? error_message : "unavailable"is dead in debug builds and silently reintroduces the exact opaque message this PR is trying to eliminate in release builds. Per the PR objective,add_entry_to_exception_logis now a thin persistence layer with a non-NULL contract — honor that here too.♻️ Proposed simplification
Assert(error_message != NULL); - values[Anum_exception_log_error_message - 1] = - CStringGetTextDatum(error_message != NULL ? error_message : "unavailable"); + values[Anum_exception_log_error_message - 1] = CStringGetTextDatum(error_message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_exception_handler.c` around lines 191 - 194, The code currently asserts error_message != NULL but then uses a ternary fallback to "unavailable" when populating values[Anum_exception_log_error_message - 1]; remove the dead fallback and pass error_message directly to CStringGetTextDatum (i.e., replace CStringGetTextDatum(error_message != NULL ? error_message : "unavailable") with CStringGetTextDatum(error_message)), keeping the Assert(error_message != NULL) and leaving add_entry_to_exception_log's non-NULL contract intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/spock_apply.c`:
- Around line 2413-2435: The code assigns error_msg to point at a locally scoped
buffer fallback_msg inside the inner else, then passes error_msg to
add_entry_to_exception_log after that buffer is out of scope; hoist the
fallback_msg buffer to the same scope as error_msg (i.e., declare a char
fallback_msg[128] alongside error_msg before the if/else), then use snprintf
into that buffer in the else branch and set error_msg = fallback_msg so the
pointer remains valid when calling add_entry_to_exception_log (follow the same
scoping pattern used by log_insert_exception).
---
Nitpick comments:
In `@src/spock_exception_handler.c`:
- Around line 191-194: The code currently asserts error_message != NULL but then
uses a ternary fallback to "unavailable" when populating
values[Anum_exception_log_error_message - 1]; remove the dead fallback and pass
error_message directly to CStringGetTextDatum (i.e., replace
CStringGetTextDatum(error_message != NULL ? error_message : "unavailable") with
CStringGetTextDatum(error_message)), keeping the Assert(error_message != NULL)
and leaving add_entry_to_exception_log's non-NULL contract intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5784fba1-8591-4732-85c5-3cd3c0562b9e
⛔ Files ignored due to path filters (1)
tests/regress/expected/replication_set.outis excluded by!**/*.out
📒 Files selected for processing (3)
src/spock_apply.csrc/spock_exception_handler.csrc/spock_injection.c
When a tuple is skipped during TRANSDISCARD/SUB_DISABLE dry-run replay (i.e. it is not the command that originally failed), the error_message column in spock.exception_log previously stored the opaque string "unavailable". Replace it with a message of the form: "<mode>: tuple discarded due to exception at command_counter N" Also fix a pre-existing asymmetry in handle_insert: xact_action_counter was incremented after spock_read_insert, whereas handle_update and handle_delete both increment it before the read. Move the increment (and errcallback_arg.action_name assignment) to match.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_exception_handler.c`:
- Around line 191-194: The code currently uses a ternary fallback
CStringGetTextDatum(error_message != NULL ? error_message : "unavailable") which
reintroduces the stale "unavailable" value; since Assert(error_message != NULL)
guarantees non-NULL by contract, replace the ternary with a direct use of
error_message (i.e., call CStringGetTextDatum(error_message)) for
values[Anum_exception_log_error_message - 1], leaving the existing Assert in
place to catch violations in debug builds.
🪄 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: 4929aad9-0435-46e0-9fdf-96d250683143
⛔ Files ignored due to path filters (1)
tests/regress/expected/replication_set.outis excluded by!**/*.out
📒 Files selected for processing (3)
src/spock_apply.csrc/spock_exception_handler.csrc/spock_injection.c
✅ Files skipped from review due to trivial changes (1)
- src/spock_injection.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spock_apply.c
1 participant
The message names the active
spock.exception_behaviourGUC value and thefailed_actioncounter from the exception log slot, allowing an operator to cross-reference the log entry that holds the actual error.Key design decisions:
log_insert_exception(covering INSERT/UPDATE/DELETE/TRUNCATE) and inline inhandle_sql_or_exception(covering DDL/SQL queued messages), where all required context is naturally available — keepingadd_entry_to_exception_loga thin persistence layer with no knowledge of exception modes.get_exception_behaviour_name()maps the enum to its GUC string, avoiding duplication between the two call sites.add_entry_to_exception_lognow assertserror_message != NULL, making the contract explicit.Additional fix
handle_insertincrementedxact_action_counterand seterrcallback_arg.action_nameafterspock_read_insert, whereashandle_updateandhandle_deleteboth do so before the read. This asymmetry meant that a "can't find relation" failure in INSERT was not counted as an action. Both are moved before the read to match the established pattern.