On Thu, Jan 26, 2023 at 7:53 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote: 1. --Can we please add a few more points to the Summary to make it more clear. a) something telling that reusability of workers is for tables under one subscription and not across multiple subscriptions. b) Since we are reusing both workers and slots, can we add: --when do we actually end up spawning a new worker --when do we actually end up creating a new slot in a worker rather than using existing one. --if we reuse existing slots, what happens to the snapshot?
I modified the commit message if that's what you mean by the Summary.
2. + The last used ID for tablesync workers. This ID is used to + create replication slots. The last used ID needs to be stored + to make logical replication can safely proceed after any interruption. + If sublastusedid is 0, then no table has been synced yet.
--typo: to make logical replication can safely proceed ==> to make logical replication safely proceed
Done
3. is_first_run; move_to_next_rel; --Do you think one variable is enough here as we do not get any extra info by using 2 variables? Can we have 1 which is more generic like 'ready_to_reuse'. Otherwise, please let me know if we must use 2.
Right. Removed is_first_run and renamed move_to_next_rel as ready_to_reuse.
4. /* missin_ok = true, since the origin might be already dropped. */ typo: missing_ok
Can we have a better error msg: ereport(ERROR, errmsg("could not receive list of slots associated with subscription %d, error: %s", subid, res->err));
Done.
6. static void clean_sync_worker(void)
--can we please add introductory comment for this function.
Done.
7. /* * Pick the table for the next run if there is not another worker * already picked that table. */ Pick the table for the next run if it is not already picked up by another worker.
Done.
8. process_syncing_tables_for_sync()
/* Cleanup before next run or ending the worker. */ --can we please improve this comment: if there is no more work left for this worker, stop the worker gracefully, else do clean-up and make it ready for the next relation/run.
Done
9. LogicalRepSyncTableStart: * Read previous slot name from the catalog, if exists. */ prev_slotname = (char *) palloc0(NAMEDATALEN); Do we need to free this at the end?
Pfree'd prev_slotname after we're done with it.
10. if (strlen(prev_slotname) == 0) { elog(ERROR, "Replication slot could not be found for relation %u", MyLogicalRepWorker->relid); } shall we mention subid also in error msg.