Re: Pinning a buffer in TupleTableSlot is unnecessary

On Mon, Nov 14, 2016 at 11:18 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Nov 14, 2016 at 10:23 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I don't believe Andres' claim anyway. There are certainly cases where an
>>> allegedly-valid slot could be pointing at garbage, but table scans aren't
>>> one of them, precisely because of the pin held by the slot. It would take
>>> a fairly wide-ranging code review to convince me that it's okay to lose
>>> that mechanism.
>
>> I don't understand your objection. It seems to me that the
>> TupleTableSlot is holding a pin, and the scan is also holding a pin,
>> so one of them is redundant. You speculated that the slot could
>> continue to point at the tuple after the scan has moved on, but how
>> could such a thing actually happen?
>
> You're implicitly assuming that a scan always returns its results in the
> same slot, and that no other slot could contain a copy of that data, but
> there is no guarantee of either. See bug #14344 and d8589946d for a
> pretty recent example where that failed to be true --- admittedly, for
> a tuplesort scan not a table scan.
>
> We could certainly set up some global convention that would make this
> universally true, but I think we'd have to do a lot of code-reading
> to ensure that everything is following that convention.
>
> Also, there might well be places that are relying on the ability of a
> slot to hold a pin for slots that are not simply the return slot of
> a plan node. We could perhaps remove the *use* of this slot feature in
> the normal table-scan case, without removing the feature itself.

I'm still not getting it. We don't have to guess who is using this
feature; we can find it out by looking at every place that calls
ExecStoreTuple and looking at whether they are passing InvalidBuffer.
As Heikki's original patch upthread demonstrates, most of them are.
The exceptions are in BitmapHeapNext, IndexNext, ExecOnConflictUpdate,
SampleNext, SeqNext, and TidNext. If all of those places are or can
be made to hold a buffer pin for as long as the slot would have held
the same pin, we're done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company