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

[Simon Riggs]

On 13 January 2017 at 10:17, Ants Aasma  wrote:
> On 5 Jan 2017 2:54 a.m., "Craig Ringer"  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 Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

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

[Ants Aasma]

On 5 Jan 2017 2:54 a.m., "Craig Ringer"  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
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index c6b54ec..0ab936f 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1157,7 +1157,10 @@ XLogWalRcvSendReply(bool force, bool requestReply)
  * in case they don't have a watch.
  *
  * If the user disables feedback, send one final message to tell sender
- * to forget about the xmin on this standby.
+ * to forget about the xmin on this standby. We also send this message
+ * on first connect because a previous connection might have set xmin
+ * on a replication slot. (If we're not using a slot it's harmless to
+ * send a feedback message explicitly setting InvalidTransactionId).
  */
 static void
 XLogWalRcvSendHSFeedback(bool immed)
@@ -1167,7 +1170,8 @@ XLogWalRcvSendHSFeedback(bool immed)
 	uint32		nextEpoch;
 	TransactionId xmin;
 	static TimestampTz sendTime = 0;
-	static bool master_has_standby_xmin = false;
+	/* initially true so we always send at least one feedback message */
+	static bool master_has_standby_xmin = true;
 
 	/*
 	 * If the user doesn't want status to be reported to the master, be sure
@@ -1192,14 +1196,17 @@ XLogWalRcvSendHSFeedback(bool immed)
 	}
 
 	/*
-	 * If Hot Standby is not yet active there is nothing to send. Check this
-	 * after the interval has expired to reduce number of calls.
+	 * If Hot Standby is not yet accepting connections there is nothing to
+	 * send. Check this after the interval has expired to reduce number of
+	 * calls.
+	 *
+	 * Bailing out here also ensures that we don't send feedback until we've
+	 * read our own replication slot state, so we don't tell the master to
+	 * discard needed xmin or catalog_xmin from any slots that may exist
+	 * on this replica.
 	 */
 	if (!HotStandbyActive())
-	{
-		Assert(!master_has_standby_xmin);
 		return;
-	}
 
 	/*
 	 * Make the expensive call to get the oldest xmin once we are certain
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index eef512d..7fb2e9e 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 22;
+use Test::More tests => 24;
 
 # Initialize master node
 my $node_master = get_new_node('master');
@@ -161,3 +161,22 @@ is($catalog_xmin, '', 'non-cascaded slot xmin still null with hs_feedback reset'
 ($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
 is($xmin, '', 'cascaded slot xmin null with hs feedback reset');
 is($catalog_xmin, '', 'cascaded slot xmin still null with hs_feedback reset');
+
+diag "re-enabling hot_standby_feedback and disabling while stopped";
+$node_standby_2->safe_psql('postgres', 'ALTER SYSTEM SET hot_standby_feedback = on;');
+$node_standby_2->reload;
+
+$node_master->safe_psql('postgres', qq[INSERT INTO tab_int VALUES (11000);]);
+replay_check();
+
+$node_standby_2->safe_psql('postgres', 'ALTER SYSTEM SET hot_standby_feedback = off;');
+$node_standby_2->stop;
+
+($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
+isnt($xmin, '', 'cascaded slot xmin non-null with postgres shut down');
+
+# Xmin from a previous run should be cleared on startup.
+$node_standby_2->start;
+
+($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
+is($xmin, '', 'cascaded slot xmin reset after startup with hs feedback reset');

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

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

[Ants Aasma]

On 5 Jan 2017 2:54 a.m., "Craig Ringer"  wrote:

On 2 January 2017 at 22:24, Craig Ringer  wrote:
>
>
> On 2 Jan. 2017 20:20, "Simon Riggs"  wrote:
>
> On 21 December 2016 at 13:23, Simon Riggs  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.


I'm travelling right now, but I should be able to give it a shot next week.

Regards,
Ants Aasma

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

[Craig Ringer]

On 2 January 2017 at 22:24, Craig Ringer  wrote:
>
>
> On 2 Jan. 2017 20:20, "Simon Riggs"  wrote:
>
> On 21 December 2016 at 13:23, Simon Riggs  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


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

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

[Craig Ringer]

On 2 Jan. 2017 20:20, "Simon Riggs"  wrote:

On 21 December 2016 at 13:23, Simon Riggs  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.

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

[Simon Riggs]

On 21 December 2016 at 13:23, Simon Riggs  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 Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

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

[Craig Ringer]

On 21 December 2016 at 21:23, Simon Riggs  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


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

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

[Craig Ringer]

On 21 December 2016 at 21:03, Ants Aasma  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.
 */
boolSharedHotStandbyActive;


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
From 5d86d42e30ccb3dd3e1b227b102384762d66e9dd Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 21 Dec 2016 21:28:32 +0800
Subject: [PATCH] Reset xmin if hot_standby_feedback is turned off while
 shutdown

If the user turns hot_standby_feedback off while we're shut down or the
walreciever is restarting, we won't remember that we sent an xmin to the
server. This never used to matter since the xmin was tied to the walsender
backend PGPROC, but now that we have replication slots it's persistent across
connections.

Always send at least one hot_standby_feedback message after walreceiver
startup, even if hot_standby_feedback is off, to ensure any saved
slot xmin from a prior connection is cleared.

Ants Aasma, Craig Ringer
---
 src/backend/replication/walreceiver.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index cc3cf7d..11de996 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1157,7 +1157,10 @@ XLogWalRcvSendReply(bool force, bool requestReply)
  * in case they don't have a watch.
  *
  * If the user disables feedback, send one final message to tell sender
- * to forget about the xmin on this standby.
+ * to forget about the xmin on this standby. We also send this message
+ * on first connect because a previous connection might have set xmin
+ * on a replication slot. (If we're not using a slot it's harmless to
+ * send a feedback message explicitly setting InvalidTransactionId).
  */
 static void
 XLogWalRcvSendHSFeedback(bool immed)
@@ -1167,7 +1170,8 @@ XLogWalRcvSendHSFeedback(bool immed)
 	uint32		nextEpoch;
 	TransactionId xmin;
 	static TimestampTz sendTime = 0;
-	static bool master_has_standby_xmin = false;
+	/* initially true so we always send at least one feedback message */
+	static bool master_has_standby_xmin = true;
 
 	/*
 	 * If the user doesn't want status to be reported to the master, be sure
@@ -1192,14 +1196,17 @@ XLogWalRcvSendHSFeedback(bool immed)
 	}
 
 	/*
-	 * If Hot Standby is not yet active there is nothing to send. Check this
-	 * after the interval has expired to reduce number of calls.
+	 * If Hot Standby is not yet accepting connections there is nothing to
+	 * send. Check this after the interval has expired to reduce number of
+	 * calls.
+	 *
+	 * Bailing out here also ensures that we don't send feedback until we've
+	 * read our own replication slot state, so we don't tell the master to
+	 * discard needed xmin or catalog_xmin from any slots that may exist
+	 * on this replica.
 	 */
 	if (!HotStandbyActive())
-	{
-		Assert(!master_has_standby_xmin);
 		return;
-	}
 
 	/*
 	 * Make the expensive call to get the oldest xmin once we are certain
-- 
2.5.5


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

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

[Simon Riggs]

On 21 December 2016 at 13:03, Ants Aasma  wrote:
> On Wed, Dec 21, 2016 at 2:09 PM, Craig Ringer  wrote:
>> On 21 December 2016 at 15:40, Ants Aasma  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 Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

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

[Ants Aasma]

On Wed, Dec 21, 2016 at 2:09 PM, Craig Ringer  wrote:
> On 21 December 2016 at 15:40, Ants Aasma  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
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index cc3cf7d..84ffa91 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1157,7 +1157,9 @@ XLogWalRcvSendReply(bool force, bool requestReply)
  * in case they don't have a watch.
  *
  * If the user disables feedback, send one final message to tell sender
- * to forget about the xmin on this standby.
+ * to forget about the xmin on this standby. We also send this message
+ * on first connect because a previous connection might have set xmin
+ * on a replication slot.
  */
 static void
 XLogWalRcvSendHSFeedback(bool immed)
@@ -1167,7 +1169,8 @@ XLogWalRcvSendHSFeedback(bool immed)
 	uint32		nextEpoch;
 	TransactionId xmin;
 	static TimestampTz sendTime = 0;
-	static bool master_has_standby_xmin = false;
+	/* initially true so we always send at least one feedback message */
+	static bool master_has_standby_xmin = true;
 
 	/*
 	 * If the user doesn't want status to be reported to the master, be sure
@@ -1192,16 +1195,6 @@ XLogWalRcvSendHSFeedback(bool immed)
 	}
 
 	/*
-	 * If Hot Standby is not yet active there is nothing to send. Check this
-	 * after the interval has expired to reduce number of calls.
-	 */
-	if (!HotStandbyActive())
-	{
-		Assert(!master_has_standby_xmin);
-		return;
-	}
-
-	/*
 	 * Make the expensive call to get the oldest xmin once we are certain
 	 * everything else has been checked.
 	 */

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

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

[Craig Ringer]

On 21 December 2016 at 15:40, Ants Aasma  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


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

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

[Ants Aasma]

On Wed, Dec 21, 2016 at 4:14 AM, Craig Ringer  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


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

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

[Michael Paquier]

On Wed, Dec 21, 2016 at 11:14 AM, Craig Ringer  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


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

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

[Craig Ringer]

On 21 December 2016 at 00:52, Ants Aasma  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


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

[HACKERS] Replication slot xmin is not reset if HS feedback is turned off while standby is shut down

[Ants Aasma]

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
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index cc3cf7d..31333ec 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1157,7 +1157,9 @@ XLogWalRcvSendReply(bool force, bool requestReply)
  * in case they don't have a watch.
  *
  * If the user disables feedback, send one final message to tell sender
- * to forget about the xmin on this standby.
+ * to forget about the xmin on this standby. We also send this message
+ * on first connect because a previous connection might have set xmin
+ * on a replication slot.
  */
 static void
 XLogWalRcvSendHSFeedback(bool immed)
@@ -1167,7 +1169,7 @@ XLogWalRcvSendHSFeedback(bool immed)
 	uint32		nextEpoch;
 	TransactionId xmin;
 	static TimestampTz sendTime = 0;
-	static bool master_has_standby_xmin = false;
+	static bool master_has_standby_xmin = true;
 
 	/*
 	 * If the user doesn't want status to be reported to the master, be sure
@@ -1192,20 +1194,13 @@ XLogWalRcvSendHSFeedback(bool immed)
 	}
 
 	/*
-	 * If Hot Standby is not yet active there is nothing to send. Check this
-	 * after the interval has expired to reduce number of calls.
-	 */
-	if (!HotStandbyActive())
-	{
-		Assert(!master_has_standby_xmin);
-		return;
-	}
-
-	/*
 	 * Make the expensive call to get the oldest xmin once we are certain
 	 * everything else has been checked.
+	 *
+	 * 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())
 		xmin = GetOldestXmin(NULL, false);
 	else
 		xmin = InvalidTransactionId;


slot-xmin-not-reset-reproduce.sh
Description: Bourne shell script

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