On Tue, Feb 4, 2025 at 8:37 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Some review comments for patch v1-0001
>
> ======
> GENERAL - Missing Test case?
>
> 1.
> Should there be some before/after test case for 'active_since' value
> with invalid slots to verify that the patch is doing what it says?
>
I think the existing tests should be sufficient. Adding a new test
just for invalid slots means we need to consider the other properties
of slots as well because in general the invalid slots shouldn't be
updated.
>
> RestoreSlotFromDisk:
>
> 3.
> + if (now == 0)
> + now = GetCurrentTimestamp();
> +
> + ReplicationSlotSetInactiveSince(slot, now, false);
>
> 3a.
> The code from v65-0001 in the other thread [1] (where the bulk of this
> v1 patch came from) used to have a RestoreSlotFromDisk function
> comment saying "Avoid calling ReplicationSlotSetInactiveSince() here,
> as it will not set the time for invalid slots."
>
> In other words, yesterday we were deliberately NOT calling function
> ReplicationSlotSetInactiveSince, but today we deliberately ARE calling
> it (???).
>
> Why has it changed?
>
See my last comment in that thread (1). In short, the invalid slots
should never be update inactive_since and the same should be updated
in docs.
> ~~~
>
> 3b.
> The other code that is similar to this deferred assignment of 'now'
> (see function update_synced_slots_inactive_since) includes a comment
> /* Use the same inactive_since time for all the slots. */.
>
> Should this code do the same?
>
This makes sense.
>
> 4b.
> Since this is a static inline function I assume performance is
> important (e.g. sometimes it is called within a spinlock). If that is
> the case, then maybe writing this logic with fewer conditions would be
> better.
>
> SUGGESTION
>
> if (s->data.invalidated == RS_INVAL_NONE)
> {
> if (acquire_lock)
> {
> SpinLockAcquire(&s->mutex);
> s->inactive_since = ts;
> SpinLockRelease(&s->mutex);
> }
> else
> s->inactive_since = now;
> }
>
I prefer the current coding in the patch as it is simpler to follow.
(1) -
https://www.postgresql.org/message-id/CAA4eK1L6A-RC2PkqAFjgmmmS6_vKxrLsG33vdJzeeRKBP8RbOA%40mail.gmail.com
--
With Regards,
Amit Kapila.
