On Thu, Feb 1, 2018 at 4:45 AM, Simon Riggs <si...@2ndquadrant.com> wrote: > I think it would be very helpful if we could discuss everything with > direct relevance to v14, so this becomes a patch review, not just a > debate.
I wish I could give you a clear answer on the way forward with total confidence, or even moderate confidence, but right now I can't. Hopefully I'll be able to convince myself that I've understood all the nuances of this EPQ + WHEN ... AND qual business shortly, perhaps in the next couple of days, at which point I'll have a position that I'm willing to defend. I don't even have that much right now. On a more positive note, I do agree with you that this only affects a small subset of real-world queries (even if we assume READ COMMITTED conflicts are common). To put it another way, I would be able to give you a simple opinion right now if WHEN ... AND quals were prohibited. Not that I'm proposing actually prohibiting them. (BTW, don't you think it's interesting that Oracle doesn't allow them, but instead requires WHERE clauses, even on an INSERT?) Leaving concurrency aside for a moment, I want to talk about bugs. I read through this: https://wiki.postgresql.org/wiki/SQL_MERGE_Patch_Status#Bugs This suggestion you make, which is that the only problem with wholerow vars is that there is a weird error message (and they're actually unsupported) is not helping your cause. While I think that users will expect that to work, that isn't really the main point. I tried using wholerow vars as a smoke test for setrefs.c handling, as well as the handling of targetlists within the rewriter. The fact that wholerow vars don't work is likely indicative of your general approach in these areas needing to be thought through in more detail. That's the really important thing. Wholerow vars shouldn't be a special case to the underlying implementation, and if you found a way to make them work that was a special case that would seem questionable to me for similar reasons. Sometimes wholerow vars actually *do* work with your patch -- the problem only happens with data_source whole-row vars, and never target_table_name wholerow vars: postgres=# merge into testoids a using (select i "key", 'foo' "data" from generate_series(0,3) i) b on a.key = b.key when matched then update set data = a.*::text when not matched then insert (key, data) values (b.key, 'foo'); MERGE 4 postgres=# merge into testoids a using (select i "key", 'foo' "data" from generate_series(0,3) i) b on a.key = b.key when matched then update set data = b.*::text when not matched then insert (key, data) values (b.key, 'foo'); ERROR: variable not found in subplan target lists There is also the matter of subselects in the update targetlist, which you similarly claim is only a problem of having the wrong error message. The idea that those are unsupported for any principled reason doesn't have any justification (unlike WHEN ... AND quals, and their restrictions, which I agree are necessary). It clearly works with Oracle, for example: http://sqlfiddle.com/#!4/2d5405/10 You're reusing set_clause_list in the grammar, so I don't see why it shouldn't work within MERGE in just the same way as it works in UPDATE. While I think that there is a legitimate need for restrictions on some merge_when_clause cases, such as the VALUES() of merge_insert, this isn't an example of that. Again, this suggests to me a need for more work within the optimizer. Finally, I noticed a problem with your new EXPLAIN ANALYZE instrumentation: Is it 4 rows inserted, or 0 inserted? postgres=# merge into testoids a using (select i "key", 'foo' "data" from generate_series(0,3) i) b on a.key = b.key when matched and 1=0 then update set data = b.data when not matched then insert (key, data) values (b.key, 'foo'); MERGE 0 postgres=# explain analyze merge into testoids a using (select i "key", 'foo' "data" from generate_series(0,3) i) b on a.key = b.key when matched and 1=0 then update set data = b.data when not matched then insert (key, data) values (b.key, 'foo'); QUERY PLAN ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Merge on testoids a (cost=38.58..61.19 rows=1000 width=42) (actual time=0.043..0.049 rows=4 loops=1) Tuples Inserted: 4 Tuples Updated: 0 Tuples Deleted: 0 -> Hash Left Join (cost=38.58..61.19 rows=1000 width=42) (actual time=0.039..0.043 rows=4 loops=1) Hash Cond: (i.i = a.key) -> Function Scan on generate_series i (cost=0.00..10.00 rows=1000 width=4) (actual time=0.009..0.010 rows=4 loops=1) -> Hash (cost=22.70..22.70 rows=1270 width=10) (actual time=0.021..0.021 rows=4 loops=1) Buckets: 2048 Batches: 1 Memory Usage: 17kB -> Seq Scan on testoids a (cost=0.00..22.70 rows=1270 width=10) (actual time=0.014..0.016 rows=4 loops=1) Planning time: 0.202 ms Execution time: 0.109 ms (12 rows) (It should be 0 rows inserted, not 4.) -- Peter Geoghegan
