On Tue, Apr 28, 2015 at 3:38 AM, Andres Freund <andres@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