Ship a valid SET search_path with replicated DDL for an empty path#502
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors Changessearch_path Serialization Refactor
Relcache Lookup Structured Error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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_functions.c (1)
2240-2240: 💤 Low valueUse
appendStringInfoStringfor literal strings.
appendStringInfo(buf, ", ")works butappendStringInfoStringis more appropriate when there are no format specifiers—it avoids parsing the format string.Suggested fix
if (lc != list_head(path_list)) - appendStringInfo(buf, ", "); + appendStringInfoString(buf, ", ");🤖 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 `@src/spock_functions.c` at line 2240, Replace the `appendStringInfo(buf, ", ")` call with `appendStringInfoString(buf, ", ")` to avoid unnecessary format string parsing when appending a literal string with no format specifiers. The `appendStringInfoString` function is the appropriate choice for appending literal strings and is more efficient than `appendStringInfo` in this case.
🤖 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.
Nitpick comments:
In `@src/spock_functions.c`:
- Line 2240: Replace the `appendStringInfo(buf, ", ")` call with
`appendStringInfoString(buf, ", ")` to avoid unnecessary format string parsing
when appending a literal string with no format specifiers. The
`appendStringInfoString` function is the appropriate choice for appending
literal strings and is more efficient than `appendStringInfo` in this case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96af3da6-96ee-4c8c-9603-ea16b1db026e
⛔ Files ignored due to path filters (2)
tests/regress/expected/autoddl.outis excluded by!**/*.outtests/regress/expected/exception_row_capture.outis excluded by!**/*.out
📒 Files selected for processing (3)
src/spock_functions.csrc/spock_relcache.ctests/regress/sql/autoddl.sql
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| 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.
The auto-DDL path in spock_auto_replicate_ddl() interpolated the search_path GUC value into the queued command with a bare "%s", guarded only by strlen() > 0. That is unsafe because GetConfigOptionByName() does not always return something that is valid SQL on its own. Fix this issue. Change error message on missed entry in spock cache: highlight the fact that the root of the issue might be schema mismatch.
3 participants
Summary
The auto-DDL path in
spock_auto_replicate_ddl()interpolated the publisher'ssearch_pathGUC straight into the queued command with a bare%s, guarded only bystrlen(search_path) > 0. That is unsafe:GetConfigOptionByName("search_path", ...)does not always return something that is valid SQL on its own. In particularset_config('search_path', ' ', false)— exactly what core'snamespaceregression test does — leaves a bare space in the GUC, which interpolated directly emits the invalidSET search_path TO ;. That statement fails to parse on the subscriber, so the DDL (and any rows depending on it) never lands.spock_replicate_ddl_command()already parsed and re-quoted the path correctly, so the two code paths disagreed. This PR factors the correct logic into a single helper and routes both call sites through it.Changes
append_set_search_path(), which parses thesearch_pathvalue withSplitIdentifierString()and re-emits each schema name throughquote_identifier(). A value that resolves to no schemas (empty or whitespace-only) is shipped asSET search_path TO '';, so the command is always valid SQL and the subscriber ends up with the same effective (empty) path as the publisher.%sinterpolation inspock_auto_replicate_ddl()with a call to the helper.spock_replicate_ddl_command()with the same helper.spock_relation_open()cache-misselogto anereportwitherrdetail/errhintthat point at the likely cause — a relation message arriving after a data change that references it, usually a schema mismatch — so the subscriber-side failure is diagnosable rather than a bare "cache lookup failed".