Re: Physical replication slot advance is not persistent

On Tue, Jan 28, 2020 at 06:06:06PM +0300, Alexey Kondratov wrote:
> On 28.01.2020 15:14, Kyotaro Horiguchi wrote:
>> I agree not to save slots immediately. The code is wrtten as described
>> above. The TAP test is correct.
> 
> +1, removing this broken saving code path from pg_replication_slot_advance
> and marking slot as dirty looks good to me. It solves the issue and does not
> add any unnecessary complexity.

Ok, good.  So I am seeing no objections on that part :D

>> But the doc part looks a bit too detailed to me. Couldn't we explain
>> that without the word 'dirty'?
>> 
>> -        and it will not be moved beyond the current insert location.  Returns
>> -        name of the slot and real position to which it was advanced to.
>> +        and it will not be moved beyond the current insert location. Returns
>> +        name of the slot and real position to which it was advanced to. The
>> +        updated slot is marked as dirty if any advancing is done, with its
>> +        information being written out at the follow-up checkpoint. In the
>> +        event of a crash, the slot may return to an earlier position.
>> 
>> and it will not be moved beyond the current insert location. Returns
>> name of the slot and real position to which it was advanced to. The
>> information of the updated slot is scheduled to be written out at the
>> follow-up checkpoint if any advancing is done. In the event of a
>> crash, the slot may return to an earlier position.
> 
> Just searched through the *.sgml files, we already use terms 'dirty' and
> 'flush' applied to writing out pages during checkpoints. Here we are trying
> to describe the very similar process, but in relation to replication slots,
> so it looks fine for me. In the same time, the term 'schedule' is used for
> VACUUM, constraint check or checkpoint itself.

Honestly, I was a bit on the fence for the term "dirty" when typing
this paragraph, so I kind of agree with Horiguchi-san's point that it
could be confusing when applied to replication slots, because there is
no other reference in the docs about the link between the two
concepts.  So, I would go for a more simplified sentence for the first
part, keeping the second sentence intact:
"The information of the updated slot is written out at the follow-up
checkpoint if any advancing is done.  In the event of a crash, the
slot may return to an earlier position."
--
Michael