On Wed, Aug 23, 2023 at 4:21 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Wed, Aug 23, 2023 at 3:38 PM shveta malik <shveta.ma...@gmail.com> wrote:
> >
> I have reviewed the v12-0002 patch and I have some comments. I see the
> latest version posted sometime back and if any of this comment is
> already fixed in this version then feel free to ignore that.
>
Thanks for the feedback. Please find my comments on a few. I will work on rest.
> 2.
> ApplyLauncherShmemInit(void)
> {
> bool found;
> + bool foundSlotSync;
>
> Is there any specific reason to launch the sync worker from the
> logical launcher instead of making this independent?
> I mean in the future if we plan to sync physical slots as well then it
> wouldn't be an expandable design.
When we started working on this, it was reusing logical-apply worker
infra, so I separated it from logical-apply worker but let it be
managed by a replication launcher considering that only logical slots
needed to be synced. I think this needs more thought and I would like
to know from others as well before concluding anything here.
> 5.
>
> +static bool
> +WaitForSlotSyncWorkerAttach(SlotSyncWorker *worker,
> + uint16 generation,
> + BackgroundWorkerHandle *handle)
>
> this function is an exact duplicate of WaitForReplicationWorkerAttach
> except for the LWlock, why don't we use the same function by passing
> the LWLock as a parameter
>
Here workers (first argument) are different. We can always pass
LWLock, but since workers are different, in order to merge the common
functionality, we need to have some common worker structure between
the two workers (apply and sync-slot) and pass that to functions which
need to be merged (similar to NodeTag used in Insert/CreateStmt etc).
But changing LogicalRepWorker() would mean changing
applyworker/table-sync worker/parallel-apply-worker files. Since there
are only two such functions which you pointed out (attach and
wait_for_attach), I prefered to keep the functions as is until we
conclude on where slot-sync worker functionality actually fits in. I
can revisit these comments then. Or if you see any better way to do
it, kindly let me know.
> 6.
> +/*
> + * Attach Slot-sync worker to worker-slot assigned by launcher.
> + */
> +void
> +slotsync_worker_attach(int slot)
>
> this is also very similar to the logicalrep_worker_attach function.
>
> Please check other similar functions and reuse them wherever possible
>
> Also, why this function is not registering the cleanup function on
> shmmem_exit?
>
It is doing it in ReplSlotSyncMain() since we have dsm-seg there.
Please see this:
/* Primary initialization is complete. Now, attach to our slot. */
slotsync_worker_attach(worker_slot);
before_shmem_exit(slotsync_worker_detach, PointerGetDatum(seg));
thanks
Shveta
