On Sun, 2005-05-15 at 15:09 -0400, Tom Lane wrote:
> I'm planning to change ExecRestrPos and the routines it calls so that
> an updated TupleTableSlot holding the restored-to tuple is explicitly
> returned.
>
> Currently, since nothing is explicitly done to the result Slot of a
> plan node when we restore its position, you might think that the Slot
> still points at the tuple that was current just before the Restore.
> You'd be wrong though, at least for seqscan and indexscan plans
> (I haven't looked yet at the other node types that support
> mark/restore). The reason is that the restore operation changes
> the contents of a HeapTupleData struct in the scan state (rs_ctup
> or xs_ctup) and all that the Slot really contains is a pointer to
> that struct.
>
> Now this is really bad. In the first place, the Slot thinks it
> has a pin on the buffer containing its current tuple. After a
> Restore, it may have pin on the wrong buffer.
Sounds terrible. Is this a particular bug you're fixing?
> It seems to be
> sheer chance that we've not had bugs due to this.
It isn't a very common case, thats why. You'd need to have value
duplicates in both joined columns, which is effectively a product join.
Granted it is syntactically allowable SQL.
AFAICS all joins should be between relations 1:1 or 1:M. A direct M:M
join is actually missing out the associative relation, or a non-key self
join. Such a join would rarely if ever have any correct and real
meaning. I can think of a few, but mostly its just incorrectly coded
SQL, or use of special values (e.g. blanks) rather than NULLs.
So my guess is that ExecRestrPos is almost never called.
> (The underlying
> scan does have pin on the right buffer, but one can easily imagine
> sequences in which the scan could be cleared while the Slot is
> still assumed valid.) As of CVS tip the consequences could be
> even worse, because the Slot may contain some pointers to extracted
> fields of the tuple, and these pointers are now out of sync with
> the tuple that the Slot really contains.
>
> So I think that it's essential that we explicitly update the scan
> result Slot during ExecRestrPos.
Yeh, no problem.
Best Regards, Simon Riggs
