Thread: gSoC add MERGE command new patch -- merge_v104
Hi,
Here comes the new patch for MERGE command. It has the following features:
1. It is based on Heikki's merge_v102-cleanedup.patch. So, it is (hopefully) clean -- no meaningless white spaces and no overlong clause.
2. The "replaced" mark in MERGE query and plan structures are removed. In rewriter, the actions replaced by INSTEAD rules will be changed into DO NOTHING actions.
3. _outDeleteStmt() is removed from code.
4. EXPLAIN MERGE is improved much. You can see the new examples at https://wiki.postgresql.org/wiki/MergeTestExamples#Explain_Merge
5. The subplan/sublinks are supported in merge actions now. Try the examples at https://wiki.postgresql.org/wiki/MergeTestExamples#Subplan.2Fsublinks_in_action
6. Updated merge.sql and merge.out for regress
7. The inheritance is still NOT supported yet.
Thanks
Regards
Yours Boxuan
Attachment
Hi,
I finished the MERGE on inheritance tables. Now comes the merge_v201
The test example can be found at https://wiki.postgresql.org/wiki/MergeTestExamples#MERGE_on_inheritance
Thanks
On Thu, Aug 19, 2010 at 10:01 PM, Boxuan Zhai <bxzhai2010@gmail.com> wrote:
Hi,Here comes the new patch for MERGE command. It has the following features:1. It is based on Heikki's merge_v102-cleanedup.patch. So, it is (hopefully) clean -- no meaningless white spaces and no overlong clause.2. The "replaced" mark in MERGE query and plan structures are removed. In rewriter, the actions replaced by INSTEAD rules will be changed into DO NOTHING actions.3. _outDeleteStmt() is removed from code.4. EXPLAIN MERGE is improved much. You can see the new examples at https://wiki.postgresql.org/wiki/MergeTestExamples#Explain_Merge5. The subplan/sublinks are supported in merge actions now. Try the examples at https://wiki.postgresql.org/wiki/MergeTestExamples#Subplan.2Fsublinks_in_action6. Updated merge.sql and merge.out for regress7. The inheritance is still NOT supported yet.ThanksRegardsYours Boxuan
Attachment
On 24/08/10 16:35, Boxuan Zhai wrote: > Hi, > > I finished the MERGE on inheritance tables. Now comes the merge_v201 Oh, great! That means that all the known issues are fixed now, and all that's left is fixing any issues raised in review. I've added this to the September commitfest, but I hope I'll find some time to look at this before that. I welcome anyone else to review this too! -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Tue, Aug 24, 2010 at 11:02:41PM +0300, Heikki Linnakangas wrote: > On 24/08/10 16:35, Boxuan Zhai wrote: > >Hi, > > > >I finished the MERGE on inheritance tables. Now comes the merge_v201 > > Oh, great! That means that all the known issues are fixed now, and > all that's left is fixing any issues raised in review. > > I've added this to the September commitfest, but I hope I'll find > some time to look at this before that. I welcome anyone else to > review this too! I have to ask one question: On a short review of the discussion and the patch I didn't find anything about the concurrency issues involved (at least nodeModifyTable.c didnt show any). Whats the plan to go forward at that subject? I think the patch needs to lock tables exclusively (the pg level, not access exclusive) as long as there is no additional handling... Thanks for the work Boxuan! Andres PS: The patch reintroduces some whitespace damage...
On Wed, Aug 25, 2010 at 4:56 AM, Andres Freund <andres@anarazel.de> wrote:
On Tue, Aug 24, 2010 at 11:02:41PM +0300, Heikki Linnakangas wrote:I have to ask one question: On a short review of the discussion and
> On 24/08/10 16:35, Boxuan Zhai wrote:
> >Hi,
> >
> >I finished the MERGE on inheritance tables. Now comes the merge_v201
>
> Oh, great! That means that all the known issues are fixed now, and
> all that's left is fixing any issues raised in review.
>
> I've added this to the September commitfest, but I hope I'll find
> some time to look at this before that. I welcome anyone else to
> review this too!
the patch I didn't find anything about the concurrency issues
involved (at least nodeModifyTable.c didnt show any).
Whats the plan to go forward at that subject? I think the patch needs
to lock tables exclusively (the pg level, not access exclusive) as
long as there is no additional handling...
Thanks for the work Boxuan!
The concurrency issues are not involved. I don't know much about this part. I think we need more discussion on it.
Andres
PS: The patch reintroduces some whitespace damage...
On Tue, Aug 24, 2010 at 4:56 PM, Andres Freund <andres@anarazel.de> wrote: > Whats the plan to go forward at that subject? I think the patch needs > to lock tables exclusively (the pg level, not access exclusive) as > long as there is no additional handling... That sounds like it might cause more problems than it solves. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Wed, Aug 25, 2010 at 08:11:18AM +0800, Boxuan Zhai wrote: > On Wed, Aug 25, 2010 at 4:56 AM, Andres Freund <andres@anarazel.de> wrote: > > > On Tue, Aug 24, 2010 at 11:02:41PM +0300, Heikki Linnakangas wrote: > > > On 24/08/10 16:35, Boxuan Zhai wrote: > > > >Hi, > > > > > > > >I finished the MERGE on inheritance tables. Now comes the > > > >merge_v201 > > > > > > Oh, great! That means that all the known issues are fixed now, > > > and all that's left is fixing any issues raised in review. > > > > > > I've added this to the September commitfest, but I hope I'll > > > find some time to look at this before that. I welcome anyone > > > else to review this too! > > I have to ask one question: On a short review of the discussion > > and the patch I didn't find anything about the concurrency issues > > involved (at least nodeModifyTable.c didnt show any). Whats the > > plan to go forward at that subject? I think the patch needs to > > lock tables exclusively (the pg level, not access exclusive) as > > long as there is no additional handling... > > > > Thanks for the work Boxuan! > > The concurrency issues are not involved. I don't know much about > this part. I think we need more discussion on it. I seem to recall Simon had volunteered some of 2ndQuadrant's time on this. :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On 24/08/10 23:56, Andres Freund wrote: > I have to ask one question: On a short review of the discussion and > the patch I didn't find anything about the concurrency issues > involved (at least nodeModifyTable.c didnt show any). The SQL spec doesn't require MERGE to be an atomic "upsert" operation. > Whats the plan to go forward at that subject? I think the patch needs > to lock tables exclusively (the pg level, not access exclusive) as > long as there is no additional handling... Well, you can always do LOCK TABLE before calling MERGE if that's what you want, but I don't think doing that automatically would make people happy. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 2010-08-25 9:26 AM +0300, Heikki Linnakangas wrote: >> Whats the plan to go forward at that subject? I think the patch needs >> to lock tables exclusively (the pg level, not access exclusive) as >> long as there is no additional handling... > > Well, you can always do LOCK TABLE before calling MERGE if that's what > you want, but I don't think doing that automatically would make people > happy. I don't think having a MERGE that throws UNIQUE violations would make people happy either. Regards, Marko Tiikkaja
On Wed, Aug 25, 2010 at 09:26:51AM +0300, Heikki Linnakangas wrote: > On 24/08/10 23:56, Andres Freund wrote: > >I have to ask one question: On a short review of the discussion and > >the patch I didn't find anything about the concurrency issues > >involved (at least nodeModifyTable.c didnt show any). > The SQL spec doesn't require MERGE to be an atomic "upsert" operation. > >Whats the plan to go forward at that subject? I think the patch needs > >to lock tables exclusively (the pg level, not access exclusive) as > >long as there is no additional handling... > Well, you can always do LOCK TABLE before calling MERGE if that's > what you want, but I don't think doing that automatically would make > people happy. But randomly loosing tuples will make much more people unhappy. At a much more problematic point of time (in production). There is no locking prohibiting situations like trying to update a tuple which was concurrently deleted and thus loosing a tuple. Unless I miss something. Andres
On 25/08/10 12:41, Andres Freund wrote: > But randomly loosing tuples will make much more people unhappy. At a > much more problematic point of time (in production). Hmm, how would you lose tuples? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Tue, 2010-08-24 at 18:23 -0700, David Fetter wrote: > On Wed, Aug 25, 2010 at 08:11:18AM +0800, Boxuan Zhai wrote: > > On Wed, Aug 25, 2010 at 4:56 AM, Andres Freund <andres@anarazel.de> wrote: > > > > The concurrency issues are not involved. I don't know much about > > this part. I think we need more discussion on it. > > I seem to recall Simon had volunteered some of 2ndQuadrant's time on > this. :) I have offered to work on the concurrency issues, though that will be after the main body of work is committed to avoid wasting effort. That seems increasingly likely to happen in next release now, given state of patch and what looks like a very short dev window for 9.1. This is one reason why I'm in favour of bringing something to commit as early as possible, so the additional aspects can be added later. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
On 2010-08-25 12:44 PM +0300, Heikki Linnakangas wrote: > On 25/08/10 12:41, Andres Freund wrote: >> But randomly loosing tuples will make much more people unhappy. At a >> much more problematic point of time (in production). > > Hmm, how would you lose tuples? I think what Andres means is: T1 starts a MERGE. INSERT fails because the tuple already exists, but then another transaction, T2, DELETEs that tuple. T1 tries to UPDATE it, but fails because it doesn't exist anymore. Not T1 should go back and INSERT the tuple, but that isn't what happens with this patch, is it? Regards, Marko Tiikkaja