Skip to content

Implement add_node/remove_node in C, eliminating dblink and ZODAN scripts.#545

Open
ibrarahmad wants to merge 1 commit into
mainfrom
zodan-in-core-node
Open

Implement add_node/remove_node in C, eliminating dblink and ZODAN scripts.#545
ibrarahmad wants to merge 1 commit into
mainfrom
zodan-in-core-node

Conversation

@ibrarahmad

Copy link
Copy Markdown
Contributor

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.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ZODAN 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 dblink.

Changes

Built-in ZODAN procedures

Layer / File(s) Summary
Procedure API and orchestration foundation
sql/spock--6.0.0.sql, src/spock_zodan.c
Adds C-backed spock.add_node and spock.remove_node procedures with shared SPI, libpq, metadata, and version-validation helpers.
Add-node lifecycle
src/spock_zodan.c
Implements prerequisite validation, node registration, synchronization, slot handling, subscription creation, readiness checks, and bounded waits.
Remove-node lifecycle
src/spock_zodan.c
Removes subscriptions, replication sets, and node state, while warning and continuing when surviving nodes are unreachable.
Documentation and test integration
README.md, docs/modify/zodan/*, tests/docker/entrypoint.sh, tests/tap/t/*
Updates usage guidance and tests to use the Spock extension without legacy ZODAN scripts or dblink.

Poem

I’m a rabbit hopping node to node,
With sync LSNs safely sewn.
No dblink scripts clog the way,
Spock calls guide the join today.
Remove with warnings, clean and bright—
ZODAN burrows through the night.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: moving add/remove node orchestration to C and removing dblink/ZODAN scripts.
Description check ✅ Passed The description is detailed and clearly aligned with the documented code, test, and documentation changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch zodan-in-core-node

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ibrarahmad
ibrarahmad requested a review from mason-sharp July 24, 2026 13:42
@codacy-production

codacy-production Bot commented Jul 24, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 2 high · 15 medium

Alerts:

⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
17 new issues

Category Results
ErrorProne 1 high
Security 1 high
Complexity 15 medium

View in Codacy

🟢 Metrics 196 complexity · 2 duplication

Metric Results
Complexity 196
Duplication 2

View in Codacy

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between db380dd and 91b36c8.

📒 Files selected for processing (15)
  • README.md
  • docs/modify/zodan/index.md
  • docs/modify/zodan/zodan_readme.md
  • docs/modify/zodan/zodan_tutorial.md
  • samples/Z0DAN/zodan.sql
  • samples/Z0DAN/zodan_cleanup.sql
  • samples/Z0DAN/zodremove.sql
  • sql/spock--6.0.0.sql
  • src/spock_zodan.c
  • tests/docker/entrypoint.sh
  • tests/tap/t/009_zodan_add_remove_nodes.pl
  • tests/tap/t/010_zodan_add_remove_python.pl
  • tests/tap/t/011_zodan_sync_third.pl
  • tests/tap/t/012_zodan_basics.pl
  • tests/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

Comment thread src/spock_zodan.c
Comment thread src/spock_zodan.c
Comment thread tests/tap/t/017_zodan_3n_timeout.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>
@ibrarahmad
ibrarahmad force-pushed the zodan-in-core-node branch from 830afcf to 2ebeb73 Compare July 25, 2026 07:23

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Escape connection strings before embedding them in SQL.

dsn() builds raw libpq keyword/value strings, then $add_node_sql interpolates them into SQL single quotes. Whitespace, backslashes, or apostrophes in $HOST, $DB, $USER, or $PASS can make the DSN unparseable or break the CALL literal. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 91b36c8 and 830afcf.

📒 Files selected for processing (4)
  • src/spock_zodan.c
  • tests/tap/t/009_zodan_add_remove_nodes.pl
  • tests/tap/t/017_zodan_3n_timeout.pl
  • tests/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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/spock_zodan.c (1)

106-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

ZVERB() ignores its verb argument and is identical to ZNOTE().

The dead parameter is what forces the (void) verb; at line 1578 and the unused verb plumbing through zodan_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 with ZNOTE(...).

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 830afcf and 2ebeb73.

📒 Files selected for processing (16)
  • README.md
  • docs/modify/zodan/index.md
  • docs/modify/zodan/zodan_readme.md
  • docs/modify/zodan/zodan_tutorial.md
  • samples/Z0DAN/zodan.sql
  • samples/Z0DAN/zodan_cleanup.sql
  • samples/Z0DAN/zodremove.sql
  • sql/spock--6.0.0.sql
  • src/spock_zodan.c
  • tests/docker/entrypoint.sh
  • tests/tap/t/009_zodan_add_remove_nodes.pl
  • tests/tap/t/010_zodan_add_remove_python.pl
  • tests/tap/t/011_zodan_sync_third.pl
  • tests/tap/t/012_zodan_basics.pl
  • tests/tap/t/017_zodan_3n_timeout.pl
  • tests/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

Comment thread src/spock_zodan.c
Comment on lines +235 to +276
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);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

Comment thread src/spock_zodan.c
Comment on lines +714 to +715
cnt = zodan_local_count(psprintf(
"SELECT count(*) FROM spock.node WHERE node_name = '%s'", ctx->new_node_name));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

Comment thread src/spock_zodan.c
Comment on lines +1227 to +1232
if (TimestampDifferenceExceeds(start, GetCurrentTimestamp(),
ctx->timeout_sec * 1000))
{
ZNOTE(" - timed out waiting for %s to become READY; continuing", sub_name);
break;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant