On Wed, Sep 6, 2023 at 2:18 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Fri, Sep 1, 2023 at 1:59 PM Peter Smith <smithpb2...@gmail.com> wrote: > > > > Hi Shveta. Here are some comments for patch v14-0002 > > > > The patch is large, so my code review is a WIP... more later next week... > > > > Thanks Peter for the feedback. I have tried to address most of these > in v15. Please find my response inline for the ones which I have not > addressed. > > > > > 26. > > +typedef struct SlotSyncWorker > > +{ > > + /* Time at which this worker was launched. */ > > + TimestampTz launch_time; > > + > > + /* Indicates if this slot is used or free. */ > > + bool in_use; > > + > > + /* The slot in worker pool to which it is attached */ > > + int slot; > > + > > + /* Increased every time the slot is taken by new worker. */ > > + uint16 generation; > > + > > + /* Pointer to proc array. NULL if not running. */ > > + PGPROC *proc; > > + > > + /* User to use for connection (will be same as owner of subscription). */ > > + Oid userid; > > + > > + /* Database id to connect to. */ > > + Oid dbid; > > + > > + /* Count of Database ids it manages */ > > + uint32 dbcount; > > + > > + /* DSA for dbids */ > > + dsa_area *dbids_dsa; > > + > > + /* dsa_pointer for database ids it manages */ > > + dsa_pointer dbids_dp; > > + > > + /* Mutex to access dbids in dsa */ > > + slock_t mutex; > > + > > + /* Info about slot being monitored for worker's naptime purpose */ > > + SlotSyncWorkerWatchSlot monitor; > > +} SlotSyncWorker; > > > > There seems an awful lot about this struct which is common with > > 'LogicalRepWorker' struct. > > > > It seems a shame not to make use of the commonality instead of all the > > cut/paste here. > > > > E.g. Can it be rearranged so all these common fields are shared: > > - launch_time > > - in_use > > - slot > > - generation > > - proc > > - userid > > - dbid > > > > Sure, I had this in mind along with previous comments where it was > suggested to merge similar functions like > WaitForReplicationWorkerAttach, WaitForSlotSyncWorkerAttach etc. That > merging could only be possible if we try to merge the common part of > these structures. This is WIP, will be addressed in the next version. >
This has been addressed in version-17 now. thanks Shveta