Re: Pinning a buffer in TupleTableSlot is unnecessary

On 08/30/2016 02:38 PM, 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. You've given no evidence at all
> that that scenario doesn't arise (or that we wouldn't need it in future).

For a SeqScan, the buffer is pinned, and the tuple stays valid, because
we have an open HeapScan on it. It will stay valid until the next
heap_getnext() call.

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

Fair enough, although we're not 100% careful about that currently
either. For examples, see gather_getnext, CopyFrom, and others.
Personally I think that's OK, as long as the window between invalidating
the underlying buffer (or other source of the tuple), and storing a new
tuple in it, is small enough. In particular, I think this is OK:

tuple = heap_getnext(); /* this leaves a dangling pointer in slot */
slot = ExecStoreTuple(tuple); /* and this makes it valid again */

>> 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?

Well, I think simplicity is a virtue all by itself. But ok, let me try a
bit harder to benchmark this:

I created a test table with:

create table foo as select repeat('x', 500) || g from generate_series(1,
1000000) g;
vacuum freeze;

And then ran:

$ cat count.sql
select count(*) from foo;
$ pgbench -n -f count.sql postgres -t1000

This is pretty much a best case for this patch. A "count(*)" doesn't do
much else than put the tuple in the slot, and the tuples are wide
enough, that you switch from one buffer to another every 15 tuples,
which exercises the buffer pinning code.

I ran that pgbench command three times with and without the patch, and
picked the best times:

Without patch:
tps = 12.413360 (excluding connections establishing)

With the patch:
tps = 13.183164 (excluding connections establishing)

That's about 6% improvement.

This was with the attached version of this patch. Compared to the
previous, I added ExecClearTuple() calls to clear the slots before
advancing the heap/index scan, to avoid the dangling pointers. Doing
that added an extra function call to this hot codepath, however, so I
compensated for that by adding an inline version of ExecStoreTuple(),
for those callers that know that the slot is already empty.

BTW, turning ExecStoreVirtualTuple into a static inline function would
also be an easy way to shave some cycles. But I refrained from including
that change into this patch.

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

Sure. I'm thinking of refactoring that so that it doesn't. Or maybe add
a new kind of a TupleTableSlot that cannot be materialized, which makes
ExecStore/ClearTuple for that slot simpler, and use that in SeqScan and
friends. INSERT/UPDATE could continue to use ExecMaterializeSlot(), but
it would need to have a slot of its own, instead of materializing the
slot it got from plan tree. Yes, this is still very hand-wavey...

- Heikki