Re: min_safe_lsn column in pg_replication_slots view

On 2020/06/15 16:35, Kyotaro Horiguchi wrote:
> At Mon, 15 Jun 2020 13:44:31 +0900, Michael Paquier <michael@paquier.xyz> wrote in
>> On Mon, Jun 15, 2020 at 12:40:03PM +0900, Fujii Masao wrote:
>>> BTW, I just wonder why each row in pg_replication_slots needs to have
>>> min_safe_lsn column? Basically min_safe_lsn should be the same between
>>> every replication slots.
>>
>> Indeed, that's confusing in its current shape.  I would buy putting
>> this value into pg_replication_slots if there were some consistency of
>> the data to worry about because of locking issues, but here this data
>> is controlled within info_lck, which is out of the the repslot LW
>> lock.  So I think that it is incorrect to put this data in this view
>> and that we should remove it, and that instead we had better push for
>> a system view that maps with the contents of XLogCtl.

Agreed. But as you know it's too late to do that for v13...
So firstly I'd like to fix the issues in pg_replication_slots view,
and then we can improve the things later for v14 if necessary.


> It was once the difference from the safe_lsn to restart_lsn which is
> distinct among slots. Then it was changed to the safe_lsn. I agree to
> the discussion above, but it is needed anywhere since no one can know
> the margin until the slot goes to the "lost" state without it.  (Note
> that currently even wal_status and min_safe_lsn can be inconsistent in
> a line.)
> 
> Just for the need for table-consistency and in-line consistency, we
> could just remember the result of XLogGetLastRemovedSegno() around
> taking info_lock in the function.  That doesn't make a practical
> difference but makes the view look consistent.

Agreed. Thanks for the patch. Here are the review comments:


Not only the last removed segment but also current write position
should be obtain at the beginning of pg_get_replication_slots()
and should be given to GetWALAvailability(), for the consistency?


Even after applying your patch, min_safe_lsn is calculated for
each slot even though the calculated result is always the same.
Which is a bit waste of cycle. We should calculate min_safe_lsn
at the beginning and use it for each slot?


            XLogSegNoOffsetToRecPtr(last_removed_seg + 1, 0,

Isn't it better to use 1 as the second argument of the above,
in order to address the issue that I reported upthread?
Otherwise, the WAL file name that pg_walfile_name(min_safe_lsn) returns
would be confusing.

Regards,

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