Thread: Re: Correcting freeze conflict horizon calculation

On Thu, May 29, 2025 at 10:57 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> However, we calculate the snapshot conflict horizon for the
> prune/freeze record in master/17 and the freeze record in 16 before
> updating all-frozen and all-visible. That means the horizon may be too
> aggressive if the page cannot actually be set all-frozen. This isn't a
> correctness issue, but it may lead to transactions on the standby
> being unnecessarily canceled for pages which have some tuples frozen
> but not all due to remaining LP_DEAD items.

I don't follow.

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?

--
Peter Geoghegan

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?

You're right. I forgot that the visibility_cutoff_xid will be older
than OldestXmin when all the tuples are going to be frozen. I
associate the visibility_cutoff_xid with being younger/newer than
OldestXmin because it often will be when there are newer live tuples
we don't freeze.

And, in the case where the page is not actually all-frozen because of
LP_DEAD items we haven't accounted for yet in the value of all_frozen,
they wouldn't affect the freeze record's snapshot conflict horizon in
16 because they won't be frozen and thus unaffected by the WAL record
and in the case of the prune/freeze WAL record in 17/master, I
calculate the newer of the latest_xid_removed and the snapshot
conflict horizon calculated for freezing, so it would also be fine.

- Melanie

On Fri, May 30, 2025 at 5:16 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> On Thu, May 29, 2025 at 11:37 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > 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?
>
> You're right. I forgot that the visibility_cutoff_xid will be older
> than OldestXmin when all the tuples are going to be frozen.

I added an assertion in a number of places that
"Assert(!TransactionIdIsValid(visibility_cutoff_xid))" when we go to
set a page all-frozen in the VM -- it's irrelvant, and recovery cannot
possibly need a conflict when it replays the record. This seemed
logical, since an all-frozen page must already be visible to every
possible MVCC snapshot (it might also have some small performance
benefit, though that's not really why I did it).

> I associate the visibility_cutoff_xid with being younger/newer than
> OldestXmin because it often will be when there are newer live tuples
> we don't freeze.

I'm not sure what you mean by this. visibility_cutoff_xid can only be
set using tuples that HTSV says are HEAPTUPLE_LIVE according to
VACUUM's OldestXmin cutoff. Personally I find it natural to think of
visibility_cutoff_xid as meaningless unless we're actually able to
apply it in some way.

--
Peter Geoghegan

On Fri, May 30, 2025 at 5:57 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I don't see how OldestXmin comes into play with the visibility_cutoff_xid.

Code in heap_page_is_all_visible() (and other place, I guess the other
one is in pruneheap.c now) have a separate OldestXmin test:

/*
 * The inserter definitely committed. But is it old enough
 * that everyone sees it as committed?
 */
xmin = HeapTupleHeaderGetXmin(tuple.t_data);
if (!TransactionIdPrecedes(xmin,
                           vacrel->cutoffs.OldestXmin))
{
    all_visible = false;
    *all_frozen = false;
    break;
}

Once we "break" here, it doesn't matter what visibility_cutoff_xid has
been set to. It cannot be used for any purpose.

--
Peter Geoghegan