Re: Pinning a buffer in TupleTableSlot is unnecessary

On 2016-11-14 10:09:02 -0500, Robert Haas wrote:
> On Sat, Nov 12, 2016 at 10:28 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-08-30 07:38:10 -0400, Tom Lane wrote:
> >> Heikki Linnakangas <hlinnaka(at)iki(dot)fi> writes:
> >> > While profiling some queries and looking at executor overhead, I
> >> > realized that we're not making much use of TupleTableSlot's ability to
> >> > hold a buffer pin. In a SeqScan, the buffer is held pinned by the
> >> > underlying heap-scan anyway. Same with an IndexScan, and the SampleScan.
> >>
> >> I think this is probably wrong, or at least very dangerous to remove.
> >> The reason for the feature is that the slot may continue to point at
> >> the tuple after the scan has moved on.
> >
> > FWIW, that's not safe to assume in upper layers *anyway*. If you want to
> > do that, the slot has to be materialized, and that'd make a local
> > copy. If you don't materialize tts_values/isnull can point into random
> > old memory (common e.g. for projections and virtual tuples in general).
>
> So, I think you are arguing in favor of proceeding with this patch?

Not really, now. I don't buy the argument here against it. I do think
the overhead is quite noticeable. But I also think it has quite the
potential for subtle bugs. I think I'd feel better if we had some form
of instrumentation trapping buffer accesses without pins present. We've
previously discussed doing that with valgrind...

Andres