Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

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?

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

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.

4.
/* missin_ok = true, since the origin might be already dropped. */
typo: missing_ok

5. GetReplicationSlotNamesBySubId:
errmsg("not tuple returned."));

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));

6.
static void
clean_sync_worker(void)

--can we please add introductory comment for this function.

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.

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.

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?

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.