Re: Pinning a buffer in TupleTableSlot is unnecessary

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. You've given no evidence at all
that that scenario doesn't arise (or that we wouldn't need it in future).

At the very least I'd want to see this patch clearing the slot before
advancing the scan, so that it doesn't have actual dangling pointers
laying about.

> So, how about we remove the ability of a TupleTableSlot to hold a buffer
> pin, per the attached patch? It shaves a few cycles from a
> ExecStoreTuple() and ExecClearTuple(), which get called a lot. I
> couldn't measure any actual difference from that, though, but it seems
> like a good idea from a readability point of view, anyway.

If you can't measure a clear performance improvement, I'm -1 on the
whole thing. You've got risk and breakage of third-party code, and
what to show for it?

> I'll start a different thread on that after
> some more experimentation, but if we e.g. get rid of
> ExecMaterializeSlot() in its current form, would that be OK?

INSERT/UPDATE, for one, relies on that.

regards, tom lane