pg.ddx.io pgsql-admin@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff
5+ messages / 2 participants
[nested] [flat]
* Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff
@ 2015-04-27 14:28 Andres Freund <andres@anarazel.de>
2015-04-27 21:52 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 5+ messages in thread
From: Andres Freund @ 2015-04-27 14:28 UTC (permalink / raw)
To: Peter Geoghegan <pg@heroku.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: hlinnaka <hlinnaka@iki.fi>; Jeff Janes <jeff.janes@gmail.com>; Stephen Frost <sfrost@snowman.net>; Dean Rasheed <dean.a.rasheed@gmail.com>; Bruce Momjian <bruce@momjian.us>; Pg Hackers <pgsql-hackers@postgresql.org>
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
> * So far, there has been a lack of scrutiny about what the patch does
> in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
> expression) and optimizer (the whole concept of an "auxiliary"
> query/plan that share a target RTE, and later target ResultRelation).
> If someone took a close look at that, it would be most helpful.
> ruleutils.c is also modified for the benefit of EXPLAIN output. This
> all applies only to the ON CONFLICT UPDATE patch. A committer could
> push out the IGNORE patch before this was 100% firm.
I'm far from an expert on the relevant regions; but I'm starting to look
nonetheless. I have to say that on a preliminary look it looks more
complicated than it has to.
Tom, any chance you could have a look at the parse->analsys->executor
transformations?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff
2015-04-27 14:28 Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
@ 2015-04-27 21:52 ` Andres Freund <andres@anarazel.de>
2015-04-27 22:28 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Peter Geoghegan <pg@heroku.com>
2015-04-28 10:38 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 5+ messages in thread
From: Andres Freund @ 2015-04-27 21:52 UTC (permalink / raw)
To: Peter Geoghegan <pg@heroku.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: hlinnaka <hlinnaka@iki.fi>; Jeff Janes <jeff.janes@gmail.com>; Stephen Frost <sfrost@snowman.net>; Dean Rasheed <dean.a.rasheed@gmail.com>; Bruce Momjian <bruce@momjian.us>; Pg Hackers <pgsql-hackers@postgresql.org>
On 2015-04-27 16:28:49 +0200, Andres Freund wrote:
> On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
> > * So far, there has been a lack of scrutiny about what the patch does
> > in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
> > expression) and optimizer (the whole concept of an "auxiliary"
> > query/plan that share a target RTE, and later target ResultRelation).
> > If someone took a close look at that, it would be most helpful.
> > ruleutils.c is also modified for the benefit of EXPLAIN output. This
> > all applies only to the ON CONFLICT UPDATE patch. A committer could
> > push out the IGNORE patch before this was 100% firm.
>
> I'm far from an expert on the relevant regions; but I'm starting to look
> nonetheless. I have to say that on a preliminary look it looks more
> complicated than it has to.
So, I'm looking. And I've a few questions:
* Why do we need to spread knowledge about speculative inserts that wide?
It's now in 1) Query, 2) ParseState 3) ModifyTable 4) InsertStmt. That
seems a bit wide - and as far as I see not really necessary. That e.g.
transformUpdateStmt() has if (!pstate->p_is_speculative) blocks seems
wrong.
* afaics 'auxiliary query' isn't something we have under that
name. This isn't really different to what wCTEs do, so I don't think
we need this term here.
* You refer to 'join like syntax' in a couple places. I don't really see
the current syntax being that join like? Is that just a different
perception of terminology or is that just remnants of earlier
approaches?
* I am rather unconvinced we need the whole 'ExcludedVar' mechanism. I
don't really see why we need it at all? Can't we 'just' make those
plain vars referring to the normal targetlist "entry"? I feel like I'm
missing something here.
* The whole dealing with the update clause doesn't seem super
clean. I'm not sure yet how to do it nicer though :(
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff
2015-04-27 14:28 Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
2015-04-27 21:52 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
@ 2015-04-27 22:28 ` Peter Geoghegan <pg@heroku.com>
1 sibling, 0 replies; 5+ messages in thread
From: Peter Geoghegan @ 2015-04-27 22:28 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; hlinnaka <hlinnaka@iki.fi>; Jeff Janes <jeff.janes@gmail.com>; Stephen Frost <sfrost@snowman.net>; Dean Rasheed <dean.a.rasheed@gmail.com>; Bruce Momjian <bruce@momjian.us>; Pg Hackers <pgsql-hackers@postgresql.org>
On Mon, Apr 27, 2015 at 2:52 PM, Andres Freund <andres@anarazel.de> wrote:
> So, I'm looking. And I've a few questions:
> * Why do we need to spread knowledge about speculative inserts that wide?
> It's now in 1) Query, 2) ParseState 3) ModifyTable 4) InsertStmt. That
> seems a bit wide - and as far as I see not really necessary. That e.g.
> transformUpdateStmt() has if (!pstate->p_is_speculative) blocks seems
> wrong.
Maybe it shouldn't be spelt "p_is_speculative", because speculative
insertion is a low-level mechanism. Maybe it should be something like
p_is_auxiliary...but you don't like that word either. :-)
If setTargetTable() took the RangeVar NULL-ness as a proxy for a child
auxiliary query, and if free_parsestate() took a flag to indicate that
it shouldn't perform a heap_close() when an auxiliary statement's
parent must do it instead, then it wouldn't be nescessary to add a
p_is_speculative field. But that seems worse.
> * afaics 'auxiliary query' isn't something we have under that
> name. This isn't really different to what wCTEs do, so I don't think
> we need this term here.
Well, I guess auxiliary is just a descriptive word, as opposed to one
that describes a technical mechanism peculiar to Postgres/this patch.
I don't feel that it's important either way.
> * You refer to 'join like syntax' in a couple places. I don't really see
> the current syntax being that join like? Is that just a different
> perception of terminology or is that just remnants of earlier
> approaches?
No. It is join-like. It looks the same as and behaves similarly to
UPDATE .... FROM ... . There is one difference, though -- excluded.*
is not visible from RETURNING (because of the ambiguity of doing it
the UPDATE way, where, as with a self-join, both tuples have identical
column names. And because the INSERT RETURNING behavior should be
dominant). That's all I mean by "join-like". People thought that
looked nicer than a straight expression that operates on a Var (which
FWIW is kind of what MySQL does), so that's the way it works.
> * I am rather unconvinced we need the whole 'ExcludedVar' mechanism. I
> don't really see why we need it at all? Can't we 'just' make those
> plain vars referring to the normal targetlist "entry"? I feel like I'm
> missing something here.
If you allow multiple RTEs by not having the rewriter swap out
EXCLUDED vars for ExcludedExpr(), what you end up with is an UPDATE
subquery with a join (and, if you change nothing else in the patch, a
segfault). Fixing that segfault is probably going to require more
complexity in the optimizer, and certainly will require more in the
executor. Imagine teaching ModifyTable nodes about rescan to make a
row lock conflict handled from its parent (if we had a "real" join).
Alternatively, maybe you could have the EPQ stuff install a tuple and
then execute a single EvalPlanQualNext() against a scantuple (which
you'd also have to install using EPQ). You can install multiple
tuples, which is used for SELECT FOR UPDATE's EPQ stuff. But that
seems very significantly more complicated than what I actually did.
Think of ExcludedExpr as a way of pretending that an expression on the
target's Vars is a Var referencing a distinct RTE, simply because
people think that looks nicer. The EXCLUDED.* tuple legitimately
originates form the same tuple context as the target tuple, which the
structure of the code reflects.
> * The whole dealing with the update clause doesn't seem super
> clean. I'm not sure yet how to do it nicer though :(
At least it isn't all that much code. I think that the main thing that
this design has to recommend it is code re-use. The patch actually
isn't all that big in terms of lines of code.
--
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff
2015-04-27 14:28 Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
2015-04-27 21:52 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
@ 2015-04-28 10:38 ` Andres Freund <andres@anarazel.de>
2015-04-28 17:31 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Peter Geoghegan <pg@heroku.com>
1 sibling, 1 reply; 5+ messages in thread
From: Andres Freund @ 2015-04-28 10:38 UTC (permalink / raw)
To: Peter Geoghegan <pg@heroku.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: hlinnaka <hlinnaka@iki.fi>; Jeff Janes <jeff.janes@gmail.com>; Stephen Frost <sfrost@snowman.net>; Dean Rasheed <dean.a.rasheed@gmail.com>; Bruce Momjian <bruce@momjian.us>; Pg Hackers <pgsql-hackers@postgresql.org>
On 2015-04-27 23:52:58 +0200, Andres Freund wrote:
> On 2015-04-27 16:28:49 +0200, Andres Freund wrote:
> > On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
> > > * So far, there has been a lack of scrutiny about what the patch does
> > > in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
> > > expression) and optimizer (the whole concept of an "auxiliary"
> > > query/plan that share a target RTE, and later target ResultRelation).
> > > If someone took a close look at that, it would be most helpful.
> > > ruleutils.c is also modified for the benefit of EXPLAIN output. This
> > > all applies only to the ON CONFLICT UPDATE patch. A committer could
> > > push out the IGNORE patch before this was 100% firm.
> >
> > I'm far from an expert on the relevant regions; but I'm starting to look
> > nonetheless. I have to say that on a preliminary look it looks more
> > complicated than it has to.
>
> So, I'm looking. And I've a few questions:
> * Why do we need to spread knowledge about speculative inserts that wide?
> It's now in 1) Query, 2) ParseState 3) ModifyTable 4) InsertStmt. That
> seems a bit wide - and as far as I see not really necessary. That e.g.
> transformUpdateStmt() has if (!pstate->p_is_speculative) blocks seems
> wrong.
>
> * afaics 'auxiliary query' isn't something we have under that
> name. This isn't really different to what wCTEs do, so I don't think
> we need this term here.
>
> * You refer to 'join like syntax' in a couple places. I don't really see
> the current syntax being that join like? Is that just a different
> perception of terminology or is that just remnants of earlier
> approaches?
>
> * I am rather unconvinced we need the whole 'ExcludedVar' mechanism. I
> don't really see why we need it at all? Can't we 'just' make those
> plain vars referring to the normal targetlist "entry"? I feel like I'm
> missing something here.
>
> * The whole dealing with the update clause doesn't seem super
> clean. I'm not sure yet how to do it nicer though :(
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.
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. 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.
I'm actually even wondering if it'd not better off *not* to reuse
ExecUpdate(), but that's essentially a separate concern.
I'll try to rejigger things roughly in that directions. If you haven't
heard of me in three days, call the police.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff
2015-04-27 14:28 Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
2015-04-27 21:52 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
2015-04-28 10:38 ` Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
@ 2015-04-28 17:31 ` Peter Geoghegan <pg@heroku.com>
0 siblings, 0 replies; 5+ messages in thread
From: Peter Geoghegan @ 2015-04-28 17:31 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; hlinnaka <hlinnaka@iki.fi>; Jeff Janes <jeff.janes@gmail.com>; Stephen Frost <sfrost@snowman.net>; Dean Rasheed <dean.a.rasheed@gmail.com>; Bruce Momjian <bruce@momjian.us>; Pg Hackers <pgsql-hackers@postgresql.org>
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2015-04-28 17:31 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2015-04-27 14:28 Re: INSERT ... ON CONFLICT UPDATE/IGNORE 4.0, parser/executor stuff Andres Freund <andres@anarazel.de>
2015-04-27 21:52 ` Andres Freund <andres@anarazel.de>
2015-04-27 22:28 ` Peter Geoghegan <pg@heroku.com>
2015-04-28 10:38 ` Andres Freund <andres@anarazel.de>
2015-04-28 17:31 ` Peter Geoghegan <pg@heroku.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox