Обсуждение: TupleTableSlot API problem

In CVS HEAD, try this in an assert-enabled build:

CREATE TEMPORARY TABLE tree(   id INTEGER PRIMARY KEY,                                           parent_id INTEGER
REFERENCEStree(id)
 
);

INSERT INTO tree
VALUES (1, NULL), (2, 1), (3,1), (4,2), (5,2), (6,2), (7,3), (8,3),      (9,4), (10,4), (11,7), (12,7), (13,7), (14,
9),(15,11), (16,11);
 

WITH RECURSIVE t(id, path) AS (   VALUES(1,ARRAY[]::integer[])
UNION ALL   SELECT tree.id, t.path || tree.id   FROM tree JOIN t ON (tree.parent_id = t.id)
)
SELECT t1.id, t2.path, t2 FROM t AS t1 JOIN t AS t2 ON
(t1.id=t2.id);

I get
ERROR:  cache lookup failed for type 2139062143
(the error might be less predictable in a non-assert build).

What is happening is that ExecProject fetches the Datum value of t2.path
from a TupleTableSlot that contains a "minimal tuple" fetched from the
tuplestore associated with the CTE "t".  Then, it fetches the value of
the whole-row variable t2.  ExecEvalWholeRowVar calls
ExecFetchSlotTuple, which finds that the slot doesn't contain a regular
tuple, so it calls ExecMaterializeSlot, which replaces the minimal tuple
with a regular tuple and frees the former.  Now the already-fetched
Datum for t2.path is pointing at freed storage.

In principle there ought to be a whole lot of bugs around this area,
because ExecFetchSlotTuple, ExecFetchSlotMinimalTuple, and
ExecFetchSlotTupleDatum all are potentially destructive of the original
slot contents; furthermore there ought be be visible bugs in 8.3 and
maybe before.  However, in an hour or so of poking at it, I've been
unable to produce a failure without using CTE syntax; it's just really
hard to get the planner to generate a whole-row-var reference in a
context where the referenced slot might contain a minimal tuple.
Still, I suspect that merely shows I'm being insufficiently creative
today.  I think we ought to assume there's a related bug in existing
branches unless someone can prove there's not.

One solution to this problem is to redefine these functions as always
returning locally palloc'd tuples, so that they can be guaranteed to not
modify the contents of the Slot.  That's fairly unfortunate though since
in the vast majority of cases it will result in a palloc/pfree cycle
that is not necessary.  It would also mean changing most of the callers
to pfree the results, unless we can tolerate memory leaks there.

Plan B is to agree that these functions should be documented as
potentially destructive of the slot contents, in which case this is just
a bug in ExecEvalWholeRowVar: it should be using a call that is
nondestructive (eg ExecCopySlotTuple).  (So far as I can determine from
looking at the callers, only ExecEvalWholeRowVar and its sibling
ExecEvalWholeRowSlow actually pose a risk; no other call sites look like
there's any risk of having other existing references into the slot
contents.)

Plan C is to change the Slot logic so that a slot can store both regular
and minimal tuples, not just one or the other.  It would only actually
do that if one format is stored into it and then the other is requested.
The eventual ExecClearTuple would then free both copies.  Whichever
format is not the one originally stored is secondary and isn't used for
fetching individual datums from the Slot.  This nets out at the same
performance we have now, it just postpones the pfree() that currently
happens immediately when we change the slot's format.  It might result
in higher peak memory use but I'm not exceedingly worried about that.

Plan C is probably the most complicated to code, but I'm inclined to try
to fix it that way, because plan A loses performance and plan B exposes
us to a continuing risk of future bugs of the same type.

Comments, better ideas?
        regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> What is happening is that ExecProject fetches the Datum value ofTom> t2.path from a TupleTableSlot that contains a
"minimaltuple"Tom> fetched from the tuplestore associated with the CTE "t".  Then,Tom> it fetches the value of the
whole-rowvariable t2.Tom> ExecEvalWholeRowVar calls ExecFetchSlotTuple, which finds thatTom> the slot doesn't contain a
regulartuple, so it callsTom> ExecMaterializeSlot, which replaces the minimal tuple with aTom> regular tuple and frees
theformer.  Now the already-fetchedTom> Datum for t2.path is pointing at freed storage.
 
Tom> In principle there ought to be a whole lot of bugs around thisTom> area, because ExecFetchSlotTuple,
ExecFetchSlotMinimalTuple,andTom> ExecFetchSlotTupleDatum all are potentially destructive of theTom> original slot
contents;furthermore there ought be be visibleTom> bugs in 8.3 and maybe before.  However, in an hour or so ofTom>
pokingat it, I've been unable to produce a failure withoutTom> using CTE syntax; it's just really hard to get the
plannertoTom> generate a whole-row-var reference in a context where theTom> referenced slot might contain a minimal
tuple.

Generating the whole-row-var reference doesn't seem to be hard, it's
doing it in a context where slot->tts_shouldFree is _already_ set that
seems to be the issue.

For example, given some function foo(out a text, out b text) returning
setof record, the query select t.a, t from foo() t; follows the
sequence of events you describe, but it doesn't fail because
slot->tts_shouldFree is false, so the original minimaltuple isn't
slot->freed.

If there aren't any code paths in the back branches that have
tts_shouldFree set in this context, that would explain the lack of
previously visible bugs, no? Or am I completely misunderstanding it?

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> In principle there ought to be a whole lot of bugs around this
>  Tom> area, because ExecFetchSlotTuple, ExecFetchSlotMinimalTuple, and
>  Tom> ExecFetchSlotTupleDatum all are potentially destructive of the
>  Tom> original slot contents; furthermore there ought be be visible
>  Tom> bugs in 8.3 and maybe before.  However, in an hour or so of
>  Tom> poking at it, I've been unable to produce a failure without
>  Tom> using CTE syntax; it's just really hard to get the planner to
>  Tom> generate a whole-row-var reference in a context where the
>  Tom> referenced slot might contain a minimal tuple.

> Generating the whole-row-var reference doesn't seem to be hard, it's
> doing it in a context where slot->tts_shouldFree is _already_ set that
> seems to be the issue.

> For example, given some function foo(out a text, out b text) returning
> setof record, the query select t.a, t from foo() t; follows the
> sequence of events you describe, but it doesn't fail because
> slot->tts_shouldFree is false, so the original minimaltuple isn't
> slot->freed.

Yeah, good point.  However I think that you could still get a failure.
The cases where a slot might contain a minimal tuple are generally where
we are reading out of a tuplestore or tuplesort object, and all you have
to do to get it to be a palloc'd mintuple is to make the test case big
enough so the tuplestore has dumped to disk.  (Now that I think about
it, I failed to try scaling up the test cases I did try...)
        regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> For example, given some function foo(out a text, out b text) returning>> setof record, the query select t.a, t from
foo()t; follows the>> sequence of events you describe, but it doesn't fail because>> slot-> tts_shouldFree is false, so
theoriginal minimaltuple isn't>> freed.
 
Tom> Yeah, good point.  However I think that you could still get aTom> failure.  The cases where a slot might contain a
minimaltupleTom> are generally where we are reading out of a tuplestore orTom> tuplesort object, and all you have to do
toget it to be aTom> palloc'd mintuple is to make the test case big enough so theTom> tuplestore has dumped to disk.
(Nowthat I think about it, ITom> failed to try scaling up the test cases I did try...)
 

Aha; and indeed if you use select t.a, t from func() t; where the
function returns a set larger than work_mem, it does indeed fail
messily (against my -O0 --enable-cassert HEAD I just get corrupted
values for t.a, though, rather than an error). I'll try and reproduce
that on a back branch...

-- 
Andrew.

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> Yeah, good point.  However I think that you could still get aTom> failure.  The cases where a slot might contain a
minimaltupleTom> are generally where we are reading out of a tuplestore orTom> tuplesort object, and all you have to do
toget it to be aTom> palloc'd mintuple is to make the test case big enough so theTom> tuplestore has dumped to disk.
(Nowthat I think about it, ITom> failed to try scaling up the test cases I did try...)
 
Andrew> Aha; and indeed if you use select t.a, t from func() t; whereAndrew> the function returns a set larger than
work_mem,it doesAndrew> indeed fail messily (against my -O0 --enable-cassert HEAD IAndrew> just get corrupted values
fort.a, though, rather than anAndrew> error). I'll try and reproduce that on a back branch...
 

Yup, fails the same way on an --enable-cassert build of 8.3.7.

Without --enable-cassert it _appears_ to work, but this is presumably
just because the freed memory happens not to be being clobbered
immediately.

-- 
Andrew.

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Yup, fails the same way on an --enable-cassert build of 8.3.7.

Do you have a quick test case?  I just finished coding up my plan-C
fix, and I need some test cases ...
        regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:>> Yup, fails the same way on an --enable-cassert build of
8.3.7.
Tom> Do you have a quick test case?  I just finished coding up myTom> plan-C fix, and I need some test cases ...

This is the one I've been using:

create or replace function foo(n integer, out a text, out b text) returns setof record language plpgsql as $f$ begin
returnquery(select 'foo '||i, 'bar '||i from generate_series(1,n) i); end;
 
$f$;

set work_mem=64;

select t.a, t, t.a from foo(100000) t limit 1;                             a                               |         t
      |   a   
 

--------------------------------------------------------------+-------------------+-------\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F
|("foo 1","bar 1") | foo 1
 
(1 row)

-- 
Andrew.

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> Yup, fails the same way on an --enable-cassert build of 8.3.7.

And on 8.2.13.
Tom> Do you have a quick test case?  I just finished coding up myTom> plan-C fix, and I need some test cases ...
Andrew> This is the one I've been using:

This one is simpler and works on 8.2 as well:

create or replace function foo(n integer, out a text, out b text)returns setof record language sqlas $f$ select 'foo
'||i,'bar '||i from generate_series(1,$1) i; $f$;
 

set work_mem=64;

select t.a, t, t.a from foo(100000) t limit 1;
ERROR:  invalid memory alloc request size 2139062147

-- 
Andrew.

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> This one is simpler and works on 8.2 as well:

Yeah, I had just finished making the same adjustment to get an
8.2-compatible test case.  8.1 and before should be okay because
they haven't got the MinimalTuple business.
        regards, tom lane