Re: BUG #17255: Server crashes in index_delete_sort_cmp() due to race condition with vacuum

Hi,

On 2021-11-10 19:33:01 -0800, Peter Geoghegan wrote:
> On Wed, Nov 10, 2021 at 7:16 PM Andres Freund <andres@anarazel.de> wrote:
> > We can't really do the same with the
> > hard horizons. It's currently super likely that the horizon moves forward a
> > lot between vacuum_set_xid_limits() and GlobalVisTestFor(), but moving it
> > closer still is going in the wrong direction.
> 
> ...why is that super likely? I have to admit that I have no idea what
> you mean by that.

Err, I missed a *not* in there, sorry for the confusion.


> > So I'm inclined to add a comment saying that we need to do the
> > GlobalVisTestFor(), pointing to the comment in ComputeXidHorizons(), and
> > adding a TODO that we should find a heuristic when to recompute horizons?
> 
> Okay. Even still, I suggest that you say something about this new
> DELETE_IN_PROGRESS pruning behavior in vacuumlazy.c. Maybe in
> lazy_scan_prune()?

That doesn't quite make sense to me. There's quite a few ways that a HTSV can
change for the same tuple, and listing them all in vacuumlazy.c doesn't really
help people notice that when they add a new call or such. There's plenty
other callsites - like the one in heap_prune_chain()...

How about changing the comment above lazy_scan_prune() to note that the
concurrent abort case is one example of this behaviour? And then add a comment
to HeapTupleSatisfiesVacuum() along the lines of:

NB: Repeated calls to HeapTupleSatisfiesVacuum for the same tuple may change
even while the buffer is locked. Cases of this include:
- the transaction that inserted a tuple (version) aborted, changing the result
  from HEAPTUPLE_INSERT_IN_PROGRESS to HEAPTUPLE_DEAD
- when used with GlobalVisTestIsRemovableXid(), HEAPTUPLE_DELETE_IN_PROGRESS
  can change to HEAPTUPLE_DEAD if the horizon is updated

However, HEAPTUPLE_DEAD will not change back to HEAPTUPLE_RECENTLY_DEAD or
such.


> > > We could probably *also* freeze tuples opportunistically (e.g., freeze
> > > a few tuples on a page early to be able to mark it all-frozen sooner),
> >
> > We should definitely do that when a page is already being dirtied for other
> > reasons.
> 
> Agreed. But at the very least we should be thinking about the
> whole-page picture. If we made freezing a whole page cheap enough
> (smaller WAL records), then maybe the separate all-visible state
> becomes totally unnecessary (except for pg_upgrade).

I doubt it. But I think getting rid of unnecessarily dirtying the page a
second (or third) time would be a big enough win already...

Greetings,

Andres Freund