Обсуждение: Why is it not sane to pass ExecStoreTuple(shouldFree=true) for tuples point into buffers

Hi,

I've the need to persist a the result of an index_getnext() in a tuple
slot. I don't want to unneccessarily duplicate the tuple data itself, so
I'd like to use ExecStoreTuple(buffer = real_buffer) notion. But since
the next index_getnext()/index_endscan() will overwrite/release the
heaptuple I need to copy the HeapTupleData().
It'd be rather useful to be able to do ExecStoreTuple(tuple, slot,
some_buffer, true), i.e. hav ethe HeapTupleData struct freed *and* the
buffer pinned. There's an Assert preventing that though.

Why?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> I've the need to persist a the result of an index_getnext() in a tuple
> slot. I don't want to unneccessarily duplicate the tuple data itself, so
> I'd like to use ExecStoreTuple(buffer = real_buffer) notion. But since
> the next index_getnext()/index_endscan() will overwrite/release the
> heaptuple I need to copy the HeapTupleData().
> It'd be rather useful to be able to do ExecStoreTuple(tuple, slot,
> some_buffer, true), i.e. hav ethe HeapTupleData struct freed *and* the
> buffer pinned. There's an Assert preventing that though.

There's an assumption that if you are asking to pin a buffer, the tuple
pointer you're passing is pointing into that buffer (and is therefore not
something that could be pfree'd).  If it isn't pointing into a buffer,
the tuple slot is not the place to be keeping the buffer reference.

I'm disinclined to remove that Assert because failing to pin a buffer
when you *are* passing a pointer into it is a very bad, hard-to-find bug.
Admittedly the Assert is only a partial defense against that problem,
but it's better than nothing.
        regards, tom lane

On 2014-04-07 15:58:31 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > I've the need to persist a the result of an index_getnext() in a tuple
> > slot. I don't want to unneccessarily duplicate the tuple data itself, so
> > I'd like to use ExecStoreTuple(buffer = real_buffer) notion. But since
> > the next index_getnext()/index_endscan() will overwrite/release the
> > heaptuple I need to copy the HeapTupleData().
> > It'd be rather useful to be able to do ExecStoreTuple(tuple, slot,
> > some_buffer, true), i.e. hav ethe HeapTupleData struct freed *and* the
> > buffer pinned. There's an Assert preventing that though.
> 
> There's an assumption that if you are asking to pin a buffer, the tuple
> pointer you're passing is pointing into that buffer (and is therefore not
> something that could be pfree'd).  If it isn't pointing into a buffer,
> the tuple slot is not the place to be keeping the buffer reference.

Yea. I *have* a HeapTupleData struct pointing into the buffer. It's just
that the lifetime of the indexscans's xs_ctup isn't sufficient for my
case. So I have to allocate a new HeapTupleData struct, *again* pointing
into the buffer. I'd like to manage the lifetime of that HeapTupleData
struct (*not* the entire HeapTupleHeader blob) via the slot.
That doesn't sound too crazy to me?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-04-07 15:58:31 -0400, Tom Lane wrote:
>> There's an assumption that if you are asking to pin a buffer, the tuple
>> pointer you're passing is pointing into that buffer (and is therefore not
>> something that could be pfree'd).  If it isn't pointing into a buffer,
>> the tuple slot is not the place to be keeping the buffer reference.

> Yea. I *have* a HeapTupleData struct pointing into the buffer. It's just
> that the lifetime of the indexscans's xs_ctup isn't sufficient for my
> case. So I have to allocate a new HeapTupleData struct, *again* pointing
> into the buffer. I'd like to manage the lifetime of that HeapTupleData
> struct (*not* the entire HeapTupleHeader blob) via the slot.
> That doesn't sound too crazy to me?

In that case you should have another tuple slot of your own and let it
keep the tuple (and buffer pin).
        regards, tom lane

On 2014-04-07 16:29:57 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-04-07 15:58:31 -0400, Tom Lane wrote:
> >> There's an assumption that if you are asking to pin a buffer, the tuple
> >> pointer you're passing is pointing into that buffer (and is therefore not
> >> something that could be pfree'd).  If it isn't pointing into a buffer,
> >> the tuple slot is not the place to be keeping the buffer reference.
>
> > Yea. I *have* a HeapTupleData struct pointing into the buffer. It's just
> > that the lifetime of the indexscans's xs_ctup isn't sufficient for my
> > case. So I have to allocate a new HeapTupleData struct, *again* pointing
> > into the buffer. I'd like to manage the lifetime of that HeapTupleData
> > struct (*not* the entire HeapTupleHeader blob) via the slot.
> > That doesn't sound too crazy to me?
>
> In that case you should have another tuple slot of your own and let it
> keep the tuple (and buffer pin).

Maybe I am just being dense here and misunderstanding tuple slots. The
executor isn't exactly my strong suit.

But say I am doing something like:

slot = ExecInitExtraTupleSlot(estate);
ExecSetSlotDescriptor(slot, RelationGetDescr(rel));
...
while ((scantuple = index_getnext(scan, ForwardScanDirection)) != NULL)
{    /* some check */    ...    if (blurb)        ExecStoreTuple(scantuple, slot, scan->xs_cbuf, true);
}

...
index_endscan(scan); /* possibly */

/* now use the stored tuple via slot->tts_tuple */

that's not going to work, because scantuple might be free'd or pointing
to another tuple, from the next index_getnext() call. Right?
So what I now do is essentially:
while ((scantuple = index_getnext(scan, ForwardScanDirection)) != NULL)
{
...       ht = palloc(sizeof(HeapTupleData)); /* in the right context */       memcpy(ht, scantuple,
sizeof(HeapTupleData));      ExecStoreTuple(ht, slot, scan->xs_cbuf, false);       slot->tts_shouldFree = true;
 
...
}

That will a) keep the buffer pinned long enough, b) keep the
HeapTupleData struct around long enough.

That works, but seems pretty ugly. Thus I am wondering if a) there's a
way to avoid the outside manipulation of tts_shouldFree b) why there's
no builtin operation doing the palloc, memcpy and store...

Greetings,

Andres Freund

--Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-04-07 16:29:57 -0400, Tom Lane wrote:
>> In that case you should have another tuple slot of your own and let it
>> keep the tuple (and buffer pin).

> that's not going to work, because scantuple might be free'd or pointing
> to another tuple, from the next index_getnext() call. Right?

It's true that scantuple is probably pointing at the xs_ctup field of the
IndexScanDesc, so you need to put that data somewhere else if you want
to hold onto it past the current loop iteration.

> So what I now do is essentially:
> while ((scantuple = index_getnext(scan, ForwardScanDirection)) != NULL)
> {
> ...
>         ht = palloc(sizeof(HeapTupleData)); /* in the right context */
>         memcpy(ht, scantuple, sizeof(HeapTupleData));
>         ExecStoreTuple(ht, slot, scan->xs_cbuf, false);
>         slot->tts_shouldFree = true;
> ...
> }

Well, that is certainly messy.  I think you could just use a local
HeapTupleData variable instead of palloc'ing every time, where "local"
means "has lifespan similar to the slot pointer".  That is
     HeapTupleData my_htup;
     while ((scantuple = index_getnext(scan, ForwardScanDirection)) != NULL)     {    memcpy(&my_htup, scantuple,
sizeof(HeapTupleData));   ExecStoreTuple(&my_htup, slot, scan->xs_cbuf, false);     }
 

If my_htup is just a local, you'd want to clear the slot before my_htup
goes out of scope, just to be sure it doesn't try to dereference a
dangling pointer.  There's some vaguely similar hacking near the end of
ExecDelete.
        regards, tom lane

On 2014-04-07 21:47:38 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > So what I now do is essentially:
> > while ((scantuple = index_getnext(scan, ForwardScanDirection)) != NULL)
> > {
> > ...
> >         ht = palloc(sizeof(HeapTupleData)); /* in the right context */
> >         memcpy(ht, scantuple, sizeof(HeapTupleData));
> >         ExecStoreTuple(ht, slot, scan->xs_cbuf, false);
> >         slot->tts_shouldFree = true;
> > ...
> > }
> 
> Well, that is certainly messy.  I think you could just use a local
> HeapTupleData variable instead of palloc'ing every time, where "local"
> means "has lifespan similar to the slot pointer".

Doesn't work nicely in this specific situation, but it's obviously an
alternative.

> There's some vaguely similar hacking near the end of ExecDelete.

Yea, and some other places. I wonder if a ExecShallowMaterializeSlot()
or something would be useful for me, that callsite and others?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-04-07 21:47:38 -0400, Tom Lane wrote:
>> Well, that is certainly messy.  I think you could just use a local
>> HeapTupleData variable instead of palloc'ing every time, where "local"
>> means "has lifespan similar to the slot pointer".

>> There's some vaguely similar hacking near the end of ExecDelete.

> Yea, and some other places. I wonder if a ExecShallowMaterializeSlot()
> or something would be useful for me, that callsite and others?

Don't like that name much, but I agree there's some room for a function
like this.  I guess you're imagining that we'd add a HeapTupleData field
to TupleTableSlots, and use that for the workspace when this situation
arises?

An alternative possibility would be to not invent a new function, but
just make ExecStoreTuple do this unconditionally when shouldFree=false.
Not sure if there'd be a noticeable runtime penalty --- but the
existing approach seems rather fragile.  I know I've always thought
of slots as being fully independent storage, and in this case they
are not.
        regards, tom lane

On 2014-04-08 09:37:33 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-04-07 21:47:38 -0400, Tom Lane wrote:
> >> Well, that is certainly messy.  I think you could just use a local
> >> HeapTupleData variable instead of palloc'ing every time, where "local"
> >> means "has lifespan similar to the slot pointer".
> 
> >> There's some vaguely similar hacking near the end of ExecDelete.
> 
> > Yea, and some other places. I wonder if a ExecShallowMaterializeSlot()
> > or something would be useful for me, that callsite and others?
> 
> Don't like that name much, but I agree there's some room for a function
> like this.

I am not the biggest fan either, for one it's really rather long, for
another it's not really descriptive. One might think it's about toast or
something. Do you have a better name?

> I guess you're imagining that we'd add a HeapTupleData field
> to TupleTableSlots, and use that for the workspace when this situation
> arises?

I wasn't really sure about the approach yet. Either do something like
you describe (possibly reusing/recoining tts_minhdr?), or just allocate
a HeapTupleData struct.

> An alternative possibility would be to not invent a new function, but
> just make ExecStoreTuple do this unconditionally when shouldFree=false.
> Not sure if there'd be a noticeable runtime penalty.

I think that's a viable alternative.

> I know I've always thought
> of slots as being fully independent storage, and in this case they
> are not.

Me to. I was initially rather confused by the memory corruptions I was
seeing. I really thought that storing a tuple pointing to a buffer in
the slot should "just work".

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services