Re: Correcting freeze conflict horizon calculation

On Thu, May 29, 2025 at 11:37 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> Your concern is that the horizon might be overly aggressive/too
> conservative. But your patch (for 16) makes us take the
> don't-use-snapshotConflictHorizon-twice block *less* frequently (and
> the "use OldestXmin conservatively" block *more* frequently):
>
> - if (prunestate->all_visible && prunestate->all_frozen)
> + if (prunestate->all_visible && prunestate->all_frozen && lpdead_items == 0)
> {
>     /* Using same cutoff when setting VM is now unnecessary */
>     snapshotConflictHorizon = prunestate->visibility_cutoff_xid;
>     prunestate->visibility_cutoff_xid = InvalidTransactionId;
> }
> else
> {
>     /* Avoids false conflicts when hot_standby_feedback in use */
>     snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
>     TransactionIdRetreat(snapshotConflictHorizon);
> }
>
> How can taking the "Avoids false conflicts when hot_standby_feedback
> in use" path more often result in fewer unnecessary conflicts on
> standbys? Isn't it the other way around?

Having discussed this and updated my understanding, now I realize I
don't understand when it would be unsafe to use
prunestate->visibility_cutoff_xid as the snapshot conflict horizon for
the freeze record.

As you've explained, it will always be <= OldestXmin. And, if the
record only freezes tuples (meaning it makes no other changes to the
page) and all of those tuples' xmins were considered when calculating
prunestate->visibility_cutoff_xid, then, even if the page isn't
all-frozen, how could it be incorrect to use the
prunestate->visibility_cutoff_xid as the horizon? Why do we use
OldestXmin when the page wouldn't be all-frozen?

- Melanie