Re: Review for GetWALAvailability()

On 2020/06/25 12:57, Alvaro Herrera wrote:
> On 2020-Jun-25, Fujii Masao wrote:
>
>> /*
>> * Find the oldest extant segment file. We get 1 until checkpoint removes
>> * 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;
>>
>> I see the point of the above comment, but this can cause wal_status to be
>> changed from "lost" to "unreserved" after the server restart. Isn't this
>> really confusing? At least it seems better to document that behavior.
>
> Hmm.
>
>> Or if we *can ensure* that the slot with invalidated_at set always means
>> "lost" slot, we can judge that wal_status is "lost" without using fragile
>> XLogGetLastRemovedSegno(). Thought?
>
> Hmm, this sounds compelling -- I think it just means we need to ensure
> we reset invalidated_at to zero if the slot's restart_lsn is set to a
> correct position afterwards.

Yes.

> I don't think we have any operation that
> does that, so it should be safe -- hopefully I didn't overlook anything?

We need to call ReplicationSlotMarkDirty() and ReplicationSlotSave()
just after setting invalidated_at and restart_lsn in InvalidateObsoleteReplicationSlots()?
Otherwise, restart_lsn can go back to the previous value after the restart.

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index e8761f3a18..5584e5dd2c 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1229,6 +1229,13 @@ restart:
s->data.invalidated_at = s->data.restart_lsn;
s->data.restart_lsn = InvalidXLogRecPtr;
SpinLockRelease(&s->mutex);
+
+ /*
+ * Save this invalidated slot to disk, to ensure that the slot
+ * is still invalid even after the server restart.
+ */
+ ReplicationSlotMarkDirty();
+ ReplicationSlotSave();
ReplicationSlotRelease();

/* if we did anything, start from scratch */

Maybe we don't need to do this if the slot is temporary?

> Neither copy nor advance seem to work with a slot that has invalid
> restart_lsn.
>
>> Or XLogGetLastRemovedSegno() should be fixed so that it returns valid
>> value even after the restart?
>
> This seems more work to implement.

Yes.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION