Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

On 2016-05-24 14:48:35 -0500, Kevin Grittner wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
> >> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >>
> >>>> That comment reminds me of a question I had: Did you consider the effect
> >>>> of this patch on analyze? It uses a snapshot, and by memory you've not
> >>>> built in a defense against analyze being cancelled.
>
> The primary defense is not considering a cancellation except in 30
> of the 500 places a page is used. None of those 30 are, as far as
> I can see (upon review in response to your question), used in the
> analyze process.

Uh. How's that a defense? That seems like a recipe for corruption, not a
protection. That might be acceptable in the analyze case, but what about
e.g. concurrent index builds? E.g. IndexBuildHeapRangeScan() doesn't
seem to contain any checks against outdated blocks, and that's certainly
not ok? It appears that concurrent index builds are currently broken
from a quick skim?

> > Is there anything preventing this from becoming a problem?
>
> The fundamental approach that the error can only appear on
> user-facing scans, not internal reads and positioning.

> Unless there is some code path that uses a scan where the snapshot
> age is checked, this cannot be a problem. I don't see any such
> path, but if you do, please let me know, and I'll fix things.

That scares me. Not throwing an error, and not being broken are entirely
different things.

Andres