On Thu, 9 Jan 2025 at 22:00, Michail Nikolaev <michail.nikol...@gmail.com> wrote: > > Hello. > > One thing I think we could add to the patches is to adapt the 10-years-old > comment below with notice about IOS: > > /* > * We save the LSN of the page as we read it, so that we know whether it > * safe to apply LP_DEAD hints to the page later. This allows us to drop > * the pin for MVCC scans, which allows vacuum to avoid blocking. > */ > so->curPageLSN = BufferGetLSNAtomic(buffer);
I don't quite read it as covering IOS. To me, the comment is more along the lines of (extensively extended): """ We'd like to use kill_prior_tuple, but that requires us to apply changes to the page when we're already done with it for all intents and purposes (because we scan the page once and buffer results). We can either keep a pin on the buffer, or re-acquire that page after finishing producing the tuples from this page. Pinning the page blocks vacuum [^1], so instead we drop the pin, then collect LP_DEAD marks, and then later we re-acquire the page to mark the tuples dead. However, in the meantime the page may have changed; by keeping a tab on changes in LSN we have a cheap method of detecting changes in the page iself. [^2] """ ... and that doesn't seem to cover much of IOS. MVCC index scans aren't that special, practically every user query has MVCC. I think this "MVCC scan" even means non-IOS scan, as I can't think of a reason why dropping a pin would otherwise be a valid behaviour (see the this thread's main issue). [^1] Well, it should. In practice, the code in HEAD doesn't, but this patchset fixes that disagreement. [^2] If the page changed, i.e. the LSN changed, GIST accepts that it can't use the collected LP_DEAD marks. We may be able to improve on that (or not) by matching LP_DEAD offsets and TIDs with the on-page TIDs, but that's far outside the scope of this patch; we'd first have to build an understanding about why it's correct to assume vacuum hasn't finished reaping old tuples and other sessions also finished inserting new ones with the same TIDs in the meantime. > Also, I think it is a good idea to add "Assert(!scan->xs_want_itup);" to > gistkillitems. Why would it be incorrect or invalid to kill items in an index-only scan? If we hit the heap (due to ! VM_ALL_VISIBLE) and detected the heap tuple was dead, why couldn't we mark it as dead in the index? IOS assumes a high rate of all-visible pages, but it's hardly unheard of to access pages with dead tuples. Kind regards, Matthias van de Meent Neon (https://neon.tech)
