Horton
Menu

Thread

  1. MERGE command for inheritance

    Boxuan Zhai <bxzhai2010@gmail.com> — 2010-08-10T09:38:09Z

    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

  2. Re: MERGE command for inheritance

    Peter Eisentraut <peter_e@gmx.net> — 2010-08-10T14:03:02Z

    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.

  3. Re: MERGE command for inheritance

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-08-10T14:15:55Z

    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

  4. Re: MERGE command for inheritance

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-08-11T08:27:51Z

    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

  5. Re: MERGE command for inheritance

    Simon Riggs <simon@2ndquadrant.com> — 2010-08-11T08:45:09Z

    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

  6. Re: MERGE command for inheritance

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-08-11T10:25:13Z

    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

  7. Re: MERGE command for inheritance

    Simon Riggs <simon@2ndquadrant.com> — 2010-08-11T11:44:00Z

    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

  8. Re: MERGE command for inheritance

    Boxuan Zhai <bxzhai2010@gmail.com> — 2010-08-11T12:51:16Z

    On Wed, Aug 11, 2010 at 4:27 PM, Heikki Linnakangas <
heikki.linnakangas@enterprisedb.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
>

  9. Re: MERGE command for inheritance

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-08-11T12:53:12Z

    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

  10. Re: MERGE command for inheritance

    Simon Riggs <simon@2ndquadrant.com> — 2010-08-11T13:32:51Z

    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

  11. Re: MERGE command for inheritance

    Boxuan Zhai <bxzhai2010@gmail.com> — 2010-08-11T14:09:43Z

    On Wed, Aug 11, 2010 at 4:45 PM, Simon Riggs <simon@2ndquadrant.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
>
>

  12. Re: MERGE command for inheritance

    Robert Haas <robertmhaas@gmail.com> — 2010-08-11T14:14:37Z

    On Wed, Aug 11, 2010 at 10:09 AM, Boxuan Zhai <bxzhai2010@gmail.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

  13. Re: MERGE command for inheritance

    Simon Riggs <simon@2ndquadrant.com> — 2010-08-11T14:45:24Z

    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

  14. Re: MERGE command for inheritance

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-08-11T14:52:24Z

    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

  15. Re: MERGE command for inheritance

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-11T15:03:13Z

    Simon Riggs <simon@2ndQuadrant.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

  16. Re: MERGE command for inheritance

    Simon Riggs <simon@2ndquadrant.com> — 2010-08-11T15:23:27Z

    On Wed, 2010-08-11 at 11:03 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.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

  17. Re: MERGE command for inheritance

    Robert Haas <robertmhaas@gmail.com> — 2010-08-11T15:29:39Z

    On Wed, Aug 11, 2010 at 11:23 AM, Simon Riggs <simon@2ndquadrant.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

  18. Re: MERGE command for inheritance

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-11T15:38:07Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Aug 11, 2010 at 11:23 AM, Simon Riggs <simon@2ndquadrant.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

  19. Re: MERGE command for inheritance

    Greg Smith <greg@2ndquadrant.com> — 2010-08-11T16:49:05Z

    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@2ndQuadrant.com   www.2ndQuadrant.us

  20. Re: MERGE command for inheritance

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-08-11T18:09:17Z

    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@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  21. Re: MERGE command for inheritance

    Boxuan Zhai <bxzhai2010@gmail.com> — 2010-08-12T06:24:55Z

    On Thu, Aug 12, 2010 at 12:49 AM, Greg Smith <greg@2ndquadrant.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@2ndQuadrant.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
>
> ======================================================================
>
>
>

  22. Re: MERGE command for inheritance

    Robert Haas <robertmhaas@gmail.com> — 2010-08-12T14:31:43Z

    On Thu, Aug 12, 2010 at 2:24 AM, Boxuan Zhai <bxzhai2010@gmail.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

  23. Re: MERGE command for inheritance

    Boxuan Zhai <bxzhai2010@gmail.com> — 2010-08-13T06:27:15Z

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

  24. Re: MERGE command for inheritance

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-08-13T06:33:22Z

    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

  25. Re: MERGE command for inheritance

    Boxuan Zhai <bxzhai2010@gmail.com> — 2010-08-13T08:25:47Z

    On Fri, Aug 13, 2010 at 2:33 PM, Heikki Linnakangas <
heikki.linnakangas@enterprisedb.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
>