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

On 21 December 2016 at 00:52, Ants Aasma <ants(dot)aasma(at)eesti(dot)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