Implement add_node/remove_node in C, eliminating dblink and ZODAN scripts.#545
Implement add_node/remove_node in C, eliminating dblink and ZODAN scripts.#545ibrarahmad wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughZODAN node addition and removal are implemented as C-backed Spock procedures using SPI and libpq. Documentation and TAP setup now invoke the built-in procedures directly without legacy SQL scripts or ChangesBuilt-in ZODAN procedures
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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
| Security | 1 high |
| Complexity | 15 medium |
🟢 Metrics 196 complexity · 2 duplication
Metric Results Complexity 196 Duplication 2
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/spock_zodan.c`:
- Around line 1564-1573: Add PG_ARGISNULL validation for required text arguments
before PG_GETARG_TEXT_PP in both procedure entry points: guard arguments 0–3
before assigning src_node_name, src_dsn, new_node_name, and new_node_dsn, and
guard argument 0 before assigning target_node_name. Handle NULL inputs with the
procedure’s existing error mechanism rather than passing them to
text_to_cstring.
- Around line 598-613: Update the role-existence query construction in the loop
using rolname to quote the value with quote_literal_cstr() before interpolating
it into SQL. Ensure the quoted value is used by zodan_local_count() and freed
appropriately, while preserving the existing missing-role collection behavior.
In `@tests/tap/t/017_zodan_3n_timeout.pl`:
- Around line 13-16: Update every failure-case timing assertion in the 3-node
timeout test, particularly the checks near the 60- and 300-second limits, to
require completion well below timeout_sec := 30 rather than accepting ordinary
timeout failures. Apply the stricter bound consistently to all failure cases,
including those around the add_node scenarios.
🪄 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: 5e1b77b8-223b-4605-af9d-f47c1e313dac
📒 Files selected for processing (15)
README.mddocs/modify/zodan/index.mddocs/modify/zodan/zodan_readme.mddocs/modify/zodan/zodan_tutorial.mdsamples/Z0DAN/zodan.sqlsamples/Z0DAN/zodan_cleanup.sqlsamples/Z0DAN/zodremove.sqlsql/spock--6.0.0.sqlsrc/spock_zodan.ctests/docker/entrypoint.shtests/tap/t/009_zodan_add_remove_nodes.pltests/tap/t/010_zodan_add_remove_python.pltests/tap/t/011_zodan_sync_third.pltests/tap/t/012_zodan_basics.pltests/tap/t/017_zodan_3n_timeout.pl
💤 Files with no reviewable changes (5)
- tests/tap/t/010_zodan_add_remove_python.pl
- samples/Z0DAN/zodan_cleanup.sql
- tests/tap/t/012_zodan_basics.pl
- samples/Z0DAN/zodremove.sql
- tests/tap/t/011_zodan_sync_third.pl
…ipts. Move the Zero Downtime Add/Remove Node orchestration out of the PL/pgSQL scripts under samples/Z0DAN and into a new C module, src/spock_zodan.c. spock.add_node and spock.remove_node now ship with the extension and reach the other cluster nodes over libpq (spock_connect + PQexec) instead of the external dblink extension. A single CREATE EXTENSION spock provides both procedures; there are no scripts to load and no dblink dependency. The procedures are LANGUAGE c PROCEDUREs that run non-atomically and use SPI_commit between phases where the old scripts committed, so apply and sync workers observe newly created or enabled subscriptions. Local work reuses the existing spock.node_create/sub_create/sub_enable/sub_drop/ repset_drop/node_drop/sync_event functions; remote work runs the same functions on the target node over libpq. add_node runs on the new node and remove_node runs on the node being removed, matching the previous model. A new timeout_sec argument bounds every wait loop. remove_node drops each surviving node's inbound subscription inside its own internal subtransaction, so an unreachable node is skipped with a warning rather than aborting the removal. add_node supports lolor large-object data: the new node may have the lolor extension installed as long as its tables are empty, and it is required when the source node replicates lolor tables. lolor is excluded from the "no user tables" prerequisite, and the initial data sync copies the lolor tables (the COPY step filters only on the subscription's own skip_schema, which add_node leaves empty). Both entry points guard their required text arguments with PG_ARGISNULL (LANGUAGE c procedures cannot be STRICT, so a SQL NULL would otherwise detoast a NULL pointer and crash the backend), and every interpolated identifier is passed through quote_literal_cstr(). Delete samples/Z0DAN/zodan.sql, zodremove.sql, zodan_cleanup.sql and the dead python TAP test, and rewire tests 009/011/012/017 and the docker entrypoint to drop the script loads and CREATE EXTENSION dblink. Test 009 tears the cluster down before done_testing() so the destroy_cluster pass() is counted within the plan, and the 017 quick-failure timing bounds sit below timeout_sec so a call that loops to the full timeout no longer passes. Add 033_zodan_lolor_add_node.pl covering the lolor add_node cases against the C-native procedures. Update the ZODAN user documentation to describe the built-in procedures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
830afcf to
2ebeb73
Compare
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)
tests/tap/t/033_zodan_lolor_add_node.pl (1)
66-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEscape connection strings before embedding them in SQL.
dsn()builds raw libpq keyword/value strings, then$add_node_sqlinterpolates them into SQL single quotes. Whitespace, backslashes, or apostrophes in$HOST,$DB,$USER, or$PASScan make the DSN unparseable or break theCALLliteral. Apply libpq value quoting and SQL-literal escaping before interpolation.🤖 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 `@tests/tap/t/033_zodan_lolor_add_node.pl` around lines 66 - 70, Update dsn() to libpq-quote the HOST, DB, USER, and PASS values, then SQL-literal-escape the complete DSN before $add_node_sql interpolates it. Preserve the existing node port selection and DSN structure, ensuring whitespace, backslashes, and apostrophes remain parseable and cannot break the CALL literal.
🤖 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.
Outside diff comments:
In `@tests/tap/t/033_zodan_lolor_add_node.pl`:
- Around line 66-70: Update dsn() to libpq-quote the HOST, DB, USER, and PASS
values, then SQL-literal-escape the complete DSN before $add_node_sql
interpolates it. Preserve the existing node port selection and DSN structure,
ensuring whitespace, backslashes, and apostrophes remain parseable and cannot
break the CALL literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6759367e-60dd-447c-a008-c1e4a20391d3
📒 Files selected for processing (4)
src/spock_zodan.ctests/tap/t/009_zodan_add_remove_nodes.pltests/tap/t/017_zodan_3n_timeout.pltests/tap/t/033_zodan_lolor_add_node.pl
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/tap/t/017_zodan_3n_timeout.pl
- tests/tap/t/009_zodan_add_remove_nodes.pl
- src/spock_zodan.c
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/spock_zodan.c (1)
106-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ZVERB()ignores itsverbargument and is identical toZNOTE().The dead parameter is what forces the
(void) verb;at line 1578 and the unusedverbplumbing throughzodan_remove_subscriptions(). Collapsing to a single macro removes the confusion.♻️ Proposed cleanup
`#define` ZNOTE(...) \ do { if (zodan_verbose) ereport(NOTICE, (errmsg(__VA_ARGS__))); } while (0) -#define ZVERB(verb, ...) \ - do { if (zodan_verbose) ereport(NOTICE, (errmsg(__VA_ARGS__))); } while (0)Then replace
ZVERB(x, ...)call sites withZNOTE(...).🤖 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_zodan.c` around lines 106 - 109, Remove the redundant ZVERB macro and use ZNOTE at all existing ZVERB call sites. Then remove the unused verb parameter and related (void) verb workaround from zodan_remove_subscriptions and any associated call plumbing.
🤖 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 `@src/spock_zodan.c`:
- Around line 714-715: Update the query construction in the node-counting flow
around zodan_local_count to escape ctx->new_node_name with quote_literal_cstr()
instead of interpolating it directly inside SQL quotes. Preserve the existing
node_name filter and count behavior while ensuring names containing quotes
produce valid, non-injectable SQL.
- Around line 1227-1232: Update the timeout branch in the add_node subscription
readiness wait loop to fail the operation instead of logging a notice and
breaking. Match the error/return behavior used by
zodan_local_wait_for_sync_event(), while preserving the existing success path
when the subscription becomes READY.
- Around line 235-276: Update zodan_remote_query and zodan_remote_command to
close the owned PGconn with PQfinish(conn) before ereport(ERROR) on remote
failure. Treat these helpers as consuming the connection on failure, and verify
their callers do not reuse or finish the handle after an error; preserve the
existing result cleanup and error details.
---
Nitpick comments:
In `@src/spock_zodan.c`:
- Around line 106-109: Remove the redundant ZVERB macro and use ZNOTE at all
existing ZVERB call sites. Then remove the unused verb parameter and related
(void) verb workaround from zodan_remove_subscriptions and any associated call
plumbing.
🪄 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: 064017d6-3365-44e5-8cdc-f6e2cbb10b83
📒 Files selected for processing (16)
README.mddocs/modify/zodan/index.mddocs/modify/zodan/zodan_readme.mddocs/modify/zodan/zodan_tutorial.mdsamples/Z0DAN/zodan.sqlsamples/Z0DAN/zodan_cleanup.sqlsamples/Z0DAN/zodremove.sqlsql/spock--6.0.0.sqlsrc/spock_zodan.ctests/docker/entrypoint.shtests/tap/t/009_zodan_add_remove_nodes.pltests/tap/t/010_zodan_add_remove_python.pltests/tap/t/011_zodan_sync_third.pltests/tap/t/012_zodan_basics.pltests/tap/t/017_zodan_3n_timeout.pltests/tap/t/033_zodan_lolor_add_node.pl
💤 Files with no reviewable changes (5)
- tests/tap/t/010_zodan_add_remove_python.pl
- tests/tap/t/012_zodan_basics.pl
- samples/Z0DAN/zodremove.sql
- samples/Z0DAN/zodan_cleanup.sql
- tests/tap/t/011_zodan_sync_third.pl
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- tests/tap/t/033_zodan_lolor_add_node.pl
- tests/tap/t/017_zodan_3n_timeout.pl
- sql/spock--6.0.0.sql
- tests/tap/t/009_zodan_add_remove_nodes.pl
| static PGresult * | ||
| zodan_remote_query(PGconn *conn, const char *sql) | ||
| { | ||
| PGresult *res = PQexec(conn, sql); | ||
|
|
||
| if (PQresultStatus(res) != PGRES_TUPLES_OK) | ||
| { | ||
| char msg[1024]; | ||
|
|
||
| snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn)); | ||
| PQclear(res); | ||
| ereport(ERROR, | ||
| (errmsg("remote query failed on node"), | ||
| errdetail("query: %s", sql), | ||
| errdetail("error: %s", msg))); | ||
| } | ||
| return res; | ||
| } | ||
|
|
||
| /* | ||
| * Run a command on a remote node (COMMAND_OK or TUPLES_OK both accepted). | ||
| * ERROR on failure. | ||
| */ | ||
| static void | ||
| zodan_remote_command(PGconn *conn, const char *sql) | ||
| { | ||
| PGresult *res = PQexec(conn, sql); | ||
| ExecStatusType st = PQresultStatus(res); | ||
|
|
||
| if (st != PGRES_COMMAND_OK && st != PGRES_TUPLES_OK) | ||
| { | ||
| char msg[1024]; | ||
|
|
||
| snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn)); | ||
| PQclear(res); | ||
| ereport(ERROR, | ||
| (errmsg("remote command failed on node"), | ||
| errdetail("command: %s", sql), | ||
| errdetail("error: %s", msg))); | ||
| } | ||
| PQclear(res); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remote failures leak the caller's PGconn.
Both helpers ereport(ERROR) while the caller still owns an open connection, and no call site (except zodan_remove_subscriptions) has a PG_CATCH/resource-owner hook to close it — e.g. zodan_fetch_cluster_nodes (line 379), zodan_check_versions (lines 467/478/501), zodan_verify_prerequisites (lines 544/643/690), zodan_create_sub (line 999), zodan_wait_ready_and_advance (lines 1274/1299). Every failed add_node leaks a socket for the remaining life of the backend and strands the remote backend until it sees EOF.
Cheapest fix: have these helpers take ownership and PQfinish(conn) before raising, or register the connection with the current resource owner so abort cleanup closes it.
🔒️ Sketch: close before raising
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
char msg[1024];
snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn));
PQclear(res);
+ PQfinish(conn);
ereport(ERROR,
(errmsg("remote query failed on node"),
errdetail("query: %s", sql),
errdetail("error: %s", msg)));
}(applies symmetrically in zodan_remote_command; callers must then not reuse or re-finish the handle)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static PGresult * | |
| zodan_remote_query(PGconn *conn, const char *sql) | |
| { | |
| PGresult *res = PQexec(conn, sql); | |
| if (PQresultStatus(res) != PGRES_TUPLES_OK) | |
| { | |
| char msg[1024]; | |
| snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn)); | |
| PQclear(res); | |
| ereport(ERROR, | |
| (errmsg("remote query failed on node"), | |
| errdetail("query: %s", sql), | |
| errdetail("error: %s", msg))); | |
| } | |
| return res; | |
| } | |
| /* | |
| * Run a command on a remote node (COMMAND_OK or TUPLES_OK both accepted). | |
| * ERROR on failure. | |
| */ | |
| static void | |
| zodan_remote_command(PGconn *conn, const char *sql) | |
| { | |
| PGresult *res = PQexec(conn, sql); | |
| ExecStatusType st = PQresultStatus(res); | |
| if (st != PGRES_COMMAND_OK && st != PGRES_TUPLES_OK) | |
| { | |
| char msg[1024]; | |
| snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn)); | |
| PQclear(res); | |
| ereport(ERROR, | |
| (errmsg("remote command failed on node"), | |
| errdetail("command: %s", sql), | |
| errdetail("error: %s", msg))); | |
| } | |
| PQclear(res); | |
| } | |
| static PGresult * | |
| zodan_remote_query(PGconn *conn, const char *sql) | |
| { | |
| PGresult *res = PQexec(conn, sql); | |
| if (PQresultStatus(res) != PGRES_TUPLES_OK) | |
| { | |
| char msg[1024]; | |
| snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn)); | |
| PQclear(res); | |
| PQfinish(conn); | |
| ereport(ERROR, | |
| (errmsg("remote query failed on node"), | |
| errdetail("query: %s", sql), | |
| errdetail("error: %s", msg))); | |
| } | |
| return res; | |
| } | |
| /* | |
| * Run a command on a remote node (COMMAND_OK or TUPLES_OK both accepted). | |
| * ERROR on failure. | |
| */ | |
| static void | |
| zodan_remote_command(PGconn *conn, const char *sql) | |
| { | |
| PGresult *res = PQexec(conn, sql); | |
| ExecStatusType st = PQresultStatus(res); | |
| if (st != PGRES_COMMAND_OK && st != PGRES_TUPLES_OK) | |
| { | |
| char msg[1024]; | |
| snprintf(msg, sizeof(msg), "%s", PQerrorMessage(conn)); | |
| PQclear(res); | |
| PQfinish(conn); | |
| ereport(ERROR, | |
| (errmsg("remote command failed on node"), | |
| errdetail("command: %s", sql), | |
| errdetail("error: %s", msg))); | |
| } | |
| PQclear(res); | |
| } |
🤖 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_zodan.c` around lines 235 - 276, Update zodan_remote_query and
zodan_remote_command to close the owned PGconn with PQfinish(conn) before
ereport(ERROR) on remote failure. Treat these helpers as consuming the
connection on failure, and verify their callers do not reuse or finish the
handle after an error; preserve the existing result cleanup and error details.
| cnt = zodan_local_count(psprintf( | ||
| "SELECT count(*) FROM spock.node WHERE node_name = '%s'", ctx->new_node_name)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unquoted new_node_name interpolation — the one remaining raw '%s' in the file.
Every other query here uses quote_literal_cstr(). A node name containing a single quote produces malformed SQL and allows literal injection into the local query.
🔒️ Proposed fix
cnt = zodan_local_count(psprintf(
- "SELECT count(*) FROM spock.node WHERE node_name = '%s'", ctx->new_node_name));
+ "SELECT count(*) FROM spock.node WHERE node_name = %s",
+ quote_literal_cstr(ctx->new_node_name)));🤖 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_zodan.c` around lines 714 - 715, Update the query construction in
the node-counting flow around zodan_local_count to escape ctx->new_node_name
with quote_literal_cstr() instead of interpolating it directly inside SQL
quotes. Preserve the existing node_name filter and count behavior while ensuring
names containing quotes produce valid, non-injectable SQL.
| if (TimestampDifferenceExceeds(start, GetCurrentTimestamp(), | ||
| ctx->timeout_sec * 1000)) | ||
| { | ||
| ZNOTE(" - timed out waiting for %s to become READY; continuing", sub_name); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Timeout here only warns, so add_node continues with an unsynced subscription.
Every other wait loop errors out on timeout_sec; this one logs a NOTICE and falls through into slot/origin advancement (Phase 7) and sub enabling (Phase 8) while the initial COPY may still be in progress. That produces a node that looks joined but is missing data, instead of the documented "fails quickly" behavior. Prefer erroring, matching zodan_local_wait_for_sync_event().
🐛 Proposed fix
if (TimestampDifferenceExceeds(start, GetCurrentTimestamp(),
ctx->timeout_sec * 1000))
- {
- ZNOTE(" - timed out waiting for %s to become READY; continuing", sub_name);
- break;
- }
+ ereport(ERROR,
+ (errmsg("timed out waiting for subscription %s to complete initial synchronization",
+ sub_name)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (TimestampDifferenceExceeds(start, GetCurrentTimestamp(), | |
| ctx->timeout_sec * 1000)) | |
| { | |
| ZNOTE(" - timed out waiting for %s to become READY; continuing", sub_name); | |
| break; | |
| } | |
| if (TimestampDifferenceExceeds(start, GetCurrentTimestamp(), | |
| ctx->timeout_sec * 1000)) | |
| ereport(ERROR, | |
| (errmsg("timed out waiting for subscription %s to complete initial synchronization", | |
| sub_name))); |
🤖 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_zodan.c` around lines 1227 - 1232, Update the timeout branch in the
add_node subscription readiness wait loop to fail the operation instead of
logging a notice and breaking. Match the error/return behavior used by
zodan_local_wait_for_sync_event(), while preserving the existing success path
when the subscription becomes READY.
Move the Zero Downtime Add/Remove Node orchestration out of the PL/pgSQL scripts under samples/Z0DAN and into a new C module, src/spock_zodan.c. spock.add_node and spock.remove_node now ship with the extension and reach the other cluster nodes over libpq (spock_connect + PQexec) instead of the external dblink extension. A single CREATE EXTENSION spock provides both procedures; there are no scripts to load and no dblink dependency.
The procedures are LANGUAGE c PROCEDUREs that run non-atomically and use SPI_commit between phases where the old scripts committed, so apply and sync workers observe newly created or enabled subscriptions. Local work reuses the existing spock.node_create/sub_create/sub_enable/sub_drop/ repset_drop/node_drop/sync_event functions; remote work runs the same functions on the target node over libpq. add_node runs on the new node and remove_node runs on the node being removed, matching the previous model. A new timeout_sec argument bounds every wait loop.
remove_node drops each surviving node's inbound subscription inside its own internal subtransaction, so an unreachable node is skipped with a warning rather than aborting the removal.
Delete samples/Z0DAN/zodan.sql, zodremove.sql, zodan_cleanup.sql and the dead python TAP test, and rewire tests 009/011/012/017 and the docker entrypoint to drop the script loads and CREATE EXTENSION dblink. Update the ZODAN user documentation to describe the built-in procedures.