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
