Re: MERGE command for inheritance

Lists: pgsql-hackers
From: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: MERGE command for inheritance
Date: 2010-08-10 09:38:09
Message-ID: AANLkTik34fNx6QSbQP+SB8BTjiCDygsBqSMQqpUHJDFW@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi,

These days I am considering what else can be done for MERGE, And, I
find inheritance tables in postgres is not supported by our MERGE command
yet.

Currently, MERGE command is only able to handle the target table itself, and
its children tables are not involved in the process.
I am not sure if inheritance of MERGE is needed by postgres. If we need, I
may propose two methods for implementing it.

An easy way to do it is use a rule-like strategy. We can generate new MERGE
query statements with the children table as their target tables. Then the
new
query statement will be planned and executed in the normal way. This process
can be put in the rewriter, before the queries are planned.
This method is quite easy but seems not follow the tradition of inheritance
in Postgres.

The difficult way is to generate the plans for children table in planner, as
the other commands like UPDATE and DELETE. However, because the structure of
MERGE plan is much more complex than the ordinary ModifyTable plans, this
job may not as simple as we expected. We need to adjust both the main plan
and the
merge actions to fit the children tables, which is not straight forward.

I would like to know your opinions on this problem.

PS: for my investigation on the inheritance actions, I find that although
the children tables are modified by the UPDATE or DELETE commands on their
ancestor tables, the rules defined on them are not activated during the
query. Is this the case (I hope I am not asking a stupid question)? And, if
so, I may ask why we want it to act like this.

Boxuan


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-10 14:03:02
Message-ID: 1281448982.19111.3.camel@fsopti579.F-Secure.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On tis, 2010-08-10 at 17:38 +0800, Boxuan Zhai wrote:
> I am not sure if inheritance of MERGE is needed by postgres.

Yes, it is.

> PS: for my investigation on the inheritance actions, I find that
> although the children tables are modified by the UPDATE or DELETE
> commands on their ancestor tables, the rules defined on them are not
> activated during the query. Is this the case (I hope I am not asking a
> stupid question)? And, if so, I may ask why we want it to act like
> this.

Your observation is correct. You could probably argue this way or that
about how it should have been designed 20+ years ago, but this is how it
is.

In general, I wouldn't design new functionality on top of rules. Rules
are pretty broken in many ways.


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-10 14:15:55
Message-ID: 4C615F1B.10606@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 10/08/10 12:38, Boxuan Zhai wrote:
> The difficult way is to generate the plans for children table in planner, as
> the other commands like UPDATE and DELETE. However, because the structure of
> MERGE plan is much more complex than the ordinary ModifyTable plans, this
> job may not as simple as we expected. We need to adjust both the main plan
> and the
> merge actions to fit the children tables, which is not straight forward.

This the approach you'll have to take. But actually, I'm surprised it
doesn't happen to just work already. It should be opaque to the merge
facility that the reference to the parent target table has inherited
child tables - expanding the inherited table to scans of all the
children should already be handled by the planner.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 08:27:51
Message-ID: 4C625F07.70209@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 10/08/10 12:38, Boxuan Zhai wrote:
> These days I am considering what else can be done for MERGE, And, I
> find inheritance tables in postgres is not supported by our MERGE command
> yet.

I played with your latest patch version a bit, and actually, it seems to
me that inherited tables work just fine. I ran into the assertion
failures earlier while trying that, but that has now been fixed. Can you
give an example of the kind of query that's not working yet?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 08:45:09
Message-ID: 1281516309.2142.1504.camel@ebony
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, 2010-08-10 at 17:15 +0300, Heikki Linnakangas wrote:
> On 10/08/10 12:38, Boxuan Zhai wrote:
> > The difficult way is to generate the plans for children table in planner, as
> > the other commands like UPDATE and DELETE. However, because the structure of
> > MERGE plan is much more complex than the ordinary ModifyTable plans, this
> > job may not as simple as we expected. We need to adjust both the main plan
> > and the
> > merge actions to fit the children tables, which is not straight forward.
>
> This the approach you'll have to take. But actually, I'm surprised it
> doesn't happen to just work already. It should be opaque to the merge
> facility that the reference to the parent target table has inherited
> child tables - expanding the inherited table to scans of all the
> children should already be handled by the planner.

The support for UPDATE and SELECT of partitioned cases is very different
in the planner and was handled as separate implementation projects.

If we want a working MERGE in the next release, I suggest that we break
down this project in the same way and look at partitioned target tables
as a separate project.

One reason for suggesting this is that all MERGE statements have a
source table, whereas UPDATE and DELETEs did not always. The plan for a
simple UPDATE and DELETE against a partitioned table is simple, but the
plan (and performance) of a joined UPDATE or DELETE is not good:

postgres=# explain update p set col2 = x.col2 from x where x.col1 =
p.col1;
QUERY
PLAN
---------------------------------------------------------------------------
Update (cost=299.56..1961.18 rows=68694 width=20)
-> Merge Join (cost=299.56..653.73 rows=22898 width=20)
Merge Cond: (public.p.col1 = x.col1)
-> Sort (cost=149.78..155.13 rows=2140 width=10)
Sort Key: public.p.col1
-> Seq Scan on p (cost=0.00..31.40 rows=2140 width=10)
-> Sort (cost=149.78..155.13 rows=2140 width=14)
Sort Key: x.col1
-> Seq Scan on x (cost=0.00..31.40 rows=2140 width=14)
-> Merge Join (cost=299.56..653.73 rows=22898 width=20)
Merge Cond: (public.p.col1 = x.col1)
-> Sort (cost=149.78..155.13 rows=2140 width=10)
Sort Key: public.p.col1
-> Seq Scan on p1 p (cost=0.00..31.40 rows=2140
width=10)
-> Sort (cost=149.78..155.13 rows=2140 width=14)
Sort Key: x.col1
-> Seq Scan on x (cost=0.00..31.40 rows=2140 width=14)
-> Merge Join (cost=299.56..653.73 rows=22898 width=20)
Merge Cond: (public.p.col1 = x.col1)
-> Sort (cost=149.78..155.13 rows=2140 width=10)
Sort Key: public.p.col1
-> Seq Scan on p2 p (cost=0.00..31.40 rows=2140
width=10)
-> Sort (cost=149.78..155.13 rows=2140 width=14)
Sort Key: x.col1
-> Seq Scan on x (cost=0.00..31.40 rows=2140 width=14)

Those plans could use some love and attention before forcing Boxuan to
implement that.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 10:25:13
Message-ID: 4C627A89.60108@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 11/08/10 11:45, Simon Riggs wrote:
> On Tue, 2010-08-10 at 17:15 +0300, Heikki Linnakangas wrote:
>> On 10/08/10 12:38, Boxuan Zhai wrote:
>>> The difficult way is to generate the plans for children table in planner, as
>>> the other commands like UPDATE and DELETE. However, because the structure of
>>> MERGE plan is much more complex than the ordinary ModifyTable plans, this
>>> job may not as simple as we expected. We need to adjust both the main plan
>>> and the
>>> merge actions to fit the children tables, which is not straight forward.
>>
>> This the approach you'll have to take. But actually, I'm surprised it
>> doesn't happen to just work already. It should be opaque to the merge
>> facility that the reference to the parent target table has inherited
>> child tables - expanding the inherited table to scans of all the
>> children should already be handled by the planner.
>
> The support for UPDATE and SELECT of partitioned cases is very different
> in the planner and was handled as separate implementation projects.

Ok, thinking and experminting this some more I finally understand what
the problem is. Yeah, the patch doesn't currently work when the target
table has inherited child tables, it only takes the parent table into
account and ignores all child tables.

> If we want a working MERGE in the next release, I suggest that we break
> down this project in the same way and look at partitioned target tables
> as a separate project.
>
> One reason for suggesting this is that all MERGE statements have a
> source table, whereas UPDATE and DELETEs did not always. The plan for a
> simple UPDATE and DELETE against a partitioned table is simple, but the
> plan (and performance) of a joined UPDATE or DELETE is not good:

I don't think we can just leave it as it is. If the performance sucks,
that's fine and can be handled in a future release, but it should at
least produce the correct result.

I concur that Boxuan's suggested "difficult" approach seems like the
right one.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 11:44:00
Message-ID: 1281527040.2142.1527.camel@ebony
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, 2010-08-11 at 13:25 +0300, Heikki Linnakangas wrote:

> I concur that Boxuan's suggested "difficult" approach seems like the
> right one.

Right, but you've completely ignored my proposal: lets do this in two
pieces. Get what we have now ready to commit, then add support for
partitioning later, as a second project.

Two reasons for this: we endanger the current project by adding more to
it in one go, plus work on other aspects of partitioning is happening
concurrently and the two are likely to conflict and/or waste effort.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services


From: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 12:51:16
Message-ID: AANLkTinBDgH-3keZP6H20nywvHxVpxRfhUtzt5m_F3Pc@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Aug 11, 2010 at 4:27 PM, Heikki Linnakangas <
heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> On 10/08/10 12:38, Boxuan Zhai wrote:
>
>> These days I am considering what else can be done for MERGE, And, I
>> find inheritance tables in postgres is not supported by our MERGE command
>> yet.
>>
>
> I played with your latest patch version a bit, and actually, it seems to me
> that inherited tables work just fine. I ran into the assertion failures
> earlier while trying that, but that has now been fixed. Can you give an
> example of the kind of query that's not working yet?
>
>
Well, in the patch I submitted, the target relation is forced not to scan
any inheritance tables. That is, the command always acts like
MERGE into *ONLY* foo USING bar ....

So, the inheritance in current MERGE should not work, I think.

>
> --
> Heikki Linnakangas
> EnterpriseDB http://www.enterprisedb.com
>


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 12:53:12
Message-ID: 4C629D38.7040609@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 11/08/10 14:44, Simon Riggs wrote:
> On Wed, 2010-08-11 at 13:25 +0300, Heikki Linnakangas wrote:
>
>> I concur that Boxuan's suggested "difficult" approach seems like the
>> right one.
>
> Right, but you've completely ignored my proposal: lets do this in two
> pieces. Get what we have now ready to commit, then add support for
> partitioning later, as a second project.

It seems like a pretty serious omission. What would you do, thrown a
"MERGE to inherited tables not implemented" error?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 13:32:51
Message-ID: 1281533571.2142.1609.camel@ebony
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, 2010-08-11 at 15:53 +0300, Heikki Linnakangas wrote:
> On 11/08/10 14:44, Simon Riggs wrote:
> > On Wed, 2010-08-11 at 13:25 +0300, Heikki Linnakangas wrote:
> >
> >> I concur that Boxuan's suggested "difficult" approach seems like the
> >> right one.
> >
> > Right, but you've completely ignored my proposal: lets do this in two
> > pieces. Get what we have now ready to commit, then add support for
> > partitioning later, as a second project.
>
> It seems like a pretty serious omission. What would you do, thrown a
> "MERGE to inherited tables not implemented" error?

It's not a "serious omission" to do work in multiple phases. I have not
proposed that we neglect that work, only that it happens afterwards.
Phasing work often allows the whole to be delivered quicker and it
reduces the risk that we end up with nothing at all or spaghetti code
through rushing things.

We have already split MERGE into two phases from its original scope,
where the majority thought for many years that MERGE without concurrent
locking was unacceptable. Splitting MERGE into 3 phases now is hardly an
earth shaking proposal.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services


From: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
To: Simon Riggs <simon(at)2ndquadrant(dot)com>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 14:09:43
Message-ID: AANLkTinzLoVCsCCt0fVm8vUS4Hpza-nv5013JkcVyX+p@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Aug 11, 2010 at 4:45 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On Tue, 2010-08-10 at 17:15 +0300, Heikki Linnakangas wrote:
> > On 10/08/10 12:38, Boxuan Zhai wrote:
> > > The difficult way is to generate the plans for children table in
> planner, as
> > > the other commands like UPDATE and DELETE. However, because the
> structure of
> > > MERGE plan is much more complex than the ordinary ModifyTable plans,
> this
> > > job may not as simple as we expected. We need to adjust both the main
> plan
> > > and the
> > > merge actions to fit the children tables, which is not straight
> forward.
> >
> > This the approach you'll have to take. But actually, I'm surprised it
> > doesn't happen to just work already. It should be opaque to the merge
> > facility that the reference to the parent target table has inherited
> > child tables - expanding the inherited table to scans of all the
> > children should already be handled by the planner.
>
> The support for UPDATE and SELECT of partitioned cases is very different
> in the planner and was handled as separate implementation projects.
>
> If we want a working MERGE in the next release, I suggest that we break
> down this project in the same way and look at partitioned target tables
> as a separate project.
>
> One reason for suggesting this is that all MERGE statements have a
> source table, whereas UPDATE and DELETEs did not always. The plan for a
> simple UPDATE and DELETE against a partitioned table is simple, but the
> plan (and performance) of a joined UPDATE or DELETE is not good:
>
> postgres=# explain update p set col2 = x.col2 from x where x.col1 =
> p.col1;
> QUERY
> PLAN
> ---------------------------------------------------------------------------
> Update (cost=299.56..1961.18 rows=68694 width=20)
> -> Merge Join (cost=299.56..653.73 rows=22898 width=20)
> Merge Cond: (public.p.col1 = x.col1)
> -> Sort (cost=149.78..155.13 rows=2140 width=10)
> Sort Key: public.p.col1
> -> Seq Scan on p (cost=0.00..31.40 rows=2140 width=10)
> -> Sort (cost=149.78..155.13 rows=2140 width=14)
> Sort Key: x.col1
> -> Seq Scan on x (cost=0.00..31.40 rows=2140 width=14)
> -> Merge Join (cost=299.56..653.73 rows=22898 width=20)
> Merge Cond: (public.p.col1 = x.col1)
> -> Sort (cost=149.78..155.13 rows=2140 width=10)
> Sort Key: public.p.col1
> -> Seq Scan on p1 p (cost=0.00..31.40 rows=2140
> width=10)
> -> Sort (cost=149.78..155.13 rows=2140 width=14)
> Sort Key: x.col1
> -> Seq Scan on x (cost=0.00..31.40 rows=2140 width=14)
> -> Merge Join (cost=299.56..653.73 rows=22898 width=20)
> Merge Cond: (public.p.col1 = x.col1)
> -> Sort (cost=149.78..155.13 rows=2140 width=10)
> Sort Key: public.p.col1
> -> Seq Scan on p2 p (cost=0.00..31.40 rows=2140
> width=10)
> -> Sort (cost=149.78..155.13 rows=2140 width=14)
> Sort Key: x.col1
> -> Seq Scan on x (cost=0.00..31.40 rows=2140 width=14)
>
> Those plans could use some love and attention before forcing Boxuan to
> implement that.
>
>

It seems that we have not decided whether to put the inheritance for MERGE
off for a latter implementation. But, I think we can discuss how to do it
now.

First of all, the inheritance of MERGE should not be implemented in the
rule-like way. I agree that the easy way I proposed is not consistent with
the general inheritance process in postgres.

The normal way of doing this is to handle it in planner, to be more
specific, we need to extend the function "inheritance_planner()" for
processing MERGE queries.

For UPDATE and DELETE commands (INSERT is not an inheritable command), if
"inheritance_planner" finds that the target table has children tables, it
will generate a list of queries. These queries are almost the same as the
original query input by user, except for the different target
relations. Each child table has it corresponding query in this list.

This list of queries will then be processed by "grouping_planner()" and
transformed into a list of plans. One most important work finished in
this function is to extend the target list of target relations to make sure
that all attributes of a target relation appears in the final result tuple
of its plan.

As for MERGE command, we need to do the same thing. But, since the main
query body is a LEFT JOIN query between source table and target table, the
top-level target list is a combination of all the attributes from source
table and target table. Thus, when we extend the target list, we should only
extent the part of target relations, and keep the source table part
untouched.

Once a main query in this style has been transformed to plan, we need to
prepare the merge actions for it too. That is, extend the target list of all
UPDATE and INSERT actions for the corresponding target relation. In this
way, each target relation will have its own "main plan + merge action" set.

The main plan will be executed one by one, so is the merge action sets, each
for one target relation.

One more thing I want to point out is that, the INSERT is also an
inheritable action in MERGE. For a plain INSERT command, all the inserted
tuples are put in the target table ONLY. It is easy to understand. We don't
want to duplicate all the new tuples in all children tables. However, in
MERGE command, an INSERT action is activated by the tuples fitting its
matching conditions. The main plan of a MERGE command will scan all the
tuples in target relation and its children tables. If one tuple in a child
table meets the requirements of INSERT actions, the insertion should be
taken on the child table itself rather than its ancestor.

PS: Since I have taken this project, I will do my best to make it perfect.
I will keep working on MERGE until it is really finished, even after the
gSoC. (unless you guys has other plans).

> --
> Simon Riggs www.2ndQuadrant.com <http://www.2ndquadrant.com/>
> PostgreSQL Development, 24x7 Support, Training and Services
>
>


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: Simon Riggs <simon(at)2ndquadrant(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 14:14:37
Message-ID: AANLkTinRzoSKr_dxczB9OzZOsHng7SmMs17YF5G2QV4_@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Aug 11, 2010 at 10:09 AM, Boxuan Zhai <bxzhai2010(at)gmail(dot)com> wrote:
> PS: Since I have taken this project, I will do my best to make it perfect.
> I will keep working on MERGE until it is really finished, even after the
> gSoC. (unless you guys has other plans).

That is great to hear!

FWIW, I agree with Heikki that we should try to have the inheritance
stuff working properly in the first version.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 14:45:24
Message-ID: 1281537924.2142.1693.camel@ebony
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, 2010-08-11 at 22:09 +0800, Boxuan Zhai wrote:

> One more thing I want to point out is that, the INSERT is also an
> inheritable action in MERGE. For a plain INSERT command, all the
> inserted tuples are put in the target table ONLY. It is easy to
> understand. We don't want to duplicate all the new tuples in all
> children tables. However, in MERGE command, an INSERT action is
> activated by the tuples fitting its matching conditions. The main plan
> of a MERGE command will scan all the tuples in target relation and its
> children tables. If one tuple in a child table meets the
> requirements of INSERT actions, the insertion should be taken on the
> child table itself rather than its ancestor.

It seems clear that your work in this area will interfere with the work
on partitioning and insert routing. We've seen it time and time again
that big projects that aim to deliver towards end of a release cycle
interfere with dev of other projects and leave loose ends from
unforeseen interactions. There's no need for that.

> PS: Since I have taken this project, I will do my best to make it
> perfect. I will keep working on MERGE until it is really finished,
> even after the gSoC. (unless you guys has other plans).

You can make things perfect in more than one phase, as indeed you
already are: concurrent locking has already been placed out of scope of
your current work.

I don't question your good intentions to both complete this work and do
it on time. I question the need for us to rely on that. I also question
the ability of the community to deliver super-size features in a single
release. Breaking things down is always the best way.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 14:52:24
Message-ID: 4C62B928.8070201@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 11/08/10 17:45, Simon Riggs wrote:
> It seems clear that your work in this area will interfere with the work
> on partitioning and insert routing.

Nothing concrete has come out of that work yet. And we should have MERGE
work with inherited tables, regardless of any future work that may
happen with partitioning.

> We've seen it time and time again
> that big projects that aim to deliver towards end of a release cycle
> interfere with dev of other projects and leave loose ends from
> unforeseen interactions. There's no need for that.

I don't understand what you're saying, we're not in the end of a release
cycle.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 15:03:13
Message-ID: 5613.1281538993@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Wed, 2010-08-11 at 13:25 +0300, Heikki Linnakangas wrote:
>> I concur that Boxuan's suggested "difficult" approach seems like the
>> right one.

> Right, but you've completely ignored my proposal: lets do this in two
> pieces. Get what we have now ready to commit, then add support for
> partitioning later, as a second project.

Do we really think this is anywhere near committable now?

If it's committable in every other respect, I could see just having it
throw a NOT_IMPLEMENTED error when the target table has children.
I thought we were still a very long way from that though.

regards, tom lane


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 15:23:27
Message-ID: 1281540207.2142.1713.camel@ebony
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, 2010-08-11 at 11:03 -0400, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Wed, 2010-08-11 at 13:25 +0300, Heikki Linnakangas wrote:
> >> I concur that Boxuan's suggested "difficult" approach seems like the
> >> right one.
>
> > Right, but you've completely ignored my proposal: lets do this in two
> > pieces. Get what we have now ready to commit, then add support for
> > partitioning later, as a second project.
>
> Do we really think this is anywhere near committable now?
>
> If it's committable in every other respect, I could see just having it
> throw a NOT_IMPLEMENTED error when the target table has children.
> I thought we were still a very long way from that though.

Well, if we go off chasing this particular goose then we will set
ourselves back at least one commitfest. I'd rather work towards having a
fully committable patch without inheritance sooner than an even bigger
patch arriving later in the cycle, which could make things difficult for
us.

I cite recent big patch experience as admissible evidence, m'lord.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Simon Riggs <simon(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 15:29:39
Message-ID: AANLkTim7p8KmwY0OP1x0dh_48nE55D-qwBC0=V5az9eq@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Aug 11, 2010 at 11:23 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Well, if we go off chasing this particular goose then we will set
> ourselves back at least one commitfest. I'd rather work towards having a
> fully committable patch without inheritance sooner than an even bigger
> patch arriving later in the cycle, which could make things difficult for
> us.

Let's give Boxuan a little time to work and see what he comes up with.
Maybe it won't be too bad.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Simon Riggs <simon(at)2ndquadrant(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 15:38:07
Message-ID: 6404.1281541087@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Aug 11, 2010 at 11:23 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Well, if we go off chasing this particular goose then we will set
>> ourselves back at least one commitfest. I'd rather work towards having a
>> fully committable patch without inheritance sooner than an even bigger
>> patch arriving later in the cycle, which could make things difficult for
>> us.

> Let's give Boxuan a little time to work and see what he comes up with.
> Maybe it won't be too bad.

I tend to agree with Simon's argument here: if the patch is near
committable then it'd be better to get it committed and work on
correcting this omission afterwards. I'm not sure about the truth of
the "if" part, though.

regards, tom lane


From: Greg Smith <greg(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Simon Riggs <simon(at)2ndQuadrant(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 16:49:05
Message-ID: 4C62D481.2030002@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane wrote:
> Do we really think this is anywhere near committable now?
>

There's a relatively objective standard for the first thing needed for
commit--does it work?--in the form of the regression tests Simon put
together before development. I just tried the latest merge_v102.patch
(regression diff attached) to see how that's going. There are still a
couple of errors in there. It looks to me like the error handling and
related DO NOTHING support are the next pair of things that patch needs
work on. I'd rather see that sorted out than to march onward to
inheritance without the fundamentals even nailed down yet.

--
Greg Smith 2ndQuadrant US Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.us

Attachment Content-Type Size
regression.diffs text/plain 4.5 KB

From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Simon Riggs <simon(at)2ndquadrant(dot)com>, Boxuan Zhai <bxzhai2010(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: MERGE command for inheritance
Date: 2010-08-11 18:09:17
Message-ID: 1281549988-sup-763@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Excerpts from Heikki Linnakangas's message of mié ago 11 10:52:24 -0400 2010:
> On 11/08/10 17:45, Simon Riggs wrote:

> > We've seen it time and time again
> > that big projects that aim to deliver towards end of a release cycle
> > interfere with dev of other projects and leave loose ends from
> > unforeseen interactions. There's no need for that.
>
> I don't understand what you're saying, we're not in the end of a release
> cycle.

This patch still needs a lot of work before it's anywhere close to
committable. I agree with Simon that it is preferrable to clean it up
to make it committable *without* the burden of extra features. If
Boxuan continues to add more features, it will be end-of-release before
it is possible to think about committing it.

It seems better to have merge-no-inheritance in 9.1 than nothing. If we
can get the inheritance case working for 9.1, that's even better, but I
don't think it needs to be a hard requirement.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


From: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
To: Greg Smith <greg(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-12 06:24:55
Message-ID: AANLkTiniKEbwi6LQxi74vFU+2CTDibvG-x1styD=qoU3@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Aug 12, 2010 at 12:49 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:

> Tom Lane wrote:
>
>> Do we really think this is anywhere near committable now?
>>
>>
>
> There's a relatively objective standard for the first thing needed for
> commit--does it work?--in the form of the regression tests Simon put
> together before development. I just tried the latest merge_v102.patch
> (regression diff attached) to see how that's going. There are still a
> couple of errors in there. It looks to me like the error handling and
> related DO NOTHING support are the next pair of things that patch needs work
> on. I'd rather see that sorted out than to march onward to inheritance
> without the fundamentals even nailed down yet.
>
>

Sorry for the mismatch problem of regress. In fact, I am still unable to
make the regression test run on my machine. When I try the command
pg_regreess in /src/test/regress/, it always gives a FATAL error:

FATAL: parameter "port" cannot be changed without restarting the server
psql: FATAL: parameter "port" cannot be changed without restarting the
server
command failed: ""C:/msys/1.0/local/pgsql/bin//psql" -X -c "DROP DATABASE IF
EXISTS \"regression\"" "postgres""

However, I can run this command directly in the MinGW command line
interface. I guess this is because the psql_command() function has some
check before accept commands. And the MinGW environment cannot pass these
checks.

All the SQL commands in the .sql file have been tested by hand. And they are
all correct. However, the .out file is not automatic generated by pgsql.

I may need to find a linux system to try to generate the correct .out file
some other time. Or, would someone help me to generate an .out file through
pg_regress?

> --
> Greg Smith 2ndQuadrant US Baltimore, MD
> PostgreSQL Training, Services and Support
> greg(at)2ndQuadrant(dot)com www.2ndQuadrant.us <http://www.2ndquadrant.us/>
>
>
> ***
> /home/postgres/pgwork/repo/git/postgresql/src/test/regress/expected/merge.out
> 2010-08-11 12:23:50.000000000 -0400
> ---
> /home/postgres/pgwork/repo/git/postgresql/src/test/regress/results/merge.out
> 2010-08-11 12:33:27.000000000 -0400
> ***************
> *** 44,57 ****
> WHEN MATCHED THEN
> UPDATE SET balance = t.balance + s.balance
> ;
> ! SELECT * FROM target;
> ! id | balance
> ! ----+---------
> ! 1 | 10
> ! 2 | 25
> ! 3 | 50
> ! (3 rows)
> !
> ROLLBACK;
> -- do a simple equivalent of an INSERT SELECT
> BEGIN;
> --- 44,50 ----
> WHEN MATCHED THEN
> UPDATE SET balance = t.balance + s.balance
> ;
> ! NOTICE: one tuple is ERROR
> ROLLBACK;
> -- do a simple equivalent of an INSERT SELECT
> BEGIN;
> ***************
> *** 61,66 ****
> --- 54,61 ----
> WHEN NOT MATCHED THEN
> INSERT VALUES (s.id, s.balance)
> ;
> + NOTICE: one tuple is ERROR
> + NOTICE: one tuple is ERROR
> SELECT * FROM target;
> id | balance
> ----+---------
> ***************
> *** 102,107 ****
> --- 97,103 ----
> WHEN MATCHED THEN
> DELETE
> ;
> + NOTICE: one tuple is ERROR
> SELECT * FROM target;
> id | balance
> ----+---------
> ***************
> *** 165,176 ****
> ERROR: multiple actions on single target row
>
> ROLLBACK;
> !
> -- This next SQL statement
> -- fails according to standard
> -- suceeds in PostgreSQL implementation by simply ignoring the second
> -- matching row since it activates no WHEN clause
> BEGIN;
> MERGE into target t
> USING (select * from source) AS s
> ON t.id = s.id
> --- 161,175 ----
> ERROR: multiple actions on single target row
>
> ROLLBACK;
> ! ERROR: syntax error at or near "ERROR"
> ! LINE 1: ERROR: multiple actions on single target row
> ! ^
> -- This next SQL statement
> -- fails according to standard
> -- suceeds in PostgreSQL implementation by simply ignoring the second
> -- matching row since it activates no WHEN clause
> BEGIN;
> + ERROR: current transaction is aborted, commands ignored until end of
> transaction block
> MERGE into target t
> USING (select * from source) AS s
> ON t.id = s.id
> ***************
> *** 179,184 ****
> --- 178,184 ----
> WHEN NOT MATCHED THEN
> INSERT VALUES (s.id, s.balance)
> ;
> + ERROR: current transaction is aborted, commands ignored until end of
> transaction block
> ROLLBACK;
> -- Now lets prepare the test data to generate 2 non-matching rows
> DELETE FROM source WHERE id = 3 AND balance = 5;
> ***************
> *** 188,195 ****
> ----+---------
> 2 | 5
> 3 | 20
> - 4 | 5
> 4 | 40
> (4 rows)
>
> -- This next SQL statement
> --- 188,195 ----
> ----+---------
> 2 | 5
> 3 | 20
> 4 | 40
> + 4 | 5
> (4 rows)
>
> -- This next SQL statement
> ***************
> *** 203,216 ****
> WHEN NOT MATCHED THEN
> INSERT VALUES (s.id, s.balance)
> ;
> SELECT * FROM target;
> id | balance
> ----+---------
> 1 | 10
> 2 | 20
> 3 | 30
> - 4 | 5
> 4 | 40
> (5 rows)
>
> ROLLBACK;
> --- 203,218 ----
> WHEN NOT MATCHED THEN
> INSERT VALUES (s.id, s.balance)
> ;
> + NOTICE: one tuple is ERROR
> + NOTICE: one tuple is ERROR
> SELECT * FROM target;
> id | balance
> ----+---------
> 1 | 10
> 2 | 20
> 3 | 30
> 4 | 40
> + 4 | 5
> (5 rows)
>
> ROLLBACK;
> ***************
> *** 225,239 ****
> WHEN NOT MATCHED AND s.balance > 100 THEN
> INSERT VALUES (s.id, s.balance)
> ;
> SELECT * FROM target;
> id | balance
> ----+---------
> 1 | 10
> 2 | 20
> 3 | 30
> ! |
> ! |
> ! (5 rows)
>
> ROLLBACK;
> -- This next SQL statement suceeds, but does nothing since there are
> --- 227,243 ----
> WHEN NOT MATCHED AND s.balance > 100 THEN
> INSERT VALUES (s.id, s.balance)
> ;
> + NOTICE: one tuple is ERROR
> + NOTICE: one tuple is ERROR
> + NOTICE: one tuple is ERROR
> + NOTICE: one tuple is ERROR
> SELECT * FROM target;
> id | balance
> ----+---------
> 1 | 10
> 2 | 20
> 3 | 30
> ! (3 rows)
>
> ROLLBACK;
> -- This next SQL statement suceeds, but does nothing since there are
> ***************
> *** 249,262 ****
> WHEN NOT MATCHED
> DO NOTHING
> ;
> SELECT * FROM target;
> ! id | balance
> ! ----+---------
> ! 1 | 10
> ! 2 | 20
> ! 3 | 30
> ! (3 rows)
> !
> ROLLBACK;
> --
> -- Weirdness
> --- 253,263 ----
> WHEN NOT MATCHED
> DO NOTHING
> ;
> + ERROR: syntax error at or near "DO"
> + LINE 7: DO NOTHING
> + ^
> SELECT * FROM target;
> ! ERROR: current transaction is aborted, commands ignored until end of
> transaction block
> ROLLBACK;
> --
> -- Weirdness
>
> ======================================================================
>
>
>


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: Greg Smith <greg(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-12 14:31:43
Message-ID: AANLkTimiZfs-hdsGQqhk+6ZFgXb8kv6x502vUVOJDGNc@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Aug 12, 2010 at 2:24 AM, Boxuan Zhai <bxzhai2010(at)gmail(dot)com> wrote:
> Sorry for the mismatch problem of regress. In fact, I am still unable to
> make the regression test run on my machine. When I try the command
> pg_regreess in /src/test/regress/, it always gives a FATAL error:

The intention is that you should run "make check" in that directory.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company


From: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-13 06:27:15
Message-ID: AANLkTi=7TEAvu8VhGKJdCzLHuf+1VVnoOR2hkK2JiWhi@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I have renewed the merge.sql and merge.out in regress. Please have a look.

Attachment Content-Type Size
merge_v103.tar application/x-tar 100.0 KB

From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-13 06:33:22
Message-ID: 4C64E732.205@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 13/08/10 09:27, Boxuan Zhai wrote:
> I have renewed the merge.sql and merge.out in regress. Please have a look.

Thanks.

Did you change the way DO INSTEAD rules are handled already?
http://archives.postgresql.org/pgsql-hackers/2010-08/msg00151.php

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Boxuan Zhai <bxzhai2010(at)gmail(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: MERGE command for inheritance
Date: 2010-08-13 08:25:47
Message-ID: AANLkTimmeSAFTEMO2MeJdg6uLH4QLzmuMj1L1sYDFwx2@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Aug 13, 2010 at 2:33 PM, Heikki Linnakangas <
heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> On 13/08/10 09:27, Boxuan Zhai wrote:
>
>> I have renewed the merge.sql and merge.out in regress. Please have a look.
>>
>
> Thanks.
>
> Did you change the way DO INSTEAD rules are handled already?
> http://archives.postgresql.org/pgsql-hackers/2010-08/msg00151.php
>
>
Yes. This mistake has been corrected a few editions ago. Now, the actions
replaced by INSTEAD rules will still catch tuples but do nothing for them.

> --
> Heikki Linnakangas
>
> EnterpriseDB http://www.enterprisedb.com
>