Re: Replication slot xmin is not reset if HS feedback is turned off while standby is shut down

On 21 December 2016 at 13:03, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
> On Wed, Dec 21, 2016 at 2:09 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> On 21 December 2016 at 15:40, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
>>
>>>> So -1 on this part of the patch, unless there's something I've misunderstood.
>>>
>>> Currently there was no feedback sent if hot standby was not active. I
>>> was not sure if it was safe to call GetOldestXmin() in that case.
>>> However I did not consider cascading replica slots wanting to hold
>>> back xmin, where resetting the parents xmin is indeed wrong. Do you
>>> know if GetOldestXmin() is safe at this point and we can just remove
>>> the HotStandbyActive() check? Otherwise I think the correct approach
>>> is to move the check and return inside the hot_standby_feedback case
>>> like this:
>>>
>>> if (hot_standby_feedback)
>>> {
>>> if (!HotStandbyActive())
>>> return;
>>
>> I feel like I'm missing something obvious here. If we force sending
>> hot standby feedback at least once, by assuming
>> master_has_standby_xmin = true at startup, why isn't that sufficient?
>> We'll send InvalidTransactionId if hot_standby_feedback is off. Isn't
>> that the point?
>>
>> It's safe to call GetOldestXmin pretty much as soon as we load the
>> recovery start checkpoint. It won't consider the state of replication
>> slots until later in startup, but that's a pre-existing flaw that
>> should be addressed separately.
>>
>> Why do we need to do more than master_has_standby_xmin = true ?
>
> There was a !HotStandbyActive() check there previously, I was not sure
> if it was only to avoid sending useless messages or was necessary
> because something was not initialized otherwise. Looks like it is not
> needed and can be removed. Revised patch that does so attached.

Valid bug, but this still ain't right. We don't want to turn feedback
on until HS is active, but we do want to turn it off once whether or
not HS is active yet.

We need a full detailed comment explaining this.

Fix it up and I'll commit. Thanks for the report.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services