Re: Spurious pgstat_drop_replslot() call

Hi,

On Fri, Mar 08, 2024 at 07:55:39PM +0900, Michael Paquier wrote:
> On Fri, Mar 08, 2024 at 10:19:11AM +0000, Bertrand Drouvot wrote:
> > Indeed, it does not seem appropriate to remove stats during slot invalidation as
> > one could still be interested to look at them.
> 
> Yeah, my take is that this can still be interesting to know, at least
> for debugging.  This would limit the stats to be dropped when the slot
> is dropped, and that looks like a sound idea.

Thanks for looking at it!

> > This spurious call has been introduced in be87200efd. I think that initially we
> > designed to drop slots when a recovery conflict occurred during logical decoding
> > on a standby. But we changed our mind to invalidate such a slot instead.
> > 
> > The spurious call is probably due to the initial design but has not been removed.
> 
> This is not a subject that has really been touched on the original
> thread mentioned on the commit, so it is a bit hard to be sure.  The
> only comment is that a previous version of the patch did the stats
> drop in the slot invalidation path at an incorrect location.

The switch in the patch from "drop" to "invalidation" is in [1], see:

"
Given the precedent of max_slot_wal_keep_size, I think it's wrong to
just drop
the logical slots. Instead we should just mark them as
invalid, 
like InvalidateObsoleteReplicationSlots().

Makes fully sense and done that way in the attached patch.
“

But yeah, hard to be sure why this call is there, at least I don't remember...

> > I don't think it's worth to add a test but can do if one feel the need.
> 
> Well, that would not be complicated while being cheap, no?  We have a
> few paths in the 035 test where we know that a slot has been
> invalidated so it is just a matter of querying once
> pg_stat_replication_slot and see if some data is still there.

We can not be 100% sure that the stats are up to date when the process holding
the active slot is killed. 

So v2 attached adds a test where we ensure first that we have non empty stats
before triggering the invalidation.

[1]: https://www.postgresql.org/message-id/26c6f320-98f0-253c-f8b5-df1e7c1f6315%40amazon.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com