Subject: Re: Physical replication slot advance is not persistent
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, craig@2ndquadrant.com
Cc: michael@paquier.xyz, andres@anarazel.de, pgsql-hackers@postgresql.org,
 simon@2ndquadrant.com
References: <20200121050730.GF2552@paquier.xyz>
 <20200128080114.GB145179@paquier.xyz>
 <CAMsr+YGkLEJQFUVO+OPdwyP7nQosu7fwOZ2XzH7oKw4OG2f+Kw@mail.gmail.com>
 <20200128.211434.296322058369197735.horikyota.ntt@gmail.com>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
Message-ID: <4e060856-dfca-eb31-f60d-616378de3edd@postgrespro.ru>
Date: Tue, 28 Jan 2020 18:06:06 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <20200128.211434.296322058369197735.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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