Kevin Grittner wrote: > On Thu, Aug 25, 2016 at 2:56 PM, Alvaro Herrera > <alvhe...@2ndquadrant.com> wrote: > > > I'm wondering about the TestForOldSnapshot call in scanPendingInsert(). > > Why do we apply it to the metapage buffer (line 1717 in master)? > > If there is any chance that GinPageGetMeta(page)->head could have > changed from a valid block number to InvalidBlockNumber or a > different pending-list page due to a vacuum freeing pages from the > pending-list, the metapage must be checked -- there is no other way > to detect that data might have disappeared.
Hmm ... but the disappearing data is moved to regular GIN pages, right? It doesn't just go away. I suppose that the error would be raised as soon as we scan a GIN data page that, because of receiving data from the pending list, has a newer LSN. I don't know GIN in detail but perhaps it's possible that the data is inserted into data pages in lsn A, then removed from the pending list in lsn B (where A < B). If the snapshot's LSN lies in between, a spurious error would be raised. > > FWIW I like the "revert" commit, because it easily shows me in what > > places you considered a snapshot-too-old test and decided not to add > > one. Bare BufferGetPage calls (the current situation) don't indicate that. > > I'm glad there is some value from having done that little dance. :-) I'm sure that was an annoying thing to do, so I agree. > Thanks for looking this over so closely! You're welcome. I'm not actually reviewing this patch specifically, just trying to figure out a different new feature and reading a few AMs code while at it. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers