Thread: [HACKERS] Replication slot xmin is not reset if HS feedback is turned off whilestandby is shut down

I just had a client issue with table bloat that I traced back to a
stale xmin value in a replication slot. xmin value from hot standby
feedback is stored in replication slot and used for vacuum xmin
calculation. If hot standby feedback is turned off while walreceiver
is active then the xmin gets reset by HS feedback message containing
InvalidTransactionId. However, if feedback gets turned off while
standby is shut down this message never gets sent and a stale value
gets left behind in the replication slot holding back vacuum.

The simple fix seems to be to always send out at least one feedback
message on each connect regardless of hot_standby_feedback setting.
Patch attached. Looks like this goes back to version 9.4. It could
conceivably cause issues for replication middleware that does not know
how to handle hot standby feedback messages. Not sure if any exist and
if that is a concern.

A shell script to reproduce the problem is also attached, adjust the
PGPATH variable to your postgres install and run in an empty
directory.

Regards,
Ants Aasma

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 21 December 2016 at 00:52, Ants Aasma <ants.aasma@eesti.ee> wrote:

> The simple fix seems to be to always send out at least one feedback
> message on each connect regardless of hot_standby_feedback setting.

I agree.

> Patch attached. Looks like this goes back to version 9.4. It could
> conceivably cause issues for replication middleware that does not know
> how to handle hot standby feedback messages. Not sure if any exist and
> if that is a concern.

If it exists it's already broken. Not our problem IMO.

> A shell script to reproduce the problem is also attached, adjust the
> PGPATH variable to your postgres install and run in an empty
> directory.

I wrote some TAP tests for 9.6 (they're on the logical decoding on
standby thread) that can be extended to check this. Once they're
committed we can add a test for this change.

Re the patch, I don't like

-       static bool master_has_standby_xmin = false;
+       static bool master_has_standby_xmin = true;

without any comment. It's addressed in the comment changes on the
header func, but the link isn't obvious. Maybe a oneliner to say
"ensure we always send at least one feedback message" ?

I think this part of the patch is correct and useful.



I don't see the point of

+        *
+        * If Hot Standby is not yet active we reset the xmin value. Check this
+        * after the interval has expired to reduce number of calls.        */
-       if (hot_standby_feedback)
+       if (hot_standby_feedback && HotStandbyActive())

though. Forcing feedback to send InvalidTransactionId until hot
standby feedback is actively running seems counterproductive; we want
to lock in feedback as soon as possible, not wait until we're
accepting transactions. Simon and I are in fact working on changes to
do the opposite of what you've got here and ensure that we don't allow
hot standby connections until we know hot_standby_feedback is in
effect and a usable xmin is locked in. That way the master won't
remove tuples we need as soon as we start an xact and cause a conflict
with recovery.

If there are no running xacts, GetOldestXmin() will return the slot
xmin if any. We should definitely not be clearing that just because
we're not accepting hot standby connections yet; we want to make very
sure it remains in effect unless the user explicitly de-configures hot
standby.
(There's another pre-exisitng problem there, where we can start the
walsender before slots are initialized, that I'll be addressing
separately).

If there's no slot xmin either, we'll send nextXid. That's a sensible
starting point for hot standby feedback until we start some
transactions.

So -1 on this part of the patch, unless there's something I've misunderstood.


I didn't see this in the CF app so I created it in
https://commitfest.postgresql.org/12/ as
https://commitfest.postgresql.org/12/924/ . I couldn't find your
PostgreSQL community username, so please log in and set yourself as
author. I have set patch state to "waiting on author" pending your
addressing these remarks.


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Dec 21, 2016 at 11:14 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> I didn't see this in the CF app so I created it in
> https://commitfest.postgresql.org/12/ as
> https://commitfest.postgresql.org/12/924/ . I couldn't find your
> PostgreSQL community username, so please log in and set yourself as
> author. I have set patch state to "waiting on author" pending your
> addressing these remarks.

You need to import the user first, which is what I did and I have
added the author name correctly.
(Glad you jumped on this patch, I was just going to begin a lookup. So
that's a nice timing.)
-- 
Michael

On Wed, Dec 21, 2016 at 4:14 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Re the patch, I don't like
>
> -       static bool master_has_standby_xmin = false;
> +       static bool master_has_standby_xmin = true;
>
> without any comment. It's addressed in the comment changes on the
> header func, but the link isn't obvious. Maybe a oneliner to say
> "ensure we always send at least one feedback message" ?

Will fix.

> I think this part of the patch is correct and useful.
>
>
>
> I don't see the point of
>
> +        *
> +        * If Hot Standby is not yet active we reset the xmin value. Check this
> +        * after the interval has expired to reduce number of calls.
>          */
> -       if (hot_standby_feedback)
> +       if (hot_standby_feedback && HotStandbyActive())
>
> though. Forcing feedback to send InvalidTransactionId until hot
> standby feedback is actively running seems counterproductive; we want
> to lock in feedback as soon as possible, not wait until we're
> accepting transactions. Simon and I are in fact working on changes to
> do the opposite of what you've got here and ensure that we don't allow
> hot standby connections until we know hot_standby_feedback is in
> effect and a usable xmin is locked in. That way the master won't
> remove tuples we need as soon as we start an xact and cause a conflict
> with recovery.
>
> If there are no running xacts, GetOldestXmin() will return the slot
> xmin if any. We should definitely not be clearing that just because
> we're not accepting hot standby connections yet; we want to make very
> sure it remains in effect unless the user explicitly de-configures hot
> standby.
>
>  (There's another pre-exisitng problem there, where we can start the
> walsender before slots are initialized, that I'll be addressing
> separately).
>
> If there's no slot xmin either, we'll send nextXid. That's a sensible
> starting point for hot standby feedback until we start some
> transactions.
>
> 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;

Regards,
Ants Aasma

On 21 December 2016 at 15:40, Ants Aasma <ants.aasma@eesti.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 ?

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Dec 21, 2016 at 2:09 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 21 December 2016 at 15:40, Ants Aasma <ants.aasma@eesti.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.

Regards,
Ants Aasma

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 21 December 2016 at 13:03, Ants Aasma <ants.aasma@eesti.ee> wrote:
> On Wed, Dec 21, 2016 at 2:09 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> On 21 December 2016 at 15:40, Ants Aasma <ants.aasma@eesti.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

On 21 December 2016 at 21:03, Ants Aasma <ants.aasma@eesti.ee> wrote:

> 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.

Ah, see, I'm blind. Too much multi-tasking. Turns out patch review
with a toddler's help is hard ;)

HotStandbyActive() call is actually just checking if we're still in
crash or archive recovery, per

        /*
         * SharedHotStandbyActive indicates if we're still in crash or archive
         * recovery.  Protected by info_lck.
         */
        bool            SharedHotStandbyActive;


so it is appropriate to defer any hot standby feedback until then. By
that point we've definitely finished setting up replication slots for
one thing, and know for sure if we have something useful to say and
won't send the wrong thing.

It looks to me like we should continue to bail out if !HotStandbyActive() . The

    Assert(!master_has_standby_xmin)

can go, since there's now a valid case where master_has_standby_xmin
can be true before HotStandbyActive() is true.

Here's a revised version. I haven't run it against your tests yet.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 21 December 2016 at 21:23, Simon Riggs <simon@2ndquadrant.com> wrote:

> 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.

I just posted an update, though I forgot to add you to the direct Cc
list. It passes the provided test script.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 21 December 2016 at 13:23, Simon Riggs <simon@2ndquadrant.com> wrote:

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

I was hoping for some more effort from Ants to correct this.

I'll commit Craig's new tests for hs feedback before this, so it can
go in with a Tap test, so I imagine we're about a week away from
commit on this.

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

On 2 January 2017 at 22:24, Craig Ringer <craig@2ndquadrant.com> wrote:
>
>
> On 2 Jan. 2017 20:20, "Simon Riggs" <simon@2ndquadrant.com> wrote:
>
> On 21 December 2016 at 13:23, Simon Riggs <simon@2ndquadrant.com> wrote:
>
>> Fix it up and I'll commit. Thanks for the report.
>
> I was hoping for some more effort from Ants to correct this.
>
> I'll commit Craig's new tests for hs feedback before this, so it can
> go in with a Tap test, so I imagine we're about a week away from
> commit on this.
>
>
> I posted a revised version of Ants's patch that passes the shell script
> test.
>
> A TAP  test would likely make sene though, I agree.


Ants, do you think you'll have a chance to convert your shell script
test into a TAP test in src/test/recovery?

Simon has said he would like to commit this fix. I'd personally be
happier if it had a test to go with it.

You could probably just add to src/test/recover/t/001 which now
contains my additions for hot standby.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 5 Jan 2017 2:54 a.m., "Craig Ringer" <craig@2ndquadrant.com> wrote:
> Ants, do you think you'll have a chance to convert your shell script
> test into a TAP test in src/test/recovery?
>
> Simon has said he would like to commit this fix. I'd personally be
> happier if it had a test to go with it.
>
> You could probably just add to src/test/recover/t/001 which now
> contains my additions for hot standby.

Do you feel the test in the attached patch is enough or would you like
to see anything else covered?

Regards,
Ants Aasma

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13 January 2017 at 10:17, Ants Aasma <ants.aasma@eesti.ee> wrote:
> On 5 Jan 2017 2:54 a.m., "Craig Ringer" <craig@2ndquadrant.com> wrote:
>> Ants, do you think you'll have a chance to convert your shell script
>> test into a TAP test in src/test/recovery?
>>
>> Simon has said he would like to commit this fix. I'd personally be
>> happier if it had a test to go with it.
>>
>> You could probably just add to src/test/recover/t/001 which now
>> contains my additions for hot standby.
>
> Do you feel the test in the attached patch is enough or would you like
> to see anything else covered?

That looks good, thanks for the patch.

Applying in next few hours, barring objections; then backpatching code
(without tests).

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