Add hint about replication slots when nearing wraparound

On Tue, Dec 19, 2017 at 5:27 AM, Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

While doing some wraparound debugging, I saw the hint regarding upcoming
wraparound did not include the problem of having a stale replication
slot (which I'm actually using to force wraparound issues).

I remember a few discussions where a stale replication slot was actually the
culprit in these situations.

Something like the attached maybe?

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Dec 20, 2017 at 12:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Dec 19, 2017 at 5:27 AM, Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

While doing some wraparound debugging, I saw the hint regarding upcoming
wraparound did not include the problem of having a stale replication
slot (which I'm actually using to force wraparound issues).

I remember a few discussions where a stale replication slot was actually the
culprit in these situations.

Something like the attached maybe?

+1.

+1.

prepare_transaction.sgml has a "Caution" block mentioning that it is
unwise to keep 2PC transactions unfinished for a too-long time as it
interferes with VACUUM. In doc/src/sgml/logicaldecoding.sgml, it would
be nice to add the a similar caution notice about replication slots so
as users can get be warned before a wraparound shows up.
--
Michael

On 20 December 2017 at 06:22, Michael Paquier <michael.paquier@gmail.com>
wrote:

prepare_transaction.sgml has a "Caution" block mentioning that it is
unwise to keep 2PC transactions unfinished for a too-long time as it
interferes with VACUUM. In doc/src/sgml/logicaldecoding.sgml, it would
be nice to add the a similar caution notice about replication slots so
as users can get be warned before a wraparound shows up.

Added.

As far as I know the issue only occurs for stale replication slots for
logical decoding
but not for physical replication, is that correct?

On Wed, Dec 20, 2017 at 10:00 PM, Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

As far as I know the issue only occurs for stale replication slots for
logical decoding but not for physical replication, is that correct?

Yeah, I recall something similar.

@@ -255,7 +255,9 @@ $ pg_recvlogical -d postgres --slot test --drop-slot
       even when there is no connection using them. This consumes storage
       because neither required WAL nor required rows from the system catalogs
       can be removed by <command>VACUUM</command> as long as they are
required by a replication
-      slot.  So if a slot is no longer required it should be dropped.
+      slot.  In extreme cases this could cause the database to shut
down to prevent
+      transaction ID wraparound (see <xref linkend="vacuum-for-wraparound"/>).
+      So if a slot is no longer required it should be dropped.
      </para>

Don't you want to put that in its own <caution> block? That's rather
important not to miss for administrators.
--
Michael

On 21 December 2017 at 05:32, Michael Paquier <michael.paquier@gmail.com> wrote:

Don't you want to put that in its own <caution> block? That's rather
important not to miss for administrators.

I didn't want to add yet another block on that documentation page,
as it already has 2, however it may be good to upgrade the
note to a caution, similar to the prepared transaction caution.

On Fri, Dec 22, 2017 at 07:55:19AM +0100, Feike Steenbergen wrote:

On 21 December 2017 at 05:32, Michael Paquier <michael.paquier@gmail.com> wrote:

Don't you want to put that in its own <caution> block? That's rather
important not to miss for administrators.

I didn't want to add yet another block on that documentation page,
as it already has 2, however it may be good to upgrade the
note to a caution, similar to the prepared transaction caution.

Yes, I agree with this position.
--
Michael

On 23 December 2017 at 11:58, Michael Paquier <michael.paquier@gmail.com> wrote:

On Fri, Dec 22, 2017 at 07:55:19AM +0100, Feike Steenbergen wrote:

On 21 December 2017 at 05:32, Michael Paquier <michael.paquier@gmail.com> wrote:

Don't you want to put that in its own <caution> block? That's rather
important not to miss for administrators.

I didn't want to add yet another block on that documentation page,
as it already has 2, however it may be good to upgrade the
note to a caution, similar to the prepared transaction caution.

Yes, I agree with this position.

Changed the block from a note to a caution,

regards,

Feike

On Wed, Dec 27, 2017 at 08:47:20AM +0100, Feike Steenbergen wrote:

Changed the block from a note to a caution,

Thanks for the new version.

- "You might also need to commit or roll back old prepared transactions.")));
+ "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
Would "or TO drop stale replication slots" be more correct English?

        ereport(WARNING,
 		(errmsg("oldest xmin is far in the past"),
-		 errhint("Close open transactions soon to avoid wraparound problems.")));
+		 errhint("Close open transactions soon to avoid wraparound problems. You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));

I am not convinced that you need this bit. autovacuum_freeze_max_age can
be set to lower to even lower values than the default.

Still, those are minor comments, so I am marking this patch as ready for
committer to get more input from higher-ups.
--
Michael

On 27 December 2017 at 11:39, Michael Paquier <michael.paquier@gmail.com> wrote:

On Wed, Dec 27, 2017 at 08:47:20AM +0100, Feike Steenbergen wrote:

Changed the block from a note to a caution,

Thanks for the new version.

- "You might also need to commit or roll back old prepared transactions.")));
+ "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
Would "or TO drop stale replication slots" be more correct English?

ereport(WARNING,
(errmsg("oldest xmin is far in the past"),
-                errhint("Close open transactions soon to avoid wraparound problems.")));
+                errhint("Close open transactions soon to avoid wraparound problems. You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));

I am not convinced that you need this bit. autovacuum_freeze_max_age can
be set to lower to even lower values than the default.

Still, those are minor comments, so I am marking this patch as ready for
committer to get more input from higher-ups.

I left that in for consistency.

Applied, thanks.

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