Minor fixes and improvements for progress tracking #311
Conversation
mason-sharp
commented
Jan 12, 2026
mason-sharp
commented
- 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
- 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
left a comment
danolivo
left a comment
There was a problem hiding this comment.
It looks correct. Minor fixes should be addressed before the merge.
| * because it is reported more frequently (by keepalive messages). | ||
| */ | ||
| Assert(!(dest->remote_commit_ts == 0 ^ dest->last_updated_ts == 0)); | ||
| Assert(!((dest->remote_commit_ts == 0) ^ (dest->last_updated_ts == 0))); |
There was a problem hiding this comment.
Hmm, why do you add extra braces here? Is the C Standard not clear here?
There was a problem hiding this comment.
This makes a compiler warning go away
There was a problem hiding this comment.
I've never seen warnings with such a code before with gcc/clang. What exactly is the problem? The same priority of operations or something else? Maybe the root issue is compiler settings?
| hdr.flags = 0; | ||
|
|
||
| /* Acquire lock before reading hash table to ensure consistency */ | ||
| LWLockAcquire(SpockCtx->apply_group_master_lock, LW_SHARED); |
There was a problem hiding this comment.
Postmaster writes the HTAB to disk at the moment when all other processes already terminated or killed. So, no potential races can happen. It would be better to check that this lock is not acquired instead (to be sure developers still follow the concept), but generally, this change is correct.
| 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", |
There was a problem hiding this comment.
Both variants are incorrect. Conventional way: use the LSN_FORMAT_ARGS macros instead.
| } | ||
|
|
||
| adjust_progress_info(origin_conn); | ||
| progress_entries_list = adjust_progress_info(origin_conn); |
There was a problem hiding this comment.
Good catch! But call:
progress_entries_list = adjust_progress_info(origin_conn);
should be done right before the ensure_replication_slot_snapshot().
