Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Feb 6, 2018 at 9:40 AM, Tomas Vondra wrote: > I think we can do that for MERGE too, assuming we actually understand > > (1) why each of the pieces is missing > > (2) what would it take to make it work Right, I completely agree with that. For example, if we

Re: [HACKERS] MERGE SQL Statement for PG11

On 02/06/2018 05:20 AM, Peter Geoghegan wrote: > On Mon, Feb 5, 2018 at 7:56 PM, Robert Haas wrote: >> I don't think you get to make a unilateral decision to exclude >> features that work everywhere else from the scope of this patch. >> If there is agreement that those

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Feb 6, 2018 at 9:19 AM, Robert Haas wrote: > On Sun, Feb 4, 2018 at 3:41 AM, Simon Riggs wrote: >>> It is not clear to me what is exactly your concern if we try to follow >>> #2? To me, #2 seems like a natural choice. >> >> At first, but it

Re: [HACKERS] MERGE SQL Statement for PG11

Greetings, * Peter Geoghegan (p...@bowt.ie) wrote: > On Mon, Feb 5, 2018 at 7:56 PM, Robert Haas wrote: > > I don't think you get to make a unilateral decision to exclude > > features that work everywhere else from the scope of this patch. If > > there is agreement that

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Feb 6, 2018 at 9:50 AM, Peter Geoghegan wrote: > On Mon, Feb 5, 2018 at 7:56 PM, Robert Haas wrote: > > I don't think you get to make a unilateral decision to exclude > > features that work everywhere else from the scope of this patch. If > > there

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Feb 5, 2018 at 7:56 PM, Robert Haas wrote: > I don't think you get to make a unilateral decision to exclude > features that work everywhere else from the scope of this patch. If > there is agreement that those features can be left out of scope, then > that is one

Re: [HACKERS] MERGE SQL Statement for PG11

On Sun, Feb 4, 2018 at 5:15 AM, Simon Riggs wrote: > Changes to support sub-selects don't invalidate what is there now in > the current patch with regard to query representation or optimization. > So support of those extra features can be added later if we choose. I don't

Re: [HACKERS] MERGE SQL Statement for PG11

On Sun, Feb 4, 2018 at 3:41 AM, Simon Riggs wrote: >> It is not clear to me what is exactly your concern if we try to follow >> #2? To me, #2 seems like a natural choice. > > At first, but it gives an anomaly so is not a good choice. The patch > does behavior #5, it

Re: [HACKERS] MERGE SQL Statement for PG11

On Sun, Feb 4, 2018 at 12:42 AM, Simon Riggs wrote: > I'm happy to quote your words. > > "I've acknowledged that the standard has something to > say on this that supports your position, which has real weight." > >

Re: [HACKERS] MERGE SQL Statement for PG11

Simon Riggs writes: > It will likely take some time to work through these and the current > work items but will fix. > > Do you have the DDL so we can recreate these easily? Attached are testcases that trigger the assertions when run against an empty database instead of the one left behind by

Re: [HACKERS] MERGE SQL Statement for PG11

On 3 February 2018 at 23:17, Peter Geoghegan wrote: > There are 3 specific issues on query structure, that together paint a > picture about what you're not doing in the optimizer: > > 1. Whether or not subselects in the UPDATE targetlist are supported. > > 2. Whether or not

Re: [HACKERS] MERGE SQL Statement for PG11

On 4 February 2018 at 06:32, Amit Kapila wrote: > On Wed, Jan 31, 2018 at 11:37 PM, Peter Geoghegan wrote: >> On Wed, Jan 31, 2018 at 7:17 AM, Robert Haas wrote: >>> I don't fully grok merge but suppose you have: >>> >>> WHEN MATCHED

Re: [HACKERS] MERGE SQL Statement for PG11

On 3 February 2018 at 23:17, Peter Geoghegan wrote: > On Sat, Feb 3, 2018 at 2:15 PM, Simon Riggs wrote: >>> I started looking at SQL Server's MERGE to verify that it also does >>> not impose any restrictions on subselects in a MERGE UPDATE's >>> targetlist,

Re: [HACKERS] MERGE SQL Statement for PG11

On Sat, Feb 3, 2018 at 2:15 PM, Simon Riggs wrote: >> I started looking at SQL Server's MERGE to verify that it also does >> not impose any restrictions on subselects in a MERGE UPDATE's >> targetlist, just like Oracle. Unsurprisingly, it does not. More >> surprisingly, I

Re: [HACKERS] MERGE SQL Statement for PG11

On 1 February 2018 at 19:39, Peter Geoghegan wrote: > 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

Re: [HACKERS] MERGE SQL Statement for PG11

On 3 February 2018 at 19:57, Andreas Seltenreich wrote: > as promised in Brussels, I taught sqlsmith about MERGE and did some > testing with merge.v14.patch applied on master at 9aef173163. > > So far, it found a couple of failing assertions and a suspicous error > message.

Re: [HACKERS] MERGE SQL Statement for PG11

On 2 February 2018 at 01:59, Peter Geoghegan wrote: > On Thu, Feb 1, 2018 at 11:39 AM, Peter Geoghegan wrote: >> 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

Re: [HACKERS] MERGE SQL Statement for PG11

Hi, as promised in Brussels, I taught sqlsmith about MERGE and did some testing with merge.v14.patch applied on master at 9aef173163. So far, it found a couple of failing assertions and a suspicous error message. Details and Testcases against the regression database below. regards, Andreas --

Re: [HACKERS] MERGE SQL Statement for PG11

On Thu, Feb 1, 2018 at 4:45 AM, Simon Riggs 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

Re: [HACKERS] MERGE SQL Statement for PG11

On 1 February 2018 at 12:45, Simon Riggs 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.e. which isolation test would we like to change from ERROR to >

Re: [HACKERS] MERGE SQL Statement for PG11

On 30 January 2018 at 21:47, Peter Geoghegan wrote: > I'm glad that we all seem to agree that serialization failures as a > way of dealing with concurrency issues in READ COMMITTED mode are a > bad idea. ERRORs are undesirable, yet safe and correct. Doing better is as yet unclear

Re: [HACKERS] MERGE SQL Statement for PG11

On 31 January 2018 at 15:17, Robert Haas wrote: > On Tue, Jan 30, 2018 at 2:28 PM, Peter Geoghegan wrote: >> What's at issue here specifically is the exact behavior of >> EvalPlanQual() in the context of having *multiple* sets of WHEN quals >> that need to be

Re: [HACKERS] MERGE SQL Statement for PG11

On Wed, Jan 31, 2018 at 7:17 AM, Robert Haas wrote: > I don't fully grok merge but suppose you have: > > WHEN MATCHED AND a = 0 THEN UPDATE ... > WHEN MATCHED AND a = 1 THEN UPDATE ... > WHEN NOT MATCHED THEN INSERT ... > > Suppose you match a tuple with a = 0 but, upon

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 30, 2018 at 2:28 PM, Peter Geoghegan wrote: > What's at issue here specifically is the exact behavior of > EvalPlanQual() in the context of having *multiple* sets of WHEN quals > that need to be evaluated one at a time (in addition to conventional > EPQ join quals). This

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 30, 2018 at 8:27 AM, Robert Haas wrote: > As far as I am able to understand, the substantive issue here is what > to do when we match an invisible tuple rather than a visible tuple. > The patch currently throws a serialization error on the basis that you >

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 30, 2018 at 8:56 AM, Simon Riggs wrote: >> Peter is arguing >> that we don't normally issue a serialization error at READ COMMITTED >> (which I think is true) and proposed that we instead try to INSERT. > > Which IMHO is case 4 since it would avoid a concurrent

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 30, 2018 at 8:27 AM, Robert Haas wrote: > As far as I am able to understand, the substantive issue here is what > to do when we match an invisible tuple rather than a visible tuple. > The patch currently throws a serialization error on the basis that you >

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 30, 2018 at 11:56 AM, Simon Riggs wrote: > Which IMHO is case 4 since it would avoid a concurrent ERROR. This > meets exactly my original implementation goals as clearly stated on > this thread, so of course I agree with him and have already said I am > happy to

Re: [HACKERS] MERGE SQL Statement for PG11

On 30 January 2018 at 16:27, Robert Haas wrote: > As far as I am able to understand, the substantive issue here is what > to do when we match an invisible tuple rather than a visible tuple. > The patch currently throws a serialization error on the basis that you > (Simon)

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 30, 2018 at 4:45 AM, Simon Riggs wrote: > "You're introducing a behavior/error"... No, I advocate nothing in the > patch, I am merely following the agreement made here: > >

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 12:03 PM, Simon Riggs wrote: > Partitioning doesn't look too bad, so that looks comfortable for PG11, > assuming it doesn't hit some unhandled complexity. > > Including RLS in the first commit/release turns this into a high risk > patch. Few people

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 7:15 PM, Michael Paquier wrote: > On Mon, Jan 29, 2018 at 04:34:48PM +, Simon Riggs wrote: >> In terms of timing of commits, I have marked the patch Ready For >> Committer. To me that signifies that it is ready for review by a >> Committer

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 10:32 AM, Simon Riggs wrote: > The code is about 1200 lines and has extensive docs, comments and tests. > > There are no contentious infrastructure changes, so the debate around > concurrency is probably the main one. So it looks to me like >

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 22:31, Peter Geoghegan wrote: > I don't think that there should be any error, so I can't say. >> I argued that was possible and desirable, but you argued it was not >> and got everybody else to agree with you. I'm surprised to see you >> change your mind on

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 04:34:48PM +, Simon Riggs wrote: > In terms of timing of commits, I have marked the patch Ready For > Committer. To me that signifies that it is ready for review by a > Committer prior to commit. My understanding of this meaning is different than yours. It should not

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 1:34 PM, Simon Riggs wrote: >> The way that routines like ExecUpdate() interact with MERGE for WHEN >> qual + EPQ handling seems kind of convoluted. I hope for something >> cleaner in the next revision. > > Cleaner? Yeah, cleaner. The fact that when

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 20:41, Peter Geoghegan wrote: > On Mon, Jan 29, 2018 at 6:11 AM, Simon Riggs wrote: >> New patch attached that correctly handles all known concurrency cases, >> with expected test output. > > This revision, v13, seems much improved in

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 6:11 AM, Simon Riggs wrote: > New patch attached that correctly handles all known concurrency cases, > with expected test output. This revision, v13, seems much improved in this area. > This means MERGE will work just fine for "normal" UPDATEs, but

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 17:35, Peter Geoghegan wrote: > On Mon, Jan 29, 2018 at 8:51 AM, Simon Riggs wrote: >> On 29 January 2018 at 16:44, Bruce Momjian wrote: >> >>> I think the question is how does it handle cases it doesn't support? >>>

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 8:51 AM, Simon Riggs wrote: > On 29 January 2018 at 16:44, Bruce Momjian wrote: > >> I think the question is how does it handle cases it doesn't support? >> Does it give wrong answers? Does it give a helpful error message? Can >>

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 8:44 AM, Bruce Momjian wrote: > On Mon, Jan 29, 2018 at 04:34:48PM +, Simon Riggs wrote: >> The only discussion would be about the word "unfinished". I'm not >> clear why this patch, which has current caveats all clearly indicated >> in the docs,

Re: [HACKERS] MERGE SQL Statement for PG11

2018-01-29 18:08 GMT+01:00 Simon Riggs : > On 29 January 2018 at 16:23, Chapman Flack wrote: > > On 01/29/2018 11:13 AM, Simon Riggs wrote: > >> On 29 January 2018 at 15:44, Bruce Momjian wrote: > >>> Uh, if we know we are going to

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 16:23, Chapman Flack wrote: > On 01/29/2018 11:13 AM, Simon Riggs wrote: >> On 29 January 2018 at 15:44, Bruce Momjian wrote: >>> Uh, if we know we are going to get question on this, the patch had >>> better have an explanation of

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 16:50, Tom Lane wrote: > Bruce Momjian writes: >> On Mon, Jan 29, 2018 at 04:34:48PM +, Simon Riggs wrote: >>> ... If unfinished means it has caveats >>> that is different to unfinished meaning crappy, risky, contentious >>> etc.. >

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 16:44, Bruce Momjian wrote: > I think the question is how does it handle cases it doesn't support? > Does it give wrong answers? Does it give a helpful error message? Can > you summarize that? I'm happy to report that it gives correct answers to every

Re: [HACKERS] MERGE SQL Statement for PG11

Bruce Momjian writes: > On Mon, Jan 29, 2018 at 04:34:48PM +, Simon Riggs wrote: >> ... If unfinished means it has caveats >> that is different to unfinished meaning crappy, risky, contentious >> etc.. > I think the question is how does it handle cases it doesn't support?

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 04:34:48PM +, Simon Riggs wrote: > I agree with all of the above. > > In terms of timing of commits, I have marked the patch Ready For > Committer. To me that signifies that it is ready for review by a > Committer prior to commit. > > In case of doubt, I would not

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 16:06, Tom Lane wrote: > Simon Riggs writes: >> On 29 January 2018 at 15:07, Robert Haas wrote: >>> An argument could be made that this patch is already too late for PG >>> 11, because it's a major feature

Re: [HACKERS] MERGE SQL Statement for PG11

On 01/29/2018 11:13 AM, Simon Riggs wrote: > On 29 January 2018 at 15:44, Bruce Momjian wrote: >> Uh, if we know we are going to get question on this, the patch had >> better have an explanation of when to use it. Pushing the problem to >> later doesn't seem helpful. > > What

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 15:44, Bruce Momjian wrote: > On Mon, Jan 29, 2018 at 03:12:23PM +, Simon Riggs wrote: >> On 29 January 2018 at 14:55, Pavel Stehule wrote: >> >> > My note was not against MERGE or INSERT ON CONFLICT. If I understand to >> >

Re: [HACKERS] MERGE SQL Statement for PG11

Simon Riggs writes: > On 29 January 2018 at 15:07, Robert Haas wrote: >> An argument could be made that this patch is already too late for PG >> 11, because it's a major feature that was not submitted in relatively >> complete form before the

Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Jan 29, 2018 at 03:12:23PM +, Simon Riggs wrote: > On 29 January 2018 at 14:55, Pavel Stehule wrote: > > > My note was not against MERGE or INSERT ON CONFLICT. If I understand to this > > topic, I agree so these commands should be implemented separately. But

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 15:07, Robert Haas wrote: > Moreover, the patch should have had meaningful review from people not > involved in writing it, and that is a process that generally takes a > few months or at least several weeks, not a few days. The code is about 1200

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 14:55, Pavel Stehule wrote: > My note was not against MERGE or INSERT ON CONFLICT. If I understand to this > topic, I agree so these commands should be implemented separately. But if we > use two commands with some intersection, there can be nice to

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 23, 2018 at 8:51 AM, Simon Riggs wrote: > This is complete and pretty clean now. 1200 lines of code, plus docs and > tests. > > I'm expecting to commit this and then come back for the Partitioning & > RLS later, but will wait a few days for comments and other

Re: [HACKERS] MERGE SQL Statement for PG11

2018-01-29 15:40 GMT+01:00 Simon Riggs : > On 29 January 2018 at 14:19, Pavel Stehule > wrote: > > >> The concurrency rules are very simple: > >> If a MATCHED row is concurrently updated/deleted > >> 1. We run EvalPlanQual > >> 2. If the updated

Re: [HACKERS] MERGE SQL Statement for PG11

On 29 January 2018 at 14:19, Pavel Stehule wrote: >> The concurrency rules are very simple: >> If a MATCHED row is concurrently updated/deleted >> 1. We run EvalPlanQual >> 2. If the updated row is gone EPQ returns NULL slot or EPQ returns a >> row with NULL values, then

Re: [HACKERS] MERGE SQL Statement for PG11

Hi 2018-01-29 15:11 GMT+01:00 Simon Riggs : > On 26 January 2018 at 11:25, Simon Riggs wrote: > > On 24 January 2018 at 04:12, Simon Riggs wrote: > >> On 24 January 2018 at 01:35, Peter Geoghegan wrote: > >>> >

Re: [HACKERS] MERGE SQL Statement for PG11

On 24 January 2018 at 01:35, Peter Geoghegan wrote: > On Tue, Jan 23, 2018 at 5:51 AM, Simon Riggs wrote: >>> Not yet working >>> * Partitioning >>> * RLS >>> >>> Based on this successful progress I imagine I'll be looking to commit >>> this by the end of the

Re: [HACKERS] MERGE SQL Statement for PG11

On Thu, Jan 4, 2018 at 12:38 PM, Simon Riggs wrote: > On 4 January 2018 at 17:29, Robert Haas wrote: >> On Sat, Dec 30, 2017 at 6:01 AM, Simon Riggs wrote: >>> Patch uses mechanism as agreed previously with Peter G et al. on

Re: [HACKERS] MERGE SQL Statement for PG11

On Sat, Dec 30, 2017 at 6:01 AM, Simon Riggs wrote: > Patch uses mechanism as agreed previously with Peter G et al. on this thread. I'm not sure that an agreement was reached, or what the substance of that agreement was. -- Robert Haas EnterpriseDB:

Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Nov 14, 2017 at 11:02 AM, Simon Riggs wrote: > That's a good place to leave this for now - we're OK to make progress > with the main feature, and we have some questions to be addressed once > we have a cake to decorate. > > Thanks for your input. Thanks for

<    1   2