Message-ID: <5e803e21-789d-4399-970f-b6ea56e6df9b@iki.fi>
Date: Tue, 5 Mar 2024 09:55:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Missing LWLock protection in pgstat_reset_replslot()
Content-Language: en-US
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <ZeGq1HDWFfLkjh4o@ip-10-97-1-34.eu-west-3.compute.internal>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <ZeGq1HDWFfLkjh4o@ip-10-97-1-34.eu-west-3.compute.internal>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/5e803e21-789d-4399-970f-b6ea56e6df9b%40iki.fi>
Precedence: bulk

On 01/03/2024 12:15, Bertrand Drouvot wrote:
> Hi hackers,
> 
> I think that pgstat_reset_replslot() is missing LWLock protection. Indeed, we
> don't have any guarantee that the slot is active (then preventing it to be
> dropped/recreated) when this function is executed.

Yes, so it seems at quick glance. We have a similar issue in 
pgstat_fetch_replslot(); it might return stats for wrong slot, if the 
slot is dropped/recreated concurrently. Do we care?

> --- a/src/backend/utils/activity/pgstat_replslot.c
> +++ b/src/backend/utils/activity/pgstat_replslot.c
> @@ -46,6 +46,8 @@ pgstat_reset_replslot(const char *name)
>  
>  	Assert(name != NULL);
>  
> +	LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> +
>  	/* Check if the slot exits with the given name. */
>  	slot = SearchNamedReplicationSlot(name, true);

SearchNamedReplicationSlot() will also acquire the lock in LW_SHARED 
mode, when you pass need_lock=true. So that at least should be changed 
to false.

-- 
Heikki Linnakangas
Neon (https://neon.tech)