Re: Should we remove vacuum_defer_cleanup_age? - Mailing list pgsql-hackers

Hi,

On 2023-03-23 10:18:35 +0100, Daniel Gustafsson wrote:
> > On 22 Mar 2023, at 18:00, Andres Freund <andres@anarazel.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