public inbox for [email protected]  

help / color / mirror / Atom feed

From: Zhijie Hou (Fujitsu) <[email protected]>
To: Nisha Moond <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: RE: Avoid updating inactive_since for invalid replication slots
Date: Tue, 4 Feb 2025 04:03:25 +0000
Message-ID: <OS0PR01MB57165D33B186D168E867147B94F42@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CABdArM7QdifQ_MHmMA=Cc4v8+MeckkwKncm2Nn6tX9wSCQ-+iw@mail.gmail.com>
References: <CABdArM7QdifQ_MHmMA=Cc4v8+MeckkwKncm2Nn6tX9wSCQ-+iw@mail.gmail.com>

On Monday, February 3, 2025 8:03 PM Nisha Moond <[email protected]> wrote:
> 
> Hi Hackers,
> (CC people involved in the earlier discussion)
> 
> Right now, it is possible for the 'inactive_since' value of an invalid replication
> slot to be updated multiple times, which is unexpected behavior.
> As suggested in the ongoing thread [1], this patch introduces a new dedicated
> function to update the inactive_since for a given replication slot, and ensures
> that inactive_since is not updated for an invalid replication slot.
> 
> 
> The v1 patch is attached. Any feedback would be appreciated.

Thanks for sharing the patch, and I agree we should avoid updating
inactive_since for invalid slots.

But I have one question for the code:

+/*
+ * Set slot's inactive_since property unless it was previously invalidated.
+ */
+static inline void
+ReplicationSlotSetInactiveSince(ReplicationSlot *s, TimestampTz now,
+								bool acquire_lock)
+{
+	if (s->data.invalidated != RS_INVAL_NONE)
+		return;

I am wondering is it safe to access the 'invalidated' flag without taking spinlock ?
Strictly speaking, I think it's only safe to access slot property without lock
when the slot is acquiring by the current process. So, if it's safe here,
could you please add some comments to clarify the same ?

Best Regards,
Hou zj

view thread (7+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: RE: Avoid updating inactive_since for invalid replication slots
  In-Reply-To: <OS0PR01MB57165D33B186D168E867147B94F42@OS0PR01MB5716.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox