Re: [COMMITTERS] pgsql: Add the "snapshot too old" feature

Kevin Grittner wrote:
> On Thu, Aug 25, 2016 at 2:56 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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