Re: [HACKERS] Restricting maximum keep segments by repslots

At Fri, 3 Apr 2020 20:14:03 -0300, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote in
> So, the more I look at this patch, the less I like the way the slots are
> handled.
>
> * I think it's a mistake to try to do anything in KeepLogSeg itself;
> that function is merely in charge of some arithmetic. I propose to
> make that function aware of the new size limitation (so that it
> doesn't trust the slot's LSNs completely), but to make the function
> have no side effects. The attached patch does that, I hope.
> To replace that responsibility, let's add another function. I named it
> InvalidateObsoleteReplicationSlots(). In CreateCheckPoint and
> CreateRestartPoint, we call the new function just before removing
> segments. Note: the one in this patch doesn't actually work or even
> compile.

Agreed and thanks for the code. The patch is enough to express the
intention. I fixed some compilation errors and made a clean up of
KeepLogSeg. InvalidateObsoleteReplicationSlots requires the "oldest
preserved segment" so it should be called before _logSegNo--, not
after.

> The new function must:
>
> 1. mark the slot as "invalid" somehow. Maybe it would make sense to
> add a new flag in the on-disk struct for this; but for now I'm just
> thinking that changing the slot's restart_lsn is sufficient.
> (Of course, I haven't tested this, so there might be side-effects that
> mean that this idea doesn't work).
>
> 2. send SIGTERM to a walsender that's using such a slot.
>
> 3. Send the warning message. Instead of trying to construct a message
> with a list of slots, send one message per slot. (I think this is
> better from a translatability point of view, and also from a
> monitoring PoV).
>
> * GetWalAvailability seems too much in competition with
> DistanceToWalRemoval. Which is weird, because both functions do
> pretty much the same thing. I think a better design is to make the
> former function return the distance as an out parameter.

I agree to the aboves. When a slot is invlidated, the following
message is logged.

LOG: slot rep1 is invalidated at 0/1C00000 due to exceeding max_slot_wal_keep_size

> * Andres complained that the "distance" column was not a great value to
> expose (20171106132050(dot)6apzynxrqrzghb4r(at)alap3(dot)anarazel(dot)de). That's
> right: it changes both by the insertion LSN as well as the slot's
> consumption. Maybe we can expose the earliest live LSN (start of the
> earliest segment?) as a new column. It'll be the same for all slots,
> I suppose, but we don't care, do we?

I don't care as far as users can calculate the "remain" of individual
slots (that is, how far the current LSN can advance before the slot
loses data). But the "earliest live LSN (EL-LSN) is really not
relevant to the safeness of each slot. The distance from EL-LSN to
restart_lsn or the current LSN doesn't generally suggest the safeness
of individual slots. The only relevance would be if the distance from
EL-LSN to the current LSN is close to max_slot_wal_keep_size, the most
lagged slot could die in a short term.

FWIW, the relationship between the values are shown below.

(now)>>>
<--- past ----------------------------+--------------------future --->
lastRemovedSegment + 1
"earliest_live_lsn" | segment X |
| min(restart_lsn) restart_lsn[i] current_lsn | "The LSN X"
.+...+................+...............+>>>..............|...+ |
<--------max_slot_wal_keep_size------> |
<---"remain" --------------->|

So the "remain" is calculated using "restart_lsn(pg_lsn)",
max_slot_wal_keep_size(int in MB), wal_keep_segments(in segments) and
wal_segment_size (int in MB) and pg_current_wal_lsn()(pg_lsn). The
formula could be simplified by ignoring the segment size, but anyway
we don't have an arithmetic between pg_lsn and int in SQL interface.

Anyway in this version I added the "min_safe_lsn". And adjust the TAP
tests for that. It can use (pg_current_wal_lsn() - min_safe_lsn) as
the alternative index since there is only one slot while the test.

> I attach a rough sketch, which as I said before doesn't work and doesn't
> compile. Sadly I have reached the end of my day here so I won't be able
> to work on this for today anymore. I'll be glad to try again tomorrow,
> but in the meantime I thought it was better to send it over and see
> whether you had any thoughts about this proposed design (maybe you know
> it doesn't work for some reason), or better yet, you have the chance to
> actually complete the code or at least move it a little further.

WALAVAIL_BEING_REMOVED is removed since walsender is now actively
killed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center