Обсуждение: gSoC add MERGE command new patch -- merge_v104

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