Hi, On Sun, Mar 31, 2024 at 10:25:46AM +0530, Bharath Rupireddy wrote: > On Thu, Mar 28, 2024 at 3:13 PM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > I think in this case it should always reflect the value from the primary (so > > that one can understand why it is invalidated). > > I'll come back to this as soon as we all agree on inactive_since > behavior for synced slots.
Makes sense. Also if the majority of us thinks it's not needed for inactive_since to be an exact copy of the primary, then let's go that way. > > I think when it is invalidated it should always reflect the value from the > > primary (so that one can understand why it is invalidated). > > I'll come back to this as soon as we all agree on inactive_since > behavior for synced slots. Yeah. > > T4 === > > > > Also, it looks like querying pg_replication_slots() does not trigger an > > invalidation: I think it should if the slot is not invalidated yet (and > > matches > > the invalidation criteria). > > There's a different opinion on this, check comment #3 from > https://www.postgresql.org/message-id/CAA4eK1LLj%2BeaMN-K8oeOjfG%2BUuzTY%3DL5PXbcMJURZbFm%2B_aJSA%40mail.gmail.com. Oh right, I can see Amit's point too. Let's put pg_replication_slots() out of the game then. > > CR6 === > > > > +static bool > > +InvalidateSlotForInactiveTimeout(ReplicationSlot *slot, bool need_locks) > > +{ > > > > InvalidatePossiblyInactiveSlot() maybe? > > I think we will lose the essence i.e. timeout from the suggested > function name, otherwise just the inactive doesn't give a clearer > meaning. I kept it that way unless anyone suggests otherwise. Right. OTOH I think that "Possibly" adds some nuance (like InvalidatePossiblyObsoleteSlot() is already doing). > Please see the attached v30 patch. 0002 is where all of the above > review comments have been addressed. Thanks! FYI, I did not look at the content yet, just replied to the above comments. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
