Re: Review for GetWALAvailability()

At Sat, 13 Jun 2020 01:38:49 +0900, Fujii Masao <masao.fujii@oss.nttdata.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.

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

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

> BTW, if we want to implement GetWALAvailability() as the document
> advertises, we can simplify it like the attached POC patch.

I'm not sure it is right that the patch removes wal_keep_segments from
the function.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center