Re: Pinning a buffer in TupleTableSlot is unnecessary

On 11/14/2016 06:18 PM, Tom Lane 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.

It's the other way round. ExecProcNode might not always return its
result in the same slot, but all the callers must assume that it might.

The tuplesort interface is slightly different: the caller passes a slot
to tuplesort_gettupleslot() as argument, and tuplesort_gettupleslot()
places the next tuple in that slot. With executor nodes, a node returns
a slot that it allocated itself, or it got from a child node. After you
call ExecProcNode(), you can *not* assume that the old tuple in the slot
is still valid. The child node can, and in most cases will, reuse the
same slot, overwriting its contents.

I think that difference in the API is exactly what caught Peter by
surprise and led to bug #14344. And I didn't see it either, until you
two debugged it.

> 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 didn't see any such use.

- Heikki