Date: Thu, 11 Nov 2021 12:05:17 -0800
From: Andres Freund <andres@anarazel.de>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17255: Server crashes in index_delete_sort_cmp() due to
 race condition with vacuum
Message-ID: <20211111200517.fcg342s5mirwdacc@alap3.anarazel.de>
References: 
 <CAH2-Wzma=Y3O+LRx2Wj_HwGbbbeNwr6FoJzXni8hxOMw55pcZg@mail.gmail.com>
 <CAH2-WznNKY6ydUczuTXutVmb_dj3MnAcoaVYc8xyignWfNQ=FQ@mail.gmail.com>
 <20211110192010.ckvfzz352hsba5xf@alap3.anarazel.de>
 <CAH2-WzmqBtFwdmBW=AG8ZZW_uF5a4entmv4QmqhsXOT-Fj4L-g@mail.gmail.com>
 <20211111004727.imn7xrimdiyo7vfv@alap3.anarazel.de>
 <CAH2-Wz=QrJsLN3UhQ2KOXsdubhqfuNUQiJDjESubscPdaDT5eg@mail.gmail.com>
 <20211111021618.n5ebam6f6licj746@alap3.anarazel.de>
 <CAH2-Wz=S91VGD7QgROVYM8A_Ou6FWUi+UE-OdQxVEAYEgd9R2A@mail.gmail.com>
 <20211111031641.kqj4w64klqt7ta2h@alap3.anarazel.de>
 <CAH2-Wz=cLQBJEYOD=c8HBwx9+s2X38Y6nhd=WGRFtUHS8fQwWg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAH2-Wz=cLQBJEYOD=c8HBwx9+s2X38Y6nhd=WGRFtUHS8fQwWg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20211111200517.fcg342s5mirwdacc%40alap3.anarazel.de>
Precedence: bulk

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