Skip site navigation (1) Skip section navigation (2)

Re: Writeable CTE patch

From: Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Writeable CTE patch
Date: 2009-11-30 18:43:51
Message-ID: 4B141267.2030107@cs.helsinki.fi (view raw or flat)
Thread:
Lists: pgsql-hackers
Tom Lane wrote:
> 1. I thought we'd agreed at
> http://archives.postgresql.org/pgsql-hackers/2009-10/msg00558.php
> that the patch should support WITH on DML statements, eg
> 	with (some-query) insert into foo ...
> This might not take much more than grammar additions, but it's
> definitely lacking at the moment.

Ok, I added these.

> One thing that really does have to draw an error is that AFAIR the current
> rule feature doesn't enforce that a rewritten query produce the same type
> of output that the original would have.  We just ship off whatever the
> results are to the client, and let it sort everything out.  In a DML WITH
> query, though, I think we do have to insist that the rewritten query(s)
> still produce the same RETURNING rowtype as before.

Agreed.

> 3. I'm pretty unimpressed with the code added to ExecutePlan.  It knows
> way more than it ought to about CTEs, and yet I don't think it's doing the
> right things anyway --- in particular, won't it run the "leader" CTE more
> than once if one CTE references another?

Yes.  Are you suggesting something more intelligent to avoid scanning
the CTE more than once or..?

> I think it would be better if
> the PlannedStmt representation just told ExecutePlan what to do, rather
> than having all these heuristics inside ExecutePlan.

Yup, seems like a better choice.

> (BTW, I also think
> it would work better if you had the CommandCounterIncrement at the bottom
> of the loop, after the subquery execution not before it.  But I'm not sure
> it's safe for ExecutePlan to be modifying the snapshot it's handed anyway.)

Agreed.  I'm a bit lost here with the snapshot business; is doing this
work in ExecutePlan() out of the question or is it just that what I'm
doing is wrong?

> 4. As previously noted, the changes to avoid using es_result_relation_info
> are broken and need to be dropped from the patch.

Done.  I kept the logic for result relations to allow nested ModifyTable
nodes, but I don't think it ever did the right thing with EvalPlanQual()
and nested nodes.  I'll have think about that.


Regards,
Marko Tiikkaja

In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2009-11-30 18:48:10
Subject: Re: Deleted WAL files held open by backends in Linux
Previous:From: Tom LaneDate: 2009-11-30 18:29:34
Subject: Re: lexeme ordering in tsvector

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group