Re: Review for GetWALAvailability()

On 2020/06/15 13:42, Kyotaro Horiguchi wrote:
> At Sat, 13 Jun 2020 01:38:49 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>> Hi,
>>
>> The document explains that "lost" value that
>> pg_replication_slots.wal_status reports means
>>
>> some WAL files are definitely lost and this slot cannot be used to
>> resume replication anymore.
>>
>> However, I observed "lost" value while inserting lots of records,
>> but replication could continue normally. So I wonder if
>> pg_replication_slots.wal_status may have a bug.
>>
>> wal_status is calculated in GetWALAvailability(), and probably I found
>> some issues in it.
>>
>>
>> keepSegs = ConvertToXSegs(Max(max_wal_size_mb, wal_keep_segments),
>> wal_segment_size) +
>> 1;
>>
>> max_wal_size_mb is the number of megabytes. wal_keep_segments is
>> the number of WAL segment files. So it's strange to calculate max of
>> them.
>
> Oops! I don't want to believe I did that but it's definitely wrong.
>
>> The above should be the following?
>>
>> Max(ConvertToXSegs(max_wal_size_mb, wal_segment_size),
>> wal_keep_segments) + 1
>
> Looks reasonable.
>
>> if ((max_slot_wal_keep_size_mb <= 0 ||
>> max_slot_wal_keep_size_mb >= max_wal_size_mb) &&
>> oldestSegMaxWalSize <= targetSeg)
>> return WALAVAIL_NORMAL;
>>
>> This code means that wal_status reports "normal" only when
>> max_slot_wal_keep_size is negative or larger than max_wal_size.
>> Why is this condition necessary? The document explains "normal
>> means that the claimed files are within max_wal_size". So whatever
>> max_slot_wal_keep_size value is, IMO that "normal" should be
>> reported if the WAL files claimed by the slot are within max_wal_size.
>> Thought?
>
> It was a kind of hard to decide. Even when max_slot_wal_keep_size is
> smaller than max_wal_size, the segments more than
> max_slot_wal_keep_size are not guaranteed to be kept. In that case
> the state transits as NORMAL->LOST skipping the "RESERVED" state.
> Putting aside whether the setting is useful or not, I thought that the
> state transition is somewhat abrupt.

IMO the direct transition of the state from normal to lost is ok to me
if each state is clearly defined.

>> Or, if that condition is really necessary, the document should be
>> updated so that the note about the condition is added.
>
> Does the following make sense?
>
> https://www.postgresql.org/docs/13/view-pg-replication-slots.html
>
> normal means that the claimed files are within max_wal_size.
> + If max_slot_wal_keep_size is smaller than max_wal_size, this state
> + will not appear.

I don't think this change is enough. For example, when max_slot_wal_keep_size
is smaller than max_wal_size and the amount of WAL files claimed by the slot
is smaller thhan max_slot_wal_keep_size, "reserved" is reported. But which is
inconsistent with the meaning of "reserved" in the docs.

To consider what should be reported in wal_status, could you tell me what
purpose and how the users is expected to use this information?

>> If the WAL files claimed by the slot exceeds max_slot_wal_keep_size
>> but any those WAL files have not been removed yet, wal_status seems
>> to report "lost". Is this expected behavior? Per the meaning of "lost"
>> described in the document, "lost" should be reported only when
>> any claimed files are removed, I think. Thought?
>>
>> Or this behavior is expected and the document is incorrect?
>
> In short, it is known behavior but it was judged as useless to prevent
> that.
>
> That can happen when checkpointer removes up to the segment that is
> being read by walsender. I think that that doesn't happen (or
> happenswithin a narrow time window?) for physical replication but
> happenes for logical replication.
>
> While development, I once added walsender a code to exit for that
> reason, but finally it is moved to InvalidateObsoleteReplicationSlots
> as a bit defferent function. With the current mechanism, there's a
> case where once invalidated slot came to revive but we decided to
> allow that behavior, but forgot to document that.
>
> Anyway if you see "lost", something bad is being happening.
>
> - lost means that some WAL files are definitely lost and this slot
> - cannot be used to resume replication anymore.
> + lost means that some required WAL files are removed and this slot is
> + no longer usable after once disconnected during this status.
>
> If it is crucial that the "lost" state may come back to reserved or
> normal state,
>
> + Note that there are cases where the state moves back to reserved or
> + normal state when all wal senders have left the just removed segment
> + before being terminated.
>
> There is a case where the state moves back to reserved or normal state when wal senders leaves the just removed segment before being terminated.

Even if walsender is terminated during the state "lost", unless checkpointer
removes the required WAL files, the state can go back to "reserved" after
new replication connection is established. This is the same as what you're
explaining at the above?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION