Re: Review for GetWALAvailability()

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?

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

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services