On Wed, Sep 11, 2024 at 8:32 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> On Tuesday, September 10, 2024 7:25 PM Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> >
> > One minor comment on 0003
> > =======================
> > 1.
> > get_slot_confirmed_flush()
> > {
> > ...
> > + /*
> > + * To prevent concurrent slot dropping and creation while filtering the
> > + * slots, take the ReplicationSlotControlLock outside of the loop.
> > + */
> > + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> > +
> > + foreach_ptr(String, name, MySubscription->feedback_slots) { XLogRecPtr
> > + confirmed_flush; ReplicationSlot *slot;
> > +
> > + slot = ValidateAndGetFeedbackSlot(strVal(name));
> >
> > Why do we need to validate slots each time here? Isn't it better to do it
> > once?
>
> I think it's possible that the slot was correct but changed or dropped later,
> so it could be useful to give a warning in this case to hint user to adjust
> the
> slots, otherwise, the xmin of the publisher's slot won't be advanced and might
> cause dead tuples accumulation. This is similar to the checks we performed for
> the slots in "synchronized_standby_slots". (E.g. StandbySlotsHaveCaughtup)
>
In the case of "synchronized_standby_slots", we seem to be invoking
such checks via StandbySlotsHaveCaughtup() when we need to wait for
WAL and also we have some optimizations that avoid the frequent
checking for validation checks. OTOH, this patch doesn't have any such
optimizations. We can optimize it by maintaining a local copy of
feedback slots to avoid looping all the slots each time (if this is
required, we can make it a top-up patch so that it can be reviewed
separately). I have also thought of maintaining the updated value of
confirmed_flush_lsn for feedback slots corresponding to a subscription
in shared memory but that seems tricky because then we have to
maintain slot->subscription mapping. Can you think of any other ways?
Having said that it is better to profile this in various scenarios
like by increasing the frequency of keep_alieve message and or in idle
subscriber cases where we try to send this new feedback message.
--
With Regards,
Amit Kapila.
