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

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(at)gmail(dot)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
>

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(at)anarazel(dot)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...
>

On Tue, Aug 24, 2010 at 4:56 PM, Andres Freund <andres(at)anarazel(dot)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(at)anarazel(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)anarazel(dot)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.com
PostgreSQL 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