Fix progress tracking

[mason-sharp]

Since we reverted cbc5bb1, fix another way and add a test.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch task/v5_progress

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/spock_sync.c (1)

446-458: ⚠️ Potential issue | 🟠 Major

Memory leak: PQescapeLiteral allocations are not freed.

PQescapeLiteral allocates memory via malloc that must be freed with PQfreemem(). In this loop, 5 escaped strings are created per iteration but never freed, causing a memory leak proportional to the number of progress rows processed.

🔧 Proposed fix to free escaped literals

 			char   *updated_by_decode = PQgetvalue(originRes, rno, 6);
+			char   *esc_commit_ts, *esc_lsn, *esc_insert_lsn;
+			char   *esc_updated_ts, *esc_updated_by;
 
 			resetStringInfo(&query);
+			esc_commit_ts = PQescapeLiteral(target_conn, remote_commit_ts,
+							strlen(remote_commit_ts));
+			esc_lsn = PQescapeLiteral(target_conn, remote_lsn,
+							strlen(remote_lsn));
+			esc_insert_lsn = PQescapeLiteral(target_conn, remote_insert_lsn,
+							strlen(remote_insert_lsn));
+			esc_updated_ts = PQescapeLiteral(target_conn, last_updated_ts,
+							strlen(last_updated_ts));
+			esc_updated_by = PQescapeLiteral(target_conn, updated_by_decode,
+							strnlen(updated_by_decode, 64));
+
 			appendStringInfo(&query, upsertQuery,
 							 MySubscription->target->id,
 							 remote_node_id,
-							 PQescapeLiteral(target_conn, remote_commit_ts,
-											 strlen(remote_commit_ts)),
-							 PQescapeLiteral(target_conn, remote_lsn,
-											 strlen(remote_lsn)),
-							 PQescapeLiteral(target_conn, remote_insert_lsn,
-											 strlen(remote_insert_lsn)),
-							 PQescapeLiteral(target_conn, last_updated_ts,
-											 strlen(last_updated_ts)),
-							 PQescapeLiteral(target_conn, updated_by_decode,
-											 strnlen(updated_by_decode, 64)));
+							 esc_commit_ts, esc_lsn, esc_insert_lsn,
+							 esc_updated_ts, esc_updated_by);
 			updateRes = PQexec(target_conn, query.data);
+
+			PQfreemem(esc_commit_ts);
+			PQfreemem(esc_lsn);
+			PQfreemem(esc_insert_lsn);
+			PQfreemem(esc_updated_ts);
+			PQfreemem(esc_updated_by);

🧹 Nitpick comments (2)

src/spock_sync.c (1)

457-458: Consider defining the 64 limit as a named constant.

The magic number 64 for the updated_by_decode length limit should ideally match the actual column size constraint in the spock.progress table. Consider extracting this to a named constant for clarity and to ensure consistency if the schema changes.

+#define UPDATED_BY_DECODE_MAXLEN 64
+
 ...
-							 strnlen(updated_by_decode, 64)));
+							 strnlen(updated_by_decode, UPDATED_BY_DECODE_MAXLEN)));

tests/regress/sql/progress_tracking.sql (1)

143-156: Polling loops may cause flaky tests on slow CI environments.

The fixed iteration counts (100 iterations × 0.1s = 10s max) might not be sufficient on heavily loaded CI runners. Consider increasing the iteration count or adding a diagnostic message when the loop exhausts without finding the expected state.

💡 Suggestion to add timeout diagnostic

 DO $$
 BEGIN
     FOR i IN 1..100 LOOP
         IF EXISTS (
             SELECT 1 FROM spock.local_sync_status
             WHERE sync_relname = 'resync_target_tbl'
               AND sync_status IN ('y', 'r')
         ) THEN
             RETURN;
         END IF;
         PERFORM pg_sleep(0.1);
     END LOOP;
+    RAISE WARNING 'Timed out waiting for resync_target_tbl sync status';
 END;
 $$;