Re: Introduce XID age and inactive timeout based replication slot invalidation

On Tue, Feb 4, 2025 at 4:42 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 4 Feb 2025 at 15:58, Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Here are the v68 patches, incorporating above as well as comments from [1].
> >
> Few comments:
> 1) Let's call TimestampDifferenceExceedsSeconds only if
> idle_replication_slot_timeout_mins is set to avoid the
> TimestampDifferenceExceedsSeconds function call and timestamp diff
> calculation if not required:
> + if (CanInvalidateIdleSlot(s) &&
> + TimestampDifferenceExceedsSeconds(s->inactive_since, now,
> +   idle_replication_slot_timeout_mins * SECS_PER_MINUTE))
> + {
> + invalidation_cause = cause;
> + inactive_since = s->inactive_since;
> + }
> + break;
>

The CanInvalidateIdleSlot(s) call does the check if
idle_replication_slot_timeout_mins is set or not. So we are good here.

> 2) Let's keep the prototype after TimestampDifferenceExceeds to keep
> it consistent with the source file and will also make it easy to
> search:
> diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
> index d26f023fb8..e1d05d6779 100644
> --- a/src/include/utils/timestamp.h
> +++ b/src/include/utils/timestamp.h
> @@ -143,5 +143,8 @@ extern int  date2isoyear(int year, int mon, int mday);
>  extern int     date2isoyearday(int year, int mon, int mday);
>
>  extern bool TimestampTimestampTzRequiresRewrite(void);
> +extern bool TimestampDifferenceExceedsSeconds(TimestampTz start_time,
> +
>                    TimestampTz stop_time,
> +
>                    int threshold_sec);
>

Done.

> 3)How about we change the below:
> +#ifdef USE_INJECTION_POINTS
> +
> +                                               /*
> +                                                * To test idle
> timeout slot invalidation, if the
> +                                                * slot-time-out-inval
> injection point is attached,
> +                                                * set inactive_since
> to a very old timestamp (1
> +                                                * microsecond since
> epoch) to immediately invalidate
> +                                                * the slot.
> +                                                */
> +                                               if
> (IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
> +                                                       s->inactive_since = 1;
> +#endif
> to:
> #ifdef USE_INJECTION_POINTS
> /*
> * To test idle timeout slot invalidation, if the
> * slot-time-out-inval injection point is attached,
> * set inactive_since to current time and invalidate the slot immediately.
> */
> if (IS_INJECTION_POINT_ATTACHED("slot-time-out-inval") &&
> idle_replication_slot_timeout_mins)
> {
> invalidation_cause = cause;
> inactive_since = s->inactive_since = now;
> }
> #else
> /*
> * Check if the slot needs to be invalidated due to
> * idle_replication_slot_timeout GUC.
> */
> if (TimestampDifferenceExceedsSeconds(s->inactive_since, now,
>   idle_replication_slot_timeout_mins * SECS_PER_MINUTE))
> {
> invalidation_cause = cause;
> inactive_since = s->inactive_since;
> }
> #endif
>
> We can just invalidate the slot directly without checking the time
> difference if idle_replication_slot_timeout_mins is set and
> inactive_since can hold the now value.
>

+1 to the idea. Implemented it in a slightly different way to avoid
enclosing the main code within "#else".

Here is v69 patch set addressing above and Kuroda-san's comments in [1].

[1] https://www.postgresql.org/message-id/OSCPR01MB14966A918EBB0674E5423EDE0F5F42%40OSCPR01MB14966.jpnprd01.prod.outlook.com

--
Thanks,
Nisha