Minor fixes and improvements for progress tracking by mason-sharp · Pull Request #311 · pgEdge/spock

mason-sharp · 2026-01-12T03:40:39Z

Refactor apply_worker_get_progress() to return only the timestamp needed. Return value instead of pointer to entire struct.
Lock sooner in spock_group_resource_dump() because of getting the number of entries
Properly handle progress updates after COPY operations in replication set data sync and avoid potential memory leak
Address compiler warnings

danolivo

It looks correct. Minor fixes should be addressed before the merge.

src/spock_group.c

danolivo · 2026-01-12T10:56:46Z

src/spock_group.c

 		elog(LOG, "SPOCK: adjust spock.progress %d->%d to "
 			 "remote_commit_ts='%s' "
-			 "remote_commit_lsn=%llX remote_insert_lsn=%llX",
+			 "remote_commit_lsn=%lX remote_insert_lsn=%lX",


Both variants are incorrect. Conventional way: use the LSN_FORMAT_ARGS macros instead.

danolivo · 2026-01-12T11:08:13Z

src/spock_sync.c

 	}

-	adjust_progress_info(origin_conn);
+	progress_entries_list = adjust_progress_info(origin_conn);


Good catch! But call:
progress_entries_list = adjust_progress_info(origin_conn);
should be done right before the ensure_replication_slot_snapshot().

After fixing this, you may launch the following workflow on your branch: https://github.com/pgEdge/spock/actions/runs/21101567363
It fails now in 100% of cases. With this case, it should pass (about 99% or so).

- Refactor apply_worker_get_progress() to return only the timestamp needed. Return value instead of pointer to entire struct. - Lock sooner in spock_group_resource_dump() because of getting the number of entries - Properly handle progress updates after COPY operations in replication set data sync and avoid potential memory leak - Address compiler warnings

danolivo · 2026-02-04T13:57:22Z

src/spock_sync.c

 		origin_conn_repl = spock_connect_replica(sub->origin_if->dsn,
 												 sub->name, "snap");

+		adjust_progress_info(origin_conn);


How is it supposed to work?
I don't see here following spock_group_progress_update_list() ...

coderabbitai · 2026-02-05T02:30:39Z

📝 Walkthrough

Walkthrough

Replaces public API apply_worker_get_progress() (returning SpockApplyProgress *) with apply_worker_get_prev_remote_ts() (returns TimestampTz); updates call sites to use the new accessor, adjusts locking and LSN formatting in group code, defers progress updates around COPY, and removes a local variable in executor code.

Changes

Cohort / File(s)	Summary
Public API & Header `include/spock_group.h`	Changed declaration: `extern SpockApplyProgress *apply_worker_get_progress(void);` → `extern TimestampTz apply_worker_get_prev_remote_ts(void);` (public signature changed).
Group implementation & logging `src/spock_group.c`	Replaced old accessor with `apply_worker_get_prev_remote_ts()` returning a timestamp; adjusted progress handling, parenthesized invariant, moved an early lock in `resource_dump`, and switched LSN log formatting to `LSN_FORMAT_ARGS`.
Apply path usage `src/spock_apply.c`	Replaced `apply_worker_get_progress()->prev_remote_ts` usages with `apply_worker_get_prev_remote_ts()` in comparisons and log messages.
Sync / COPY progress aggregation `src/spock_sync.c`	Deferred immediate `adjust_progress_info()`; introduced `progress_entries_list` to collect entries and call `spock_group_progress_update_list(progress_entries_list)` after the COPY phase.
Executor potential issue `src/spock_executor.c`	Removed local `ObjectAddress object` declaration in the `OAT_DROP` branch of `spock_object_access()`, which may leave `object` undefined for subsequent `ObjectAddressSubSet` use—requires review.

Poem

🐇 I nibble timestamps under starlit code,
I fetch the last remote where markers flowed.
Locks lined up, LSNs in bright array,
Progress bundled after COPY’s long day.
Hop—new name, same trail, off I go!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main refactoring work: improving progress tracking through API simplification and minor fixes.
Description check	✅ Passed	The description is directly related to the changeset, covering the key improvements: API refactoring, lock timing, progress update handling, and compiler warnings.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch progress-track-minor-improvements

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 and usage tips.}

Previously, adjust_progress_info() and spock_group_progress_update_list() were called inside copy_replication_sets_data(), between the COPY transaction finish and function return. This was incorrect because the progress update must happen after the COPY transaction is committed on both origin and target, and the snapshot used for progress info must be consistent with the replication slot snapshot. Move adjust_progress_info() to spock_sync_subscription() right before ensure_replication_slot_snapshot(), so it captures progress using the same connection and transaction context. Move spock_group_progress_update_list() to after copy_replication_sets_data() returns, ensuring replication progress is updated only after all copied data has been committed.

coderabbitai

Actionable comments posted: 0

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)

1163-1254: ⚠️ Potential issue | 🟠 Major

Progress list is captured outside the COPY snapshot.

Line 1179 calls adjust_progress_info(origin_conn) before the replication-slot snapshot is exported, so the values can drift from the snapshot used for COPY. That can lead to over/under‑skipping when replication starts. Consider capturing progress inside the COPY snapshot transaction (e.g., within copy_replication_sets_data after start_copy_origin_tx) and returning the list for the post‑COPY update at Line 1254. Also, when SyncKindData(sync->kind) is false, the list is allocated but never freed; guard the call or free the list in the non‑data path.

mason-sharp requested review from danolivo and rasifr January 12, 2026 03:40

danolivo requested changes Jan 12, 2026

View reviewed changes

mason-sharp force-pushed the progress-track-minor-improvements branch from 679df29 to a78e2cf Compare February 3, 2026 02:18

danolivo requested changes Feb 4, 2026

View reviewed changes

Remove one adjust_progress_info call

7a36d8b

mason-sharp force-pushed the progress-track-minor-improvements branch from ec5beab to 7a36d8b Compare February 5, 2026 03:07

danolivo force-pushed the progress-track-minor-improvements branch from 648f376 to d5d7c1a Compare February 5, 2026 10:13

coderabbitai bot reviewed Feb 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor fixes and improvements for progress tracking#311

Minor fixes and improvements for progress tracking#311
mason-sharp wants to merge 3 commits intomainfrom
progress-track-minor-improvements

mason-sharp commented Jan 12, 2026

Uh oh!

danolivo left a comment

Uh oh!

Uh oh!

Uh oh!

danolivo Jan 12, 2026

Uh oh!

danolivo Jan 12, 2026

Uh oh!

danolivo Jan 19, 2026

Uh oh!

danolivo Feb 4, 2026

Uh oh!

coderabbitai bot commented Feb 5, 2026 •

edited

Loading

Walkthrough

Changes

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mason-sharp commented Jan 12, 2026

Uh oh!

danolivo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danolivo Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

danolivo Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

danolivo Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

danolivo Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Feb 5, 2026 •

edited

Loading