Here are my review comments for the v23-0005 patch: ======
Commit Message says: main_worker_pid is Process ID of the main apply worker, if this process is a apply background worker. NULL if this process is a main apply worker or a synchronization worker. The new column can make it easier to distinguish main apply worker and apply background worker. -- Having a column called ‘main_worker_pid’ which is defined to be NULL if the process *is* the main apply worker does not make any sense to me. IMO it feels hacky trying to squeeze meaning out of the 'main_worker_pid' member of the LogicalRepWorker like this. If the intention really is to make it easier to distinguish the different kinds of subscription workers then surely there are much better ways to achieve that. For example, why not introduce a new 'type' enum member of the LogicalRepWorker (e.g. WORKER_TYPE_TABLESYNC='t', WORKER_TYPE_APPLY='a', WORKER_TYPE_PARALLEL_APPLY='p'), then use some char column to expose it? As a bonus, I think the other code (i.e.patch 0001) will also be improved if a 'type' member is added. ------ Kind Regards, Peter Smith. Fujitsu Australia
