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

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.

> Shouldn't we apply it to the pending-list pages themselves only, if any?

If the pending-list pages are never freed, we could drop the
metapage test.

> (If there are no pending-list pages, surely the age of the snapshot used
> to read the metapage doesn't matter; and if there are, then the age of
> the pending-list pages will fail the test.)

True as long as no pending-list page is ever removed from the
pending-list. If we take out the test, it might be worth a comment
explaining why it's not needed and that it will be needed if
someone modifies the AM to recycle empty pages from the list for
reuse.

> 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. :-)

Thanks for looking this over so closely!

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company