Thread: Should a plan node's result tuple slot be read-only to caller?
I looked into Frank van Vugt's recent report of bizarre behavior in 8.1: http://archives.postgresql.org/pgsql-bugs/2005-11/msg00121.php The problem occurs because execMain.c's ExecInsert() replaces the contents of the TupleTableSlot passed to it with whatever the trigger hands back. This slot is the result slot of the top-level plan node, which in the case at hand is a Unique node. In 8.0 and before, this does not result in a failure, because Unique is keeping a separate copy of its last output tuple to compare to; the 8.0 code comments: /* * We have a new tuple different from the previous saved tuple (if * any). Save it and return it. We must copyit because the source * subplan won't guarantee that this source tuple is still accessible * after fetching thenext source tuple. * * Note that we manage the copy ourselves. We can't rely on the result * tuple slot tomaintain the tuple reference because our caller may * replace the slot contents with a different tuple. We assume that * the caller will no longer be interested in the current tuple after * he next calls us. * * tgl 3/2004:the above concern is no longer valid; junkfilters used to * modify their input's return slot but don't anymore,and I don't * think anyplace else does either. Not worth changing this code * though. */ In connection with the "virtual tuple slot" optimization added for 8.1, I rewrote this code and got rid of the supposedly-redundant extra tuple copy. nodeUnique is now comparing the next input tuple directly to the contents of its result slot, and so it gets fooled when the caller changes that slot. (IOW, the comment I added in 3/2004 was wrong...) The minimum-change way to fix the bug would be to revert the logic change in nodeUnique.c and go back to maintaining a separate tuple copy. But I'm thinking that this sort of thing could happen again. ISTM it's not intuitive to allow a plan node's caller to scribble on the plan node's result slot. An alternative solution would be to require execMain.c to keep an extra tuple table slot that has no other purpose than to temporarily hold replacement tuples during ExecInsert and ExecUpdate. I'm leaning towards the extra-slot approach, but wondered if anyone has comments or better ideas. regards, tom lane
Re: Should a plan node's result tuple slot be read-only to caller?
From
Martijn van Oosterhout
Date:
On Mon, Nov 14, 2005 at 10:14:53AM -0500, Tom Lane wrote: > The minimum-change way to fix the bug would be to revert the logic > change in nodeUnique.c and go back to maintaining a separate tuple copy. > But I'm thinking that this sort of thing could happen again. ISTM > it's not intuitive to allow a plan node's caller to scribble on the plan > node's result slot. An alternative solution would be to require > execMain.c to keep an extra tuple table slot that has no other purpose > than to temporarily hold replacement tuples during ExecInsert and > ExecUpdate. I agree that execMain should play with its own memory. The rule that a node's result slot is valid until the next call is good because it solves the memory management issue. By allowing other people to scribble over your slot may cause issues w.r.t. knowing when you can safely free it. From a modularity point of view, nodes own their tupleslots and should be able to rely on them not changing... Hope this helps, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.