Thread: gSoC add MERGE command new patch -- merge_v104

gSoC add MERGE command new patch -- merge_v104

From
Boxuan Zhai
Date:
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

Re: gSoC add MERGE command new patch -- merge_v104

From
Boxuan Zhai
Date:
Hi,
 
I finished the MERGE on inheritance tables. Now comes the merge_v201
 
 
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_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

Re: gSoC add MERGE command new patch -- merge_v104

From
Heikki Linnakangas
Date:
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


Re: gSoC add MERGE command new patch -- merge_v104

From
Andres Freund
Date:
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...


Re: gSoC add MERGE command new patch -- merge_v104

From
Boxuan Zhai
Date:


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. 
  
 
Andres

PS: The patch reintroduces some whitespace damage...

Re: gSoC add MERGE command new patch -- merge_v104

From
Robert Haas
Date:
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


Re: gSoC add MERGE command new patch -- merge_v104

From
David Fetter
Date:
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


Re: gSoC add MERGE command new patch -- merge_v104

From
Heikki Linnakangas
Date:
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


Re: gSoC add MERGE command new patch -- merge_v104

From
Marko Tiikkaja
Date:
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


Re: gSoC add MERGE command new patch -- merge_v104

From
Andres Freund
Date:
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 


Re: gSoC add MERGE command new patch -- merge_v104

From
Heikki Linnakangas
Date:
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


Re: gSoC add MERGE command new patch -- merge_v104

From
Simon Riggs
Date:
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



Re: gSoC add MERGE command new patch -- merge_v104

From
Marko Tiikkaja
Date:
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