+ /* + * apply_dispatch() may have gone into apply_handle_commit() + * which can call process_syncing_tables_for_sync. + * + * process_syncing_tables_for_sync decides whether the sync of + * the current table is completed. If it is completed, + * streaming must be already ended. So, we can break the loop. + */ + if (MyLogicalRepWorker->is_sync_completed) + { + endofstream = true; + break; + } +
and
+ /* + * If is_sync_completed is true, this means that the tablesync + * worker is done with synchronization. Streaming has already been + * ended by process_syncing_tables_for_sync. We should move to the + * next table if needed, or exit. + */ + if (MyLogicalRepWorker->is_sync_completed) + endofstream = true;
~
Instead of those code fragments above assigning 'endofstream' as a side-effect, would it be the same (but tidier) to just modify the other "breaking" condition below:
BEFORE: /* Check if we need to exit the streaming loop. */ if (endofstream) break;
AFTER: /* Check if we need to exit the streaming loop. */ if (endofstream || MyLogicalRepWorker->is_sync_completed) break;
First place you mentioned also breaks the infinite loop. Such an if statement is needed there with or without endofstream assignment.
I think if there is a flag to break a loop, using that flag to indicate that we should exit the loop seems more appropriate to me. I see that it would be a bit tidier without endofstream = true lines, but I feel like it would also be less readable.
I don't have a strong opinion though. I'm just keeping them as they are for now, but I can change them if you disagree.
10b. All the other tablesync-related fields of this struct are named as relXXX, so I wonder if is better for this to follow the same pattern. e.g. 'relsync_completed'
Aren't those start with rel because they're related to the relation that the tablesync worker is syncing? is_sync_completed is not a relation specific field. I'm okay with changing the name but feel like relsync_completed would be misleading.
