Message-ID: <8147005b-42b5-47d5-84a4-53965568fd6a@oss.nttdata.com>
Date: Mon, 30 Jun 2025 21:42:47 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Mention idle_replication_slot_timeout in pg_replication_slots
 docs
To: Nisha Moond <nisha.moond412@gmail.com>
Cc: pgsql-docs@lists.postgresql.org
References: <78b34e84-2195-4f28-a151-5d204a382fdd@oss.nttdata.com>
 <CABdArM5r8wD8w1dTMh+ozXZw5TbiWRYHzrc2tCy6PNOsffHvLA@mail.gmail.com>
 <938e2d16-4449-413b-a2fc-2595e732e3e1@oss.nttdata.com>
 <CABdArM5qHWK9MTjq9Jj=35UYqv02BjRam0FPCLUHNaMceACpcA@mail.gmail.com>
 <c9d23cdc-b5dd-455a-8ee9-f1f24d701d89@oss.nttdata.com>
 <CABdArM4KqFVoSzZBYs4jPBEo=4e1kjcOpCF8rWZSthxZFcnWdw@mail.gmail.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: 
 <CABdArM4KqFVoSzZBYs4jPBEo=4e1kjcOpCF8rWZSthxZFcnWdw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/8147005b-42b5-47d5-84a4-53965568fd6a%40oss.nttdata.com>
Precedence: bulk


On 2025/06/30 20:32, Nisha Moond wrote:
> On Fri, Jun 27, 2025 at 5:40=E2=80=AFPM Fujii Masao <masao.fujii@oss.nt=
tdata.com> wrote:
>>
>>
>>
>> On 2025/06/27 15:32, Nisha Moond wrote:
>>> On Thu, Jun 26, 2025 at 1:33=E2=80=AFPM Fujii Masao <masao.fujii@oss.=
nttdata.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2025/06/26 15:46, Nisha Moond wrote:
>>>>> On Wed, Jun 25, 2025 at 9:56=E2=80=AFPM Fujii Masao <masao.fujii@os=
s.nttdata.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> The pg_replication_slots documentation mentions only max_slot_wal_=
keep_size
>>>>>> as a condition under which the wal_status column can show unreserv=
ed or lost.
>>>>>> However, since commit ac0e33136ab, idle_replication_slot_timeout c=
an also
>>>>>> cause this behavior when it is set. This has not been documented y=
et.
>>>>>> https://www.postgresql.org/docs/devel/view-pg-replication-slots.ht=
ml
>>>>>>
>>>>>
>>>>> +1 to the doc update.
>>>>
>>>> Thanks for the review!
>>>>
>>>>
>>>>>> So, how about updating the documentation to also mention
>>>>>> idle_replication_slot_timeout as a factor that can cause wal_statu=
s to
>>>>>> become unreserved or lost? Patch attached.
>>>>>>
>>>>>
>>>>> Since idle_replication_slot_timeout can only cause wal_status to
>>>>> become 'lost' and not 'unreserved', perhaps we can reword the sente=
nce
>>>>> slightly for clarity, suggestion -
>>>>> "The last two states are seen when max_slot_wal_keep_size is
>>>>> non-negative and, the 'lost' state may also appear when
>>>>> idle_replication_slot_timeout is greater than zero."
>>>>
>>>> I was thinking that when idle_replication_slot_timeout triggers,
>>>> the following functions are called, and that wal_status can become
>>>> "unreserved" before ReplicationSlotRelease() runs. It's very short
>>>> period, though. Am I wrong?
>>>>
>>>> ReplicationSlotMarkDirty();
>>>> ReplicationSlotSave();
>>>> ReplicationSlotRelease();
>>>>
>>>
>>> Thank you for pointing it out.
>>> You are correct that while the checkpointer is in the process of
>>> invalidating a slot, it sets its PID as the slot=E2=80=99s active_pid=
. During
>>> this short window, if a user queries pg_replication_slot, the
>>> underlying function pg_get_replication_slots will compute the
>>> wal_status as 'unreserved' for the invalidated slot because the slot
>>> has a valid active_pid.
>>>
>>> That said, it's reasonable to mention in the doc that 'unreserved' ma=
y
>>> appear when idle_replication_slot_timeout is greater than zero, as
>>> this can indeed happen. So, let's retain the current description.
>>>
>>> However, this behavior isn=E2=80=99t specific to
>>> idle_replication_slot_timeout. For example, when a slot is being
>>> invalidated due to a different cause "wal_level_insufficient",
>>> 'unreserved' may also briefly appear in wal_status.
>>
>> Yes, and "lost" can appear for various reasons, including wal_level_in=
sufficient,
>> so it seems odd to highlight max_slot_wal_keep_size as the cause of th=
e "lost"
>> status in the note. It would probably be better to remove the mention =
of "lost"
>> from that note.
>>
>=20
> +1

Is this true starting from v16, when logical replication from standby was=
 introduced?
In other words, in v15 and earlier, only max_slot_wal_keep_size could cau=
se
the wal_status to become "unreserved" or "lost"? I'm wondering where to b=
ack-patch
this fix to.


>> As for "unreserved", it can also occur for different reasons, but typi=
cally,
>> it happens when max_slot_wal_keep_size is set to a non-negative value.
>> So it might make sense to keep the explanation focused just on "unrese=
rved"
>> and max_slot_wal_keep_size. For example:
>>
>> ----------------------
>>            <listitem>
>>             <para>
>>              <literal>unreserved</literal> means that the slot no long=
er
>>              retains the required WAL files and some of them are to be=
 removed at
>> -          the next checkpoint.  This state can return
>> +          the next checkpoint.  This can occur when
>> +          <xref linkend=3D"guc-max-slot-wal-keep-size"/> is set to
>> +          a non-negative value.  This state can return
>>              to <literal>reserved</literal> or <literal>extended</lite=
ral>.
>>             </para>
>>            </listitem>
>>            <listitem>
>> ----------------------
>>
>> What do you think?
>>
>=20
> The change LGTM, only a minor suggestion to add "typically", as =E2=80=9C=
This
> can typically occur when=E2=80=A6=E2=80=9D to indicate that max_slot_wa=
l_keep_size is
> one possible reason, not the only one.

OK.


>> Also, I noticed the note that says =E2=80=9CIf <structfield>restart_ls=
n</structfield>
>> is NULL, this field is null=E2=80=9D seems inaccurate. For example, wh=
en "wal_removed"
>> happens, restart_lsn is NULL but wal_status is "lost". So maybe we sho=
uld remove
>> that note as well?
>=20
> You're right, the statement is not accurate.
> We could rephrase it as: "If <structfield>restart_lsn</structfield> is
> NULL, this field is either null or lost." But since 'unreserved' can
> also appear briefly during invalidation, it might be better to remove
> it altogether.=20
I agree with removing the description. Unless I'm missing something,
it has been incorrect since at least v13, so we should back-patch this fi=
x
to all supported versions.

Regards,

--=20
Fujii Masao
NTT DATA Japan Corporation