Re: heapam_index_build_range_scan's anyvisible

On Fri, Jun 7, 2019 at 4:30 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> On 2019-Jun-07, Robert Haas wrote:
> > I spent some time today studying heapam_index_build_range_scan and
> > quickly reached the conclusion that it's kind of a mess. At heart
> > it's pretty simple: loop over all the table, check each tuple against
> > any qual, and pass the visible ones to the callback. However, in an
> > attempt to make it cater to various needs slightly outside of its
> > original design purpose, various warts have been added, and there are
> > enough of them now that I at least find it fairly difficult to
> > understand. One of those warts is anyvisible, which I gather was
> > added in support of BRIN.
>
> Yes, commit 2834855cb9fd added that flag. SnapshotNonVacuumable did not
> exist back then. It seems like maybe it would work to remove the flag
> and replace with passing SnapshotNonVacuumable. The case that caused
> that flag to be added is tested by a dedicated isolation test, so if
> BRIN becomes broken by the change at least it'd be obvious ...

Yeah, I wondered whether SnapshotNonVacuumable might've been added
later, but I was too lazy to check the commit log. I'll try coding up
that approach and see how it looks.

But do you have any comment on the question of whether this function
is actually safe with < ShareLock, per the comments about caching
HOT-related state across buffer lock releases?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company