Hi Peter, Thanks for your reviews. I tried to apply most of them. I just have some comments below for some of them.
Peter Smith <smithpb2...@gmail.com>, 14 Haz 2023 Çar, 08:45 tarihinde şunu yazdı: > > 9. process_syncing_tables_for_sync > > @@ -378,7 +387,13 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn) > */ > replorigin_drop_by_name(originname, true, false); > > - finish_sync_worker(); > + /* > + * Sync worker is cleaned at this point. It's ready to sync next table, > + * if needed. > + */ > + SpinLockAcquire(&MyLogicalRepWorker->relmutex); > + MyLogicalRepWorker->ready_to_reuse = true; > + SpinLockRelease(&MyLogicalRepWorker->relmutex); > > 9a. > I did not quite follow the logic. It says "Sync worker is cleaned at > this point", but who is doing that? -- more details are needed. But, > why not just call clean_sync_worker() right here like it use to call > finish_sync_worker? I agree that these explanations at places where the worker decides to not continue with the current table were confusing. Even the name of ready_to_reuse was misleading. I renamed it and tried to improve comments in such places. Can you please check if those make more sense now? > ====== > src/backend/replication/logical/worker.c > > 10. General -- run_tablesync_worker, TablesyncWorkerMain > > IMO these functions would be more appropriately reside in the > tablesync.c instead of the (common) worker.c. Was there some reason > why they cannot be put there? I'm not really against moving those functions to tablesync.c. But what's not clear to me is worker.c. Is it the places to put common functions for all log. rep. workers? Then, what about apply worker? Should we consider a separate file for apply worker too? Thanks, -- Melih Mutlu Microsoft
