On Tue, Apr 28, 2015 at 3:38 AM, Andres Freund <and...@anarazel.de> wrote: > The more I look at approach taken in the executor, the less I like it. > I think the fundamental structural problem is that you've chosen to > represent the ON CONFLICT UPDATE part as fully separate plan tree node; > planned nearly like a normal UPDATE. But it really isn't. That's what > then requires you to coordinate knowedge around with p_is_speculative, > have these auxiliary trees, have update nodes without a relation, and > other similar things.
The "update node without a relation" thing is misleading. There is an UpdateStmt parse node that briefly lacks a RangeVar, but it takes its target RTE from the parent without bothering with a RangeVar. From there on in, it has an RTE (shared with the parent), just as the executor has the two share their ResultRelation. It is a separate node - it's displayed in EXPLAIN output as a separate node. It's not the first type of node to have to supply its own instrumentation because of the way its executed. I don't know how you can say it isn't a separate plan node when it is displayed as such in EXPLAIN, and is separately instrumented as one. > My feeling is that this will look much less ugly if there's simply no > UpdateStmt built. I mean, all we need is the target list and the where > clause. Yes, that's all that is needed - most of the structure of a conventional UPDATE! There isn't much that you can't do that you can with a regular UPDATE. Where are you going to cut? > As far as I can see so far that'll get rid of a lot of > structural uglyness. There'll still be some more localized stuff, but I > don't think it'll be that bad. You're going to need a new targetlist just for this case. So you've just added a new field to save one within Query, etc. > I'm actually even wondering if it'd not better off *not* to reuse > ExecUpdate(), but that's essentially a separate concern. I think that makes no sense. ExecUpdate() has to do many things that are applicable to both. The *only* thing that it does that's superfluous for ON CONFLICT DO UPDATE is walking the update chain. That is literally the only thing. I think that you're uncomfortable with the way that the structure is different to anything that exists at the moment, which is understandable. But this is UPSERT - why would the representation of what is more or less a hybrid DML query type not have a novel new representation? What I've done works with most existing abstractions without too much extra code. The code reuse that this approach allows seems like a major advantage. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers