Re: Should we remove vacuum_defer_cleanup_age?

Hi,

On 2023-03-23 10:18:35 +0100, Daniel Gustafsson wrote:
> > On 22 Mar 2023, at 18:00, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > It wasn't actually that much work to write a patch to remove
> > vacuum_defer_cleanup_age, see the attached.
>
> - and <xref linkend="guc-vacuum-defer-cleanup-age"/> provide protection against
> + provides protection against
> relevant rows being removed by vacuum, but the former provides no
> protection during any time period when the standby is not connected,
> and the latter often needs to be set to a high value to provide adequate
>
> Isn't "the latter" in the kept part of the sentence referring to the guc we're
> removing here?

You're right. That paragraph generally seems a bit off. In HEAD:

<para>
In lieu of using replication slots, it is possible to prevent the removal
of old WAL segments using <xref linkend="guc-wal-keep-size"/>, or by
storing the segments in an archive using
<xref linkend="guc-archive-command"/> or <xref linkend="guc-archive-library"/>.
However, these methods often result in retaining more WAL segments than
required, whereas replication slots retain only the number of segments
known to be needed. On the other hand, replication slots can retain so
many WAL segments that they fill up the space allocated
for <literal>pg_wal</literal>;
<xref linkend="guc-max-slot-wal-keep-size"/> limits the size of WAL files
retained by replication slots.
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
and <xref linkend="guc-vacuum-defer-cleanup-age"/> provide protection against
relevant rows being removed by vacuum, but the former provides no
protection during any time period when the standby is not connected,
and the latter often needs to be set to a high value to provide adequate
protection. Replication slots overcome these disadvantages.
</para>

Replication slots alone don't prevent row removal, that requires
hot_standby_feedback to be used as well.

A minimal rephrasing would be:
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/> on its own, without
also using a replication slot, provides protection against relevant rows
being removed by vacuum, but provides no protection during any time period
when the standby is not connected. Replication slots overcome these
disadvantages.
</para>

> - * It's possible that slots / vacuum_defer_cleanup_age backed up the
> - * horizons further than oldest_considered_running. Fix.
> + * It's possible that slots backed up the horizons further than
> + * oldest_considered_running. Fix.
>
> While not the fault of this patch, can't we use the opportunity to expand
> "Fix." to a short "Fix this by ..." sentence? Or remove "Fix." perhaps, either
> of those would improve the comment IMHO.

Perhaps unsurprisingly, given that I wrote that comment, I don't see a problem
with the "Fix."...

Greetings,

Andres Freund