From: | Peter Geoghegan <pg(at)heroku(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, hlinnaka <hlinnaka(at)iki(dot)fi>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff |
Date: | 2015-04-28 17:31:55 |
Message-ID: | CAM3SWZSVQN9GFedktvr0kG0Bn8DsgQ0dY0EuUWn696gVga9bZQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-admin pgsql-hackers |
On Tue, Apr 28, 2015 at 3:38 AM, Andres Freund <andres(at)anarazel(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Ankur Kaushik | 2015-04-29 09:43:07 | Adding Column in Specific Position of table |
Previous Message | Alvaro Herrera | 2015-04-28 15:13:05 | Re: TR: Re: Postgres and multiprocessor? |
From | Date | Subject | |
---|---|---|---|
Next Message | David G. Johnston | 2015-04-28 17:36:07 | Re: INSERT ... ON CONFLICT syntax issues |
Previous Message | Peter Geoghegan | 2015-04-28 17:06:07 | Re: INSERT ... ON CONFLICT syntax issues |