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