On 28.01.2020 15:14, Kyotaro Horiguchi wrote:
> At Tue, 28 Jan 2020 17:45:47 +0800, Craig Ringer <craig@2ndquadrant.com> wrote in
>> On Tue, 28 Jan 2020 at 16:01, Michael Paquier <michael@paquier.xyz> wrote:
>>> So attached is an updated patch which addresses the problem just by
>>> marking a physical slot as dirty if any advancing is done. Some
>>> documentation is added about the fact that an advance is persistent
>>> only at the follow-up checkpoint. And the tests are fixed to not use
>>> a fake LSN but instead advance to the latest LSN position produced.
>>>
>>> Any objections?
>> LGTM. Thankyou.
> 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.
>
> 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.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
