On Sun, Jul 19, 2009 at 12:39 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I think this is a great feature, and it would be REALLY great if it
>> supported UPDATE and DELETE as well.
>
> It won't get applied until it does, and I imagine the patch author
> wasn't expecting any differently. The submission was clearly marked
> "WIP" not "ready to apply".
>
>> However, it sounds to me like this is going to need more reworking
>> than is going to get done in the next week or two.
>
> Yeah. I did a quick scan of the patch and was distressed at how much of
> it seemed to be addition of new code; that implies that he's more or
> less duplicated the top-level executor processing.
>
> The way that I think this should be approached is
> (1) a code-refactoring patch that moves INSERT/UPDATE/DELETE control
> into plan nodes; then
> (2) a feature patch that makes use of that to expose RETURNING in CTEs.
>
> One thing that's not totally clear to me is whether we'd like to use
> control plan nodes all the time, or only for statements with RETURNING.
> The nice thing about the latter approach is that there's a well-defined
> meaning for the plan node's targetlist. (Its input is the stuff to be
> stored, its output is the RETURNING result; much cleaner than the way
> RETURNING is bolted onto the executor now.) If we do it all the time
> then the control nodes would have dummy targetlists for non-RETURNING
> cases, which is a little bit ugly. OTOH it may not be practical to
> handle it like that without a lot of code duplication.
>
> Another thing to think about is whether there should be three types
> of control nodes or only one.
>
> But anyway, my $0.02 is that the *first* order of business is to propose
> a suitable refactoring of execMain.c. We skipped doing that when we
> first added RETURNING, but it's time to make it happen. Not fasten
> another kluge on top of a kluge.
Tom,
This is a great review and great input. I wish there were enough
hours in the day for you to do something like this for every patch. I
feel good about saying that this is Returned With Feedback at this
point, and I see that Jaime Casanova has already made that update.
Thanks very much,
...Robert