Re: Review for GetWALAvailability()

Thanks for looking this.

At Fri, 19 Jun 2020 18:23:59 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in 
> On 2020-Jun-17, Kyotaro Horiguchi wrote:
> 
> > @@ -9524,7 +9533,7 @@ GetWALAvailability(XLogRecPtr targetLSN)
> >  	 * the first WAL segment file since startup, which causes the status being
> >  	 * wrong under certain abnormal conditions but that doesn't actually harm.
> >  	 */
> > -	oldestSeg = XLogGetLastRemovedSegno() + 1;
> > +	oldestSeg = last_removed_seg + 1;
> >  
> >  	/* calculate oldest segment by max_wal_size and wal_keep_segments */
> >  	XLByteToSeg(currpos, currSeg, wal_segment_size);
> 
> This hunk should have updated the comment two lines above.  However:
> 
> > @@ -272,6 +273,14 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
> >  	rsinfo->setResult = tupstore;
> >  	rsinfo->setDesc = tupdesc;
> >  
> > +	/*
> > +	 * Remember the last removed segment at this point for the consistency in
> > +	 * this table. Since there's no interlock between slot data and
> > +	 * checkpointer, the segment can be removed in-between, but that doesn't
> > +	 * make any practical difference.
> > +	 */
> > +	last_removed_seg = XLogGetLastRemovedSegno();
> 
> I am mystified as to why you added this change.  I understand that your
> point here is to make all slots reported their state as compared to the
> same LSN, but why do it like that?  If a segment is removed in between,
> it could mean that the view reports more lies than it would if we update
> the segno for each slot.  I mean, suppose two slots are lagging behind
> and one is reported as 'extended' because when we compute it it's still
> in range; then a segment is removed.  With your coding, we'll report
> both as extended, but with the original coding, we'll report the new one
> as lost.  By the time the user reads the result, they'll read one
> incorrect report with the original code, and two incorrect reports with
> your code.  So ... yes it might be more consistent, but what does that
> buy the user?

I agree to you. Anyway the view may show "wrong" statuses if
concurrent WAL-file removal is running. But I can understand it is
better that the numbers in a view are consistent.  The change
contributes only to that point. So I noted as "doesn't make any
practical difference".  Since it is going to be removed, I removed the
changes for the part.

https://www.postgresql.org/message-id/9ddfbf8c-2f67-904d-44ed-cf8bc5916228@oss.nttdata.com

> OTOH it makes GetWALAvailability gain a new argument, which we have to
> document.
> 
> > +	/*
> > +	 * However segments required by the slot has been lost, if walsender is
> > +	 * active the walsender can read into the first reserved slot.
> > +	 */
> > +	if (slot_is_active)
> > +		return WALAVAIL_BEING_REMOVED;
> 
> I don't understand this comment; can you please clarify what you mean?

I have had comments that the "lost" state should be a definite state,
that is, a state mustn't go back to other states.  I had the same from
Fujii-san again.

Suppose we are starting from the following situation:

State A:
|---- seg n-1 ----|---- seg n ----|
                 ^
                 X (restart_lsn of slot S) - max_slot_wal_keep_size 

If the segment n-1 is removed, slot S's status becomes
"lost". However, if the walsender that is using the slot has not been
killed yet, the point X can move foward to the segment n (State B).

State B:
|XXXX seg n-1 XXXX|---- seg n ----|
                   ^
                   X (restart_lsn of slot S) - max_slot_wal_keep_size 

This is the normal (or extend) state.  If we want to the state "lost"
to be definitive, we cannot apply the state label "lost" to State A if
it is active.

WALAVAIL_BEING_REMOVED (I noticed it has been removed for a wrong
reason so I revived it in this patch [1].) was used for the same state,
that is, the segment at restart_lsn will be removed soon but not yet.

1: https://www.postgresql.org/message-id/20200406.185027.648866525989475817.horikyota.ntt@gmail.com

> I admit I don't like this slot_is_active argument you propose to add to
> GetWALAvailability either; previously the function can be called with
> an LSN coming from anywhere, not just a slot; the new argument implies
> that the LSN comes from a slot.  (Your proposed patch doesn't document
> this one either.)

Agreed. I felt like you at the time.  I came up with another way after
hearing that from you.

In the attached GetWALAvailability() returns the state assuming the
walsender is not active. And the caller (pg_get_replication_slots())
considers the case where the walsender is active.

regares.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center