On Mon, Feb 19, 2024 at 5:32 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > Few comments on 0001
Thanks for the feedback. > ==================== > 1. I think it is better to error out when the valid GUC or option is > not set in ensure_valid_slotsync_params() and > ensure_valid_remote_info() instead of waiting. And we shouldn't start > the worker in the first place if not all required GUCs are set. This > will additionally simplify the code a bit. Sure, removed 'ensure' functions. Moved the validation checks to the postmaster before starting the slot sync worker. > 2. > +typedef struct SlotSyncWorkerCtxStruct > { > - /* prevents concurrent slot syncs to avoid slot overwrites */ > + pid_t pid; > + bool stopSignaled; > bool syncing; > + time_t last_start_time; > slock_t mutex; > -} SlotSyncCtxStruct; > +} SlotSyncWorkerCtxStruct; > > I think we don't need to change the name of this struct as this can be > used both by the worker and the backend. We can probably add the > comment to indicate that all the fields except 'syncing' are used by > slotsync worker. Modified. > 3. Similar to above, function names like SlotSyncShmemInit() shouldn't > be changed to SlotSyncWorkerShmemInit(). Modified. > 4. > +ReplSlotSyncWorkerMain(int argc, char *argv[]) > { > ... > + on_shmem_exit(slotsync_worker_onexit, (Datum) 0); > ... > + before_shmem_exit(slotsync_failure_callback, PointerGetDatum(wrconn)); > ... > } > > Do we need two separate callbacks? Can't we have just one (say > slotsync_failure_callback) that cleans additional things in case of > slotsync worker? And, if we need both those callbacks then please add > some comments for both and why one is before_shmem_exit() and the > other is on_shmem_exit(). I think we can merge these now. Earlier 'on_shmem_exit' was needed to avoid race-condition between startup and slot sync worker process to drop 'i' slots on promotion. Now we do not have any such scenario. But I need some time to analyze it well. Will do it in the next version. > In addition to the above, I have made a few changes in the comments > and code (cosmetic code changes). Please include those in the next > version if you find them okay. You need to rename .txt file to remove > .txt and apply atop v90-0001*. Sure, included these. Please find the patch001 attached. I will rebase the rest of the patches and post them tomorrow. thanks Shveta
v91-0001-Add-a-new-slotsync-worker.patch
Description: Binary data