Re: Spurious pgstat_drop_replslot() call

From: Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Spurious pgstat_drop_replslot() call
Date: 2024-03-08 15:04:10
Message-ID: Zeso6vvK+k/
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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
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.



Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services:

Attachment Content-Type Size
v2-0001-Remove-spurious-pgstat_drop_replslot-call.patch text/x-diff 3.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-03-08 15:24:48 Re: Call perror on popen failure
Previous Message Robert Treat 2024-03-08 14:52:27 Re: [DOC] Add detail regarding resource consumption wrt max_connections