Hi,
On Wed, Apr 03, 2024 at 11:17:41AM +0530, Bharath Rupireddy wrote:
> On Wed, Apr 3, 2024 at 8:38 AM shveta malik <[email protected]> wrote:
> >
> > > > Or a simple solution is that the slotsync worker updates
> > > > inactive_since as it does for non-synced slots, and disables
> > > > timeout-based slot invalidation for synced slots.
> >
> > I like this idea better, it takes care of such a case too when the
> > user is relying on sync-function rather than worker and does not want
> > to get the slots invalidated in between 2 sync function calls.
>
> Please find the attached v31 patches implementing the above idea:
Thanks!
Some comments related to v31-0001:
=== testing the behavior
T1 ===
> - synced slots get their on inactive_since just like any other slot
It behaves as described.
T2 ===
> - synced slots inactive_since is set to current timestamp after the
> standby gets promoted to help inactive_since interpret correctly just
> like any other slot.
It behaves as described.
CR1 ===
+ <structfield>inactive_since</structfield> value will get updated
+ after every synchronization
indicates the last synchronization time? (I think that after every
synchronization
could lead to confusion).
CR2 ===
+ /*
+ * Set the time since the slot has become inactive
after shutting
+ * down slot sync machinery. This helps correctly
interpret the
+ * time if the standby gets promoted without a restart.
+ */
It looks to me that this comment is not at the right place because there is
nothing after the comment that indicates that we shutdown the "slot sync
machinery".
Maybe a better place is before the function definition and mention that this is
currently called when we shutdown the "slot sync machinery"?
CR3 ===
+ * We get the current time beforehand and only once to
avoid
+ * system calls overhead while holding the lock.
s/avoid system calls overhead while holding the lock/avoid system calls while
holding the spinlock/?
CR4 ===
+ * Set the time since the slot has become inactive. We get the current
+ * time beforehand to avoid system call overhead while holding the lock
Same.
CR5 ===
+ # Check that the captured time is sane
+ if (defined $reference_time)
+ {
s/Check that the captured time is sane/Check that the inactive_since is sane/?
Sorry if some of those comments could have been done while I did review
v29-0001.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
