Re: Pinning a buffer in TupleTableSlot is unnecessary

Hi,

On 2016-08-30 13:12:41 +0300, Heikki Linnakangas wrote:
> 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.

FWIW, I came to a similar conclusion, while working on passing around
making the executor batched.

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

I actually found that rather beneficial from a performance point of
view.

> How much do we need to worry about breaking extensions? I.e. to what extent
> do we consider the TupleTableSlot API to be stable, for extensions to use?
> The FDW API uses TupleTableSlots - this patch had to fix the ExecStoreTuple
> calls in postgres_fdw.

I think we're just going to have to deal with the fallout. Trying to
maintain backward compatibility with the TupleTableSlot API seems to
prevent some rather important improvement in the area.

> We could refrain from changing the signature of ExecStoreTuple(), and throw
> an error if you try to pass a valid buffer to it. But I also have some
> bigger changes to TupleTableSlot in mind.

We should probably coordinate ;)

> I think we could gain some speed
> by making slots "read-only". For example, it would be nice if a SeqScan
> could just replace the tts_tuple pointer in the slot, and not have to worry
> that the upper nodes might have materialized the slot, and freeing the
> copied tuple. Because that happens very rarely in practice. It would be
> better if those few places where we currently materialize an existing slot,
> we would create a copy of the slot, and leave the original slot unmodified.
> 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?

Hm.

Greetings,

Andres Freund