Re: Using results from INSERT ... RETURNING

On Tue, Sep 22, 2009 at 11:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Right now, it looks like most of the code is being shared between all
>> three plan types.  I'm pretty suspicious of how much code this patch
>> moves around and how little of it is actually changed.  I can't really
>> tell if there's an actual design improvement here or if this is all
>> window-dressing.
>
> My recollection is that we *told* Marko to set things up so that the
> first patch was mainly just code refactoring.  So your second sentence
> doesn't surprise me.  As to the third, I've not looked at the patch,
> but perhaps it needs to expend more effort on documentation?

Well, part of the problem is that I've not had a lot of luck trying to
understand how the executor really works (what's a tuple table slot
and why do we need to know in advance how many of them there are?).
There's this fine comment, which has been in
src/backend/executor/README for 8 years and change:

XXX a great deal more documentation needs to be written here...

However, that's not the whole problem, either. To your point about
documentation, it seems this path doesn't touch the README at all, and
it needs to, because some of the statements in that file would
certainly become false were this patch to be applied.

So I think we should at a minimum ask the patch author to (1) fix the
explain bugs I found and (2) update the README, as well as (3) revert
needless whitespace changes - there are a couple in execMain.c, from
the looks of it.

However, before he spends too much more time on this feature, it would
probably be good for you to take a quick scan through the patch and
see what you think of the general approach. I don't think I'm
qualified to judge.

...Robert