Re: [HACKERS] Restricting maximum keep segments by repslots

Hi,

First, it seems you did not reply to Alvaro's concerns in your new set of
patch. See:

https://www.postgresql.org/message-id/20190917195800.GA16694%40alvherre.pgsql

On Tue, 24 Dec 2019 21:26:14 +0900 (JST)
Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
[...]
> > Indeed, "loosing" is a better match for this state.
> >
> > However, what's the point of this state from the admin point of view? In
> > various situation, the admin will have no time to react immediately and fix
> > whatever could help.
> >
> > How useful is this specific state?
>
> If we assume "losing" segments as "lost", a segment once "lost" can
> return to "keeping" or "streaming" state. That is intuitively
> impossible. On the other hand if we assume it as "keeping", it should
> not be removed by the next checkpoint but actually it can be
> removed. The state "losing" means such a unstable state different from
> both "lost" and "keeping".

OK, indeed.

But I'm still unconfortable with this "unstable" state. It would be better if
we could grab a stable state: either "keeping" or "lost".

> > + <entry>Availability of WAL records claimed by this
> > + slot. <literal>streaming</literal>, <literal>keeping</literal>,
> >
> > Slots are keeping WALs, not WAL records. Shouldn't it be "Availability of
> > WAL files claimed by this slot"?
>
> I choosed "record" since a slot points a record. I'm not sure but I'm
> fine with "file". Fixed catalogs.sgml and config.sgml that way.

Thanks.

> > + <literal>streaming</literal> means that the claimed records are
> > + available within max_wal_size. <literal>keeping</literal> means
> >
> > I wonder if streaming is the appropriate name here. The WALs required might
> > be available for streaming, but the slot not active, thus not "streaming".
> > What about merging with the "active" field, in the same fashion as
> > pg_stat_activity.state? We would have an enum "pg_replication_slots.state"
> > with the following states:
> > * inactive: non active slot
> > * active: activated, required WAL within max_wal_size
> > * keeping: activated, max_wal_size is exceeded but still held by replication
> > slots or wal_keep_segments.
> > * lost: some WAL are definitely lost
> >
> > Thoughts?
>
> In the first place, I realized that I was missed a point about the
> relationship between max_wal_size and max_slot_wal_keep_size
> here. Since the v15 of this patch, GetLsnAvailablity returns
> "streaming" when the restart_lsn is within max_wal_size. That behavior
> makes sense when max_slot_wal_keep_size > max_wal_size. However, in
> the contrary case, restart_lsn could be lost even it is
> withinmax_wal_size. So we would see "streaming" (or "normal") even
> though restart_lsn is already lost. That is broken.
>
> In short, the "streaming/normal" state is useless if
> max_slot_wal_keep_size < max_wal_size.

Good catch!

> Finally I used the following wordings.
>
> (there's no "inactive" wal_state)
>
> * normal: required WAL within max_wal_size when max_slot_wal_keep_size
> is larger than max_wal_size.
> * keeping: required segments are held by replication slots or
> wal_keep_segments.
>
> * losing: required segments are about to be removed or may be already
> removed but streaming is not dead yet.

As I wrote, I'm still uncomfortable with this state. Maybe we should ask
other reviewers opinions on this.

[...]
> > WARNING: some replication slots have lost required WAL segments
> > DETAIL: Slot slot_limit_st lost 177 segment(s)
> >
> > I wonder if this is useful to show these messages for slots that were
> > already dead before this checkpoint?
>
> Makes sense. I changed KeepLogSeg so that it emits the message only on
> slot_names changes.

Thanks.

Bellow some code review.

In regard with FindOldestXLogFileSegNo(...):

> /*
> * Return the oldest WAL segment file.
> *
> * The returned value is XLogGetLastRemovedSegno() + 1 when the function
> * returns a valid value. Otherwise this function scans over WAL files and
> * finds the oldest segment at the first time, which could be very slow.
> */
> XLogSegNo
> FindOldestXLogFileSegNo(void)

The comment is not clear to me. I suppose "at the first time" might better be
expressed as "if none has been removed since last startup"?

Moreover, what about patching XLogGetLastRemovedSegno() itself instead of
adding a function?

In regard with GetLsnAvailability(...):

> /*
> * Detect availability of the record at given targetLSN.
> *
> * targetLSN is restart_lsn of a slot.

Wrong argument name. It is called "restart_lsn" in the function
declaration.

> * restBytes is the pointer to uint64 variable, to store the remaining bytes
> * until the slot goes into "losing" state.

I'm not convinced with this argument name. What about "remainingBytes"? Note
that you use remaining_bytes elsewhere in your patch.

> * -1 is stored to restBytes if the values is useless.

What about returning a true negative value when the slot is really lost?

All in all, I feel like this function is on the fence between being generic
because of its name and being slot-only oriented because of the first parameter
name, use of "max_slot_wal_keep_size_mb", returned status and "slotPtr".

I wonder if it should be more generic and stay here or move to xlogfuncs.c with
a more specific name?

> * slot limitation is not activated, WAL files are kept unlimitedlllly

"unlimitedly"? "infinitely"? "unconditionally"?

> /* it is useless for the states below */
> *restBytes = -1;

This might be set to the real bytes kept, even if status is "losing".

> * The segment is alrady lost or being lost. If the oldest segment is just

"already"

> if (oldestSeg == restartSeg + 1 && walsender_pid != 0)
> return "losing";

I wonder if this should be "oldestSeg > restartSeg"?
Many segments can be removed by the next or running checkpoint. And a running
walsender can send more than one segment in the meantime I suppose?

In regard with GetOldestKeepSegment(...):

> static XLogSegNo
> GetOldestKeepSegment(XLogRecPtr currLSN, XLogRecPtr minSlotLSN,
> XLogRecPtr targetLSN, int64 *restBytes)

I wonder if minSlotLSN is really useful as a parameter or if it should be
fetched from GetOldestKeepSegment() itself? Currently,
XLogGetReplicationSlotMinimumLSN() is always called right before
GetOldestKeepSegment() just to fill this argument.

> walstate =
> GetLsnAvailability(restart_lsn, active_pid, &remaining_bytes);

I agree with Alvaro: we might want to return an enum and define the related
state string here. Or, if we accept negative remaining_bytes, GetLsnAvailability
might even only return remaining_bytes and we deduce the state directly from
here.

Regards,