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

Hi,

On 2023-04-13 13:18:38 +0200, Alvaro Herrera wrote:
> On 2023-Apr-11, Andres Freund wrote:
>
> > Updated patch attached. I think we should either apply something like that
> > patch, or at least add a <warning/> to the docs.
>
> I gave this patch a look.  The only code change is that
> ComputeXidHorizons() and GetSnapshotData() no longer handle the case
> where vacuum_defer_cleanup_age is different from zero.  It looks good.
> The TransactionIdRetreatSafely() code being removed is pretty weird (I
> spent a good dozen minutes writing a complaint that your rewrite was
> faulty, but it turns out I had misunderstood the function), so I'm glad
> it's being retired.

My rewrite of what? The creation of TransactionIdRetreatSafely() in
be504a3e974?

I'm afraid we'll need TransactionIdRetreatSafely() again, when we convert more
things to 64bit xids (lest they end up with the same bug as fixed by
be504a3e974), so it's perhaps worth thinking about how to make it less
confusing.


> >     <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.
> > +    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.
>
> I think it made sense to have this paragraph be separate from the
> previous one when it was talking about two separate variables, but now
> that it's just one, it looks a bit isolated.  I would merge it with the
> one above, which is talking about pretty much the same thing, and
> reorder the whole thing approximately like this
>
>    <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.
>     Similarly, <xref linkend="guc-hot-standby-feedback"/> without
>     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.
>    </para>
>    <para>
>     Replication slots overcome these disadvantages by retaining 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>

It seems a bit confusing now, because "by retaining only the number of
segments ..." now also should cover hs_feedback (due to merging), but doesn't.


> Though the "However," looks a poor fit; I would do this:

I agree, I don't like the however.


I think I'll push the version I had. Then we can separately word-smith the
section? Unless somebody protests I'm gonna do that soon.

Greetings,

Andres Freund