Robert Haas <robertmhaas@gmail.com> writes:
> I find the way we program with slots to be unintuitive and, indeed,
> outright confusing. Your comment here is about making sure that the
> slot to which newslot pointed on entry to the function ends up with
> the correct contents, which is fair enough. But the naive reader could
> be forgiven for thinking that epqslot_clean =
> ExecGetUpdateNewTuple(relinfo, epqslot_candidate, oldslot) doesn't
> affect the validity of newslot, and it definitely does. It seems like
> ExecBRUpdateTriggers critically depends on the fact that on entry,
> oldslot is fully valid, whereas newslot may depend on oldslot ... but
> that isn't clearly documented.
Hm? oldslot doesn't even exist on entry, it's set up within the
function. The present bug existed because the result of that
ExecGetUpdateNewTuple call depended on oldslot, which is something
that ought not surprise the reader all that much. We just neglected
to follow out the consequences of that.
> Nor is the fact that the original
> newslot must itself get updated for the benefit of the caller. There's
> no header comment explaining what's supposed to be true on entry and
> on exit -- you just have to know how it's supposed to work. And it
> seems to me that we have a ton of slot-related code that's like that.
I agree that ExecBRUpdateTriggers is woefully underdocumented, but
I don't find that that's particularly associated with its use of
slots. I thought about adding a header comment as part of this patch,
but desisted because really three-quarters of trigger.c needs work
that way and it didn't seem like bug-fix material.
ISTM the really problematic thing here is just that the same slot
is being used for more than one purpose, in that ExecGetUpdateNewTuple
returns its result in the same slot no matter where it's called from.
That doesn't look to be trivial to change, but maybe we should look
for a way. In the meantime, the comments in ExecBRUpdateTriggers do
point out that hazard.
> You might think for example that each executor node type would have
> comments explaining what slots it used, what tuple descriptor each one
> uses and why, and the anticipated lifetime of each slot.
There's a lot of stuff that's pretty woefully underdocumented, and
we very frequently accept patches that make things worse not better.
But again, I don't buy that tuple slots are worse than anywhere else.
regards, tom lane
