Обсуждение: 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 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
backold 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
rollback 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