On Fri, Feb 9, 2024 at 4:08 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, February 9, 2024 2:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Feb 8, 2024 at 8:01 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Thu, Feb 8, 2024 at 12:08 PM Peter Smith <smithpb2250@gmail.com>
> > wrote:
> > > >
> > > > Here are some review comments for patch v80_2-0001.
> > >
> > > Thanks for the feedback Peter. Addressed the comments in v81.
> > > Attached patch001 for early feedback. Rest of the patches need
> > > rebasing and thus will post those later.
> > >
> > > It also addresses comments by Amit in [1].
> >
> > Thank you for updating the patch! Here are random comments:
>
> Thanks for the comments!
>
> >
> > ---
> > +
> > + /*
> > + * Register the callback function to clean up the shared memory of
> > slot
> > + * synchronization.
> > + */
> > + SlotSyncInitialize();
> >
> > I think it would have a wider impact than expected. IIUC this callback is needed
> > only for processes who calls synchronize_slots(). Why do we want all processes
> > to register this callback?
>
> I think the current style is similar to the ReplicationSlotInitialize() above it. For backend,
> both of them can only be used when user calls slot SQL functions. So, I think it could be fine to
> register it at the general place which can also avoid registering the same again for the later
> slotsync worker patch.
Yes, but it seems to be a legitimate case since replication slot code
involves many functions that need the callback to clear the flag. On
the other hand, in the slotsync code, only one function,
SyncReplicationSlots(), needs the callback at least in 0001 patch.
> Another alternative is to register the callback when calling slotsync functions
> and unregister it after the function call. And register the callback in
> slotsyncworkmain() for the slotsync worker patch, although this may adds a few
> more codes.
Another idea is that SyncReplicationSlots() calls synchronize_slots()
in PG_ENSURE_ERROR_CLEANUP() block instead of PG_TRY(), to make sure
to clear the flag in case of ERROR or FATAL. And the slotsync worker
uses the before_shmem_callback to clear the flag.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com