Hi, In the original thread [0] we established that GIST and SP-GiST support index-only scans, but don't have sufficient interlocking to prevent dead tuples from being revived for their scan by vacuum. As that thread already contains a lot of detail, I won't go into the specifics here, but the gist (heh) of it is as follows:
(SP-)GiST index scans don't interlock with their index vacuum in a way that guarantees that their index-only scans only return TIDs that are known to be valid TIDs when they're returned by amgettuple, thus allowing index vacuum to clean TIDs up and let VACUUM mark their pages ALL_VISIBLE before the tids were returned by the index-only scan. The keen-eyed reader may have noticed that this issue is very similar to the issue fixed with 459e7bf8, and that's correct. Both are cases where the visibility map was accessed to determine the visibility of a TID of which we didn't know for sure it was still a valid TID on that heap page. While the thread in [0] contains a very thorough fix, and (IMO) some good additions to how index-only scans work with table AMs, the patch currently proposed there changes things in non-backpatchable ways. So, attached is a set of 4 patches designed for backpatching a fix. The patches work similar to [0]'s, but with less invasive code changes and no meaningful ABI breaks: index vacuum's lock requirements are increased to super-exclusive (cleanup) locks. Additionally, index-only scans will now do visibility checks of each tuple they want to return, before the pin on the index page that contains those tuples is released. The visibility check is done on a tuple-by-tuple basis, through a new function table_index_vischeck_tuple (so as to not totally break the semantic barrier between indexes and tables regarding visibility checks). In the patchset of [0] I added a table AM routine to allow tableAMs to extend this, but that's not backportable, hence this separate thread for discussing a backpatchable bugfix. The one exposed ABI change is done by adding a single-byte "visrecheck" field in an alignment area in IndexScanDescData - which historically has been the place to put new fields required for backported bugfixes. The size of the struct doesn't change, and there should be no backward or forward compatibility issues for extensions that might make use of this new field as long as they don't assume the value of the field when they haven't set it themselves. Kind regards, Matthias van de Meent Neon (https://neon.tech) [0] https://postgr.es/m/flat/CAH2-Wz=PqOziyRSrnN5jAtfXWXY7-BJcHz9S355LH8Dt=5q...@mail.gmail.com
v01-0002-GIST-Fix-visibility-issues-in-IOS.patch
Description: Binary data
v01-0001-Expose-visibility-checking-shim-for-index-usage.patch
Description: Binary data
v01-0003-SP-GIST-Fix-visibility-issues-in-IOS.patch
Description: Binary data
v01-0004-Test-for-IOS-Vacuum-race-conditions-in-index-AMs.patch
Description: Binary data
