Re: Pinning a buffer in TupleTableSlot is unnecessary - Mailing list pgsql-hackers
From | Heikki Linnakangas |
---|---|
Subject | Re: Pinning a buffer in TupleTableSlot is unnecessary |
Date | |
Msg-id | fb2a7cac-d03e-2c0b-27c0-7b9b146fd70c@iki.fi Whole thread Raw |
In response to | Re: Pinning a buffer in TupleTableSlot is unnecessary (Tom Lane <tgl@sss.pgh.pa.us>) |
List | pgsql-hackers |
On 08/30/2016 02:38 PM, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.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
Attachment
pgsql-hackers by date: