Re: Using results from INSERT ... RETURNING

Hello.

Here's a patch(WIP) that implements INSERT .. RETURNING inside a CTE.
Should apply cleanly against CVS head.

The INSERT query isn't rewritten so rules and default values don't work.
Recursive CTEs don't work either.

Regards,
Marko Tiikkaja

On Tuesday 07 July 2009 23:31:54 Marko Tiikkaja wrote:
> Here's a patch(WIP) that implements INSERT .. RETURNING inside a CTE.

Could you supply some test cases to illustrate what this patch accomplishes?

On Fri, Jul 17, 2009 at 10:42:02AM +0300, Peter Eisentraut wrote:
> On Tuesday 07 July 2009 23:31:54 Marko Tiikkaja wrote:
> > Here's a patch(WIP) that implements INSERT .. RETURNING inside a CTE.
>
> Could you supply some test cases to illustrate what this patch accomplishes?

postgres:54321=# CREATE TABLE t(i INTEGER);
CREATE TABLE

postgres:54321=# WITH t1 AS (
INSERT INTO t VALUES (1),(2),(3)
RETURNING 'INSERT', i
) SELECT * FROM t1;
?column? | i
----------+---
INSERT | 1
INSERT | 2
INSERT | 3
(3 rows)

Not working yet:

CREATE TABLE t(i SERIAL PRIMARY KEY);
NOTICE: CREATE TABLE will create implicit sequence "t_i_seq" for serial column "t.i"
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "t_pkey" for table "t"
CREATE TABLE

postgres:54321=# WITH t1 AS (INSERT INTO t VALUES
(DEFAULT),(DEFAULT),(DEFAULT) RETURNING 'INSERT', i) SELECT * FROM t1;
ERROR: unrecognized node type: 337

Also planned, but no code written yet:

UPDATE ... RETURNING
DELETE ... RETURNING

UNION [ALL] of each of INSERT, UPDATE, and DELETE...RETURNING inside the
CTE, analogous to recursive CTEs with SELECT.

Way Out There Possibility: mix'n'match recursion.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Jul 7, 2009 at 3:31 PM, Marko
Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> Hello.
>
> Here's a patch(WIP) that implements INSERT .. RETURNING inside a CTE. Should
> apply cleanly against CVS head.
>
> The INSERT query isn't rewritten so rules and default values don't work.
> Recursive CTEs don't work either.
>

my questions first:
- what's the use case for this?
- why you need a node InsertReturning (see nodeInsertReturning.c) at all?
- if we will support this, shouldn't we supporting INSERT RETURNING
inside subqueries too?

and it crashes for triggers (example using regression's int4_tbl)

create function trg_int4_tbl() returns trigger as $$
begin
raise notice 'ejecutando';
return new;
end;
$$ language plpgsql;

create trigger trig_int4_tbl before insert on int4_tbl for each row
execute procedure trg_int4_tbl();

with
q as (insert into int4_tbl select generate_series(1, 5) returning *)
select * from q;
NOTICE: ejecutando
LOG: server process (PID 20356) was terminated by signal 11: Segmentation fault
LOG: terminating any other active server processes
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: FATAL: the
database system is in recovery mode
Failed.
!> LOG: all server processes terminated; reinitializing

and for defaults (even if i'm providing the values, actually is worse
in that case)

CREATE TABLE t(i SERIAL PRIMARY KEY);

with
t1 as (insert into t values (default), (default), (default)
returning 'INSERT', i)
select * from t1;
ERROR: unrecognized node type: 337

with
t1 as (insert into t values (1), (2), (3) returning 'INSERT', i)
select * from t1;
LOG: server process (PID 21604) was terminated by signal 11: Segmentation fault
LOG: terminating any other active server processes
LOG: all server processes terminated; reinitializing
LOG: database system was interrupted; last known up at 2009-07-18 15:29:28 ECT
LOG: database system was not properly shut down; automatic recovery in progress
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: FATAL: the
database system is in recovery mode
Failed.
!> LOG: redo starts at 0/32A0310

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

On Sat, Jul 18, 2009 at 5:21 PM, Jaime
Casanova<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
> my questions first:
> - what's the use case for this?

Being able to use 'returning' in a subquery is probably the #1 most
requested feature for postgresql (it's also a todo). Solving it for
'with' queries is a nice step in the right direction, and sidesteps
some of the traps that result from the general case. There are many
obvious ways this feature is helpful...here's a couple:

move records from one table to another:
with foo as (delete from bar where something returning *) insert
insert into baz select foo.*:

gather defaulted values following an insert for later use:
with foo as (insert into bar(field) select 'hello' from
generate_series(1,n) returning *) insert into baz select foo.*;

merlin

On Sat, Jul 18, 2009 at 6:25 PM, Merlin Moncure<mmoncure(at)gmail(dot)com> wrote:
> On Sat, Jul 18, 2009 at 5:21 PM, Jaime
> Casanova<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>> my questions first:
>> - what's the use case for this?
>
> Being able to use 'returning' in a subquery is probably the #1 most
> requested feature for postgresql (it's also a todo). Solving it for
> 'with' queries is a nice step in the right direction, and sidesteps
> some of the traps that result from the general case.

ah! that's why i asked: 'if we will support this, shouldn't we
supporting INSERT RETURNING inside subqueries too?'
i'm not too confident with the code but i think the problems for both
cases have to be similar so if we solve one, why not the other?

>
> move records from one table to another:
> with foo as (delete from bar where something returning *) insert
> insert into baz select foo.*:
>

seems like a corner case...

> gather defaulted values following an insert for later use:
> with foo as (insert into bar(field) select 'hello' from
> generate_series(1,n) returning *)  insert into baz select foo.*;
>

ok

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec> writes:
> On Sat, Jul 18, 2009 at 6:25 PM, Merlin Moncure<mmoncure(at)gmail(dot)com> wrote:
>> Being able to use 'returning' in a subquery is probably the #1 most
>> requested feature for postgresql (it's also a todo). Solving it for
>> 'with' queries is a nice step in the right direction, and sidesteps
>> some of the traps that result from the general case.

> ah! that's why i asked: 'if we will support this, shouldn't we
> supporting INSERT RETURNING inside subqueries too?'

We've been over that: when will you fire triggers? What happens if the
outer query doesn't want to read the whole output of the DML command,
or wants to read it more than once?

If the DML command is in a single-evaluation WITH clause at the top
level of the command, then it's reasonable to identify the outer
command's own begin and end as the times to fire triggers; and there is
no issue about partial or repeated evaluation. If it's in a subquery
then things get much messier and more poorly defined.

Note that it can't just be "a WITH clause". It has to be one at the top
query level, or the problem comes right back.

regards, tom lane

On Sat, Jul 18, 2009 at 5:21 PM, Jaime
Casanova<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
> On Tue, Jul 7, 2009 at 3:31 PM, Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
>> [...] rules and default values don't work.
>> Recursive CTEs don't work either.
[...]
> and it crashes for triggers

I think this is a great feature, and it would be REALLY great if it
supported UPDATE and DELETE as well. DELETE in particular seems like
a really useful case for, e.g., moving records between two partitions
of a partitioned table.

However, it sounds to me like this is going to need more reworking
than is going to get done in the next week or two. I would encourage
the Marko (the patch author) to ask any specific questions he may have
so we can try to get him some assistance in resolving them, and then I
think we should mark this "Returned with feedback".

...Robert

Jaime Casanova wrote:
> - why you need a node InsertReturning (see nodeInsertReturning.c) at all?
>

I couldn't come up with a better way to do this.

> and it crashes for triggers (example using regression's int4_tbl)
>

Right. I never tested this with triggers. The trigger tuple slot isn't
allocated in InitPlan(). Seems to be one of the many places where the
code isn't aware that there can be a non-top-level DML statement. Thanks
for testing.

Regards,
Marko Tiikkaja

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think this is a great feature, and it would be REALLY great if it
> supported UPDATE and DELETE as well.

It won't get applied until it does, and I imagine the patch author
wasn't expecting any differently. The submission was clearly marked
"WIP" not "ready to apply".

> However, it sounds to me like this is going to need more reworking
> than is going to get done in the next week or two.

Yeah. I did a quick scan of the patch and was distressed at how much of
it seemed to be addition of new code; that implies that he's more or
less duplicated the top-level executor processing.

The way that I think this should be approached is
(1) a code-refactoring patch that moves INSERT/UPDATE/DELETE control
into plan nodes; then
(2) a feature patch that makes use of that to expose RETURNING in CTEs.

One thing that's not totally clear to me is whether we'd like to use
control plan nodes all the time, or only for statements with RETURNING.
The nice thing about the latter approach is that there's a well-defined
meaning for the plan node's targetlist. (Its input is the stuff to be
stored, its output is the RETURNING result; much cleaner than the way
RETURNING is bolted onto the executor now.) If we do it all the time
then the control nodes would have dummy targetlists for non-RETURNING
cases, which is a little bit ugly. OTOH it may not be practical to
handle it like that without a lot of code duplication.

Another thing to think about is whether there should be three types
of control nodes or only one.

But anyway, my $0.02 is that the *first* order of business is to propose
a suitable refactoring of execMain.c. We skipped doing that when we
first added RETURNING, but it's time to make it happen. Not fasten
another kluge on top of a kluge.

regards, tom lane

On Sun, Jul 19, 2009 at 12:39 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I think this is a great feature, and it would be REALLY great if it
>> supported UPDATE and DELETE as well.
>
> It won't get applied until it does, and I imagine the patch author
> wasn't expecting any differently.  The submission was clearly marked
> "WIP" not "ready to apply".
>
>> However, it sounds to me like this is going to need more reworking
>> than is going to get done in the next week or two.
>
> Yeah.  I did a quick scan of the patch and was distressed at how much of
> it seemed to be addition of new code; that implies that he's more or
> less duplicated the top-level executor processing.
>
> The way that I think this should be approached is
> (1) a code-refactoring patch that moves INSERT/UPDATE/DELETE control
> into plan nodes; then
> (2) a feature patch that makes use of that to expose RETURNING in CTEs.
>
> One thing that's not totally clear to me is whether we'd like to use
> control plan nodes all the time, or only for statements with RETURNING.
> The nice thing about the latter approach is that there's a well-defined
> meaning for the plan node's targetlist.  (Its input is the stuff to be
> stored, its output is the RETURNING result; much cleaner than the way
> RETURNING is bolted onto the executor now.)  If we do it all the time
> then the control nodes would have dummy targetlists for non-RETURNING
> cases, which is a little bit ugly.  OTOH it may not be practical to
> handle it like that without a lot of code duplication.
>
> Another thing to think about is whether there should be three types
> of control nodes or only one.
>
> But anyway, my $0.02 is that the *first* order of business is to propose
> a suitable refactoring of execMain.c.  We skipped doing that when we
> first added RETURNING, but it's time to make it happen.  Not fasten
> another kluge on top of a kluge.

Tom,

This is a great review and great input. I wish there were enough
hours in the day for you to do something like this for every patch. I
feel good about saying that this is Returned With Feedback at this
point, and I see that Jaime Casanova has already made that update.

Thanks very much,

...Robert

On 7/19/2009, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The way that I think this should be approached is
> (1) a code-refactoring patch that moves INSERT/UPDATE/DELETE control
> into plan nodes; then
> (2) a feature patch that makes use of that to expose RETURNING in CTEs.

I've been working on this and here's a patch I came up with. It's a
WIP that tries to
put together my thoughts about this. It works only for INSERT and DELETE
and
probably breaks with triggers.

In the attached patch, an Append node isn't added for inheritance sets.
Instead,
the UPDATE/DELETE node tries to take care of choosing the correct result
relation.
I tried to keep estate->es_result_relations as it is in order not to
break anything that
relies on it (for example ExecRelationIsTargetRelation) but
estate->es_result_relation_info
doesn't point to the target relation of the DML node any more. This was
replaced with
a pointer in DmlState to make it possible to add more DML nodes in the
future. Also
the result relation info initialization was moved to the node, because
InitResultRelInfo
needs to know the type of operation that is going to be performed on the
result relation.
Currently that info isn't available at the top level, so I went this
way. I'm not happy with it,
but couldn't come up with better ideas. Currently the result relation
for SELECT FOR
UPDATE/SHARE isn't initialized anywhere, so that won't work.

The patch doesn't do this, but the idea was that if the DML node has a
RETURNING
clause, the node returns the projected tuple and ExecutePlan sends it to
the DestReceiver.
In cases where there is no RETURNING clause, the node would return a
dummy tuple.

Comments are welcome.

Regards,
Marko Tiikkaja

On 7/31/2009, "Marko Tiikkaja" <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> ..

I seem to be having problems with my email client. The patch should be
attached this time. Sorry for the noise.

Regards,
Marko Tiikkaja

Hi,

This WIP patch refactors the executor by creating nodes for DML (INSERT,
UPDATE, DELETE). It is designed both to clean up the executor and to
help with making it possible to use (INSERT|UPDATE|DELETE) ...RETURNING
inside a WITH clause. At first I thought about removing
PlannedStmt::returningLists, but there are a couple of places where it's
still used, and having it there won't hurt so I didn't touch it.
ExecInitDml() could still be better.

Does anyone see something seriously wrong with it? Ideas and further
improvements are welcome too.

Attached to the upcoming commitfest.

Regards,
Marko Tiikkaja

Hi,

Fixed a couple of bugs and renovated ExecInitDml() a bit. Patch attached.

Regards,
Marko Tiikkaja

On Sun, Sep 6, 2009 at 6:10 AM, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> Fixed a couple of bugs and renovated ExecInitDml() a bit.  Patch attached.

Hi, I'm reviewing this patch for this CommitFest.

With regard to the changes in explain.c, I think that the way you've
capitalized INSERT, UPDATE, and DELETE is not consistent with our
usual style for labelling nodes. Also, you've failed to set sname, so
this reads from uninitialized memory when using JSON or XML format. I
think that you should handle XML/JSON format by setting sname to "Dml"
and then emit an "operation" field down around where we do if
(strategy) ExplainPropertyText("Strategy", ...).

I am not sure that I like the name Dml for the node type. Most of our
node types are descriptions of the action that will be performed, like
Sort or HashJoin; Dml is the name of the feature we're trying to
implement, but it's not the name of the action we're performing. Not
sure what would be better, though. Write? Modify?

Can you explain the motivation for changing the Append stuff as part
of this patch? It's not immediately clear to me why that needs to be
done as part of this patch or what we get out of it.

What is your general impression about the level of maturity of this
code? Are you submitting this as complete and ready for commit, or is
it a WIP? If the latter, what are the known issues?

I'll try to provide some more feedback on this after I look it over some more.

...Robert

(Sorry, forgot to CC hackers)

Robert Haas wrote:
> With regard to the changes in explain.c, I think that the way you've
> capitalized INSERT, UPDATE, and DELETE is not consistent with our
> usual style for labelling nodes. Also, you've failed to set sname, so
> this reads from uninitialized memory when using JSON or XML format. I
> think that you should handle XML/JSON format by setting sname to "Dml"
> and then emit an "operation" field down around where we do if
> (strategy) ExplainPropertyText("Strategy", ...).

You're right, I should fix that.

> I am not sure that I like the name Dml for the node type. Most of our
> node types are descriptions of the action that will be performed, like
> Sort or HashJoin; Dml is the name of the feature we're trying to
> implement, but it's not the name of the action we're performing. Not
> sure what would be better, though. Write? Modify?

Dml was the first name I came up with and it stuck, but it could be
better. I don't really like Write or Modify either.

> Can you explain the motivation for changing the Append stuff as part
> of this patch? It's not immediately clear to me why that needs to be
> done as part of this patch or what we get out of it.

It seemed to me that the Append on top was only a workaround for the
fact that we didn't have a node for DML operations that would select the
correct result relation. I don't see why an Append node should do this
at all if we have a special node for handling DML.

> What is your general impression about the level of maturity of this
> code? Are you submitting this as complete and ready for commit, or is
> it a WIP? If the latter, what are the known issues?

Aside from the EXPLAIN stuff you brought up, there are no issues that
I'm aware of. There are a few spots that could be prettier, but I have
no good ideas for them.

> I'll try to provide some more feedback on this after I look it over some more.

Thanks!

Regards,
Marko Tiikkaja

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> Robert Haas wrote:
>> Can you explain the motivation for changing the Append stuff as part
>> of this patch? It's not immediately clear to me why that needs to be
>> done as part of this patch or what we get out of it.

> It seemed to me that the Append on top was only a workaround for the
> fact that we didn't have a node for DML operations that would select the
> correct result relation. I don't see why an Append node should do this
> at all if we have a special node for handling DML.

The stuff for inherited target relations is certainly ugly, and if this
can clean it up then so much the better ... but is a DML node that has
to deal with multiple targets really better? It's not only the Append
itself that's funny, it's the fact that the generated tuples don't all
have the same tupdesc in UPDATE cases.

FWIW, I'd think of having three separate node types Insert, Update,
Delete, not a combined Dml node. The behavior and the required inputs
are sufficiently different that I don't think a combined node type
is saving much. And it'd avoid the "what is that?" problem.

regards, tom lane

On Tue, Sep 22, 2009 at 11:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
>> Robert Haas wrote:
>>> Can you explain the motivation for changing the Append stuff as part
>>> of this patch?  It's not immediately clear to me why that needs to be
>>> done as part of this patch or what we get out of it.
>
>> It seemed to me that the Append on top was only a workaround for the
>> fact that we didn't have a node for DML operations that would select the
>> correct result relation.  I don't see why an Append node should do this
>> at all if we have a special node for handling DML.
>
> The stuff for inherited target relations is certainly ugly, and if this
> can clean it up then so much the better ... but is a DML node that has
> to deal with multiple targets really better?  It's not only the Append
> itself that's funny, it's the fact that the generated tuples don't all
> have the same tupdesc in UPDATE cases.
>
>
> FWIW, I'd think of having three separate node types Insert, Update,
> Delete, not a combined Dml node.  The behavior and the required inputs
> are sufficiently different that I don't think a combined node type
> is saving much.  And it'd avoid the "what is that?" problem.

Right now, it looks like most of the code is being shared between all
three plan types. I'm pretty suspicious of how much code this patch
moves around and how little of it is actually changed. I can't really
tell if there's an actual design improvement here or if this is all
window-dressing.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Right now, it looks like most of the code is being shared between all
> three plan types. I'm pretty suspicious of how much code this patch
> moves around and how little of it is actually changed. I can't really
> tell if there's an actual design improvement here or if this is all
> window-dressing.

My recollection is that we *told* Marko to set things up so that the
first patch was mainly just code refactoring. So your second sentence
doesn't surprise me. As to the third, I've not looked at the patch,
but perhaps it needs to expend more effort on documentation?

regards, tom lane

On Tue, Sep 22, 2009 at 11:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Right now, it looks like most of the code is being shared between all
>> three plan types.  I'm pretty suspicious of how much code this patch
>> moves around and how little of it is actually changed.  I can't really
>> tell if there's an actual design improvement here or if this is all
>> window-dressing.
>
> My recollection is that we *told* Marko to set things up so that the
> first patch was mainly just code refactoring.  So your second sentence
> doesn't surprise me.  As to the third, I've not looked at the patch,
> but perhaps it needs to expend more effort on documentation?

Well, part of the problem is that I've not had a lot of luck trying to
understand how the executor really works (what's a tuple table slot
and why do we need to know in advance how many of them there are?).
There's this fine comment, which has been in
src/backend/executor/README for 8 years and change:

XXX a great deal more documentation needs to be written here...

However, that's not the whole problem, either. To your point about
documentation, it seems this path doesn't touch the README at all, and
it needs to, because some of the statements in that file would
certainly become false were this patch to be applied.

So I think we should at a minimum ask the patch author to (1) fix the
explain bugs I found and (2) update the README, as well as (3) revert
needless whitespace changes - there are a couple in execMain.c, from
the looks of it.

However, before he spends too much more time on this feature, it would
probably be good for you to take a quick scan through the patch and
see what you think of the general approach. I don't think I'm
qualified to judge.

...Robert

Robert Haas wrote:
> However, that's not the whole problem, either. To your point about
> documentation, it seems this path doesn't touch the README at all, and
> it needs to, because some of the statements in that file would
> certainly become false were this patch to be applied.
>
> So I think we should at a minimum ask the patch author to (1) fix the
> explain bugs I found and (2) update the README, as well as (3) revert
> needless whitespace changes - there are a couple in execMain.c, from
> the looks of it.

I overlooked the README completely. I'll see what I can do about these
and submit an updated patch in a couple of days.

Regards,
Marko Tiikkaja

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Well, part of the problem is that I've not had a lot of luck trying to
> understand how the executor really works (what's a tuple table slot
> and why do we need to know in advance how many of them there are?).

You know, that's actually a really good question. There is not, as far
as I can see, much advantage to keeping the Slots in an array. We could
perfectly well keep them in a List and eliminate the notational overhead
of having to count them in advance. There'd be a bit more palloc
overhead (for the list cells) but avoiding the counting work would more
or less make up for that.

regards, tom lane

On Sun, Sep 27, 2009 at 12:40 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Well, part of the problem is that I've not had a lot of luck trying to
>> understand how the executor really works (what's a tuple table slot
>> and why do we need to know in advance how many of them there are?).
>
> You know, that's actually a really good question.  There is not, as far
> as I can see, much advantage to keeping the Slots in an array.  We could
> perfectly well keep them in a List and eliminate the notational overhead
> of having to count them in advance.  There'd be a bit more palloc
> overhead (for the list cells) but avoiding the counting work would more
> or less make up for that.

Heh. I was actually asking an even stupider question, which is why do
we need to keep all of them in ANY centrally known data structure?
What operation do we perform that requires us to find all of the
exstant TTS?

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Heh. I was actually asking an even stupider question, which is why do
> we need to keep all of them in ANY centrally known data structure?
> What operation do we perform that requires us to find all of the
> exstant TTS?

ExecDropTupleTable is used to release slot-related buffer pins and
tupdesc refcounts at conclusion of a query. I suppose we could require
the individual plan node End routines to do it instead. Not sure if
that's an improvement.

regards, tom lane

Robert Haas wrote:
> So I think we should at a minimum ask the patch author to (1) fix the
> explain bugs I found and (2) update the README, as well as (3) revert
> needless whitespace changes - there are a couple in execMain.c, from
> the looks of it.

In the attached patch, I made the changes to explain as you suggested
and reverted the only whitespace change I could find from execMain.c.
However, English isn't my first language so I'm not very confident about
fixing the README.

Regards,
Marko Tiikkaja

On Mon, Sep 28, 2009 at 11:31 AM, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> Robert Haas wrote:
>>
>> So I think we should at a minimum ask the patch author to (1) fix the
>> explain bugs I found and (2) update the README, as well as (3) revert
>> needless whitespace changes - there are a couple in execMain.c, from
>> the looks of it.
>
> In the attached patch, I made the changes to explain as you suggested and
> reverted the only whitespace change I could find from execMain.c. However,
> English isn't my first language so I'm not very confident about fixing the
> README.

Can you at least take a stab at it? We can fix your grammar, but
guessing what's going on without documentation is hard.

...Robert

Robert Haas wrote:
> Can you at least take a stab at it? We can fix your grammar, but
> guessing what's going on without documentation is hard.

With some help from David Fetter, I took another try at it. I hope
someone finds this helpful. I'm happy to answer any questions.

Regards,
Marko Tiikkaja

Marko Tiikkaja escribió:
> Robert Haas wrote:
> >So I think we should at a minimum ask the patch author to (1) fix the
> >explain bugs I found and (2) update the README, as well as (3) revert
> >needless whitespace changes - there are a couple in execMain.c, from
> >the looks of it.
>
> In the attached patch, I made the changes to explain as you
> suggested and reverted the only whitespace change I could find from
> execMain.c. However, English isn't my first language so I'm not very
> confident about fixing the README.

BTW what was the conclusion of the idea about having three separate
nodes Insert, Delete, Update instead of a single Dml node?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> BTW what was the conclusion of the idea about having three separate
> nodes Insert, Delete, Update instead of a single Dml node?

If we stick with a single node type then I'd suggest calling it
something like ModifyTable.

regards, tom lane

On Tue, Sep 29, 2009 at 11:29 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Marko Tiikkaja escribió:
>> Robert Haas wrote:
>> >So I think we should at a minimum ask the patch author to (1) fix the
>> >explain bugs I found and (2) update the README, as well as (3) revert
>> >needless whitespace changes - there are a couple in execMain.c, from
>> >the looks of it.
>>
>> In the attached patch, I made the changes to explain as you
>> suggested and reverted the only whitespace change I could find from
>> execMain.c. However, English isn't my first language so I'm not very
>> confident about fixing the README.
>
> BTW what was the conclusion of the idea about having three separate
> nodes Insert, Delete, Update instead of a single Dml node?

It wasn't obvious from reading the patch why multiple node types would
be superior; but I'm not 100% sure I understand what Tom had in mind,
either.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Sep 29, 2009 at 11:29 AM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> BTW what was the conclusion of the idea about having three separate
>> nodes Insert, Delete, Update instead of a single Dml node?

> It wasn't obvious from reading the patch why multiple node types would
> be superior; but I'm not 100% sure I understand what Tom had in mind,
> either.

I thought they would be simpler/faster individually. However, that
might not be enough to outweigh 3x repetition of the per-node-type
boilerplate. I haven't read the patch yet so this isn't an informed
recommendation, just a suggestion of an alternative to consider.

regards, tom lane

On Mon, Sep 28, 2009 at 3:19 PM, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> Robert Haas wrote:
>>
>> Can you at least take a stab at it?  We can fix your grammar, but
>> guessing what's going on without documentation is hard.
>
> With some help from David Fetter, I took another try at it.  I hope
> someone finds this helpful.  I'm happy to answer any questions.

Thanks. I read through this patch some more tonight and I guess I am
a bit confused about what it accomplishes. AIUI, the point here is to
lay the groundwork for a future patch to allow writeable CTEs, and I
guess I'm not understanding how it's going to do that.

rhaas=# create table project (id serial primary key, name varchar not
null);NOTICE: CREATE TABLE will create implicit sequence
"project_id_seq" for serial column "project.id"
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"project_pkey" for table "project"
CREATE TABLE
rhaas=# create table shadow (id integer not null primary key, name
varchar not null);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"shadow_pkey" for table "shadow"
CREATE TABLE
rhaas=# create rule clone as on insert to project do also insert into
shadow (id, name) values (NEW.id, NEW.name);
CREATE RULE
rhaas=# insert into project (name) values ('Writeable CTEs') returning id;
id
----
1
(1 row)

INSERT 0 1
rhaas=# explain insert into project (name) values ('Writeable CTEs')
returning id;
QUERY PLAN
------------------------------------------------
Insert (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)

Insert (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)
(5 rows)

Now the point here is that I eventually want to be able to write
something like this:

with foo as (insert into project (name) values ('Writeable CTEs')
returning id) select * from foo;

...but how does this get me any closer? It seems to me that the plan
for THAT statement has to be a CTE scan over top of BOTH of the
inserts, but here I have two insert nodes that comprise two separate
plans. The DML node, as presently implemented, supports a list of
plans, but they all have to be of the same type, so it's really only
useful for handling append, and as previously discussed, it's not
clear that the proposed handling is any better than what we already
have.

What am I missing?

...Robert

On Thu, Oct 01, 2009 at 10:48:41PM -0400, Robert Haas wrote:
> On Mon, Sep 28, 2009 at 3:19 PM, Marko Tiikkaja
> <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> > Robert Haas wrote:
> >>
> >> Can you at least take a stab at it?  We can fix your grammar, but
> >> guessing what's going on without documentation is hard.
> >
> > With some help from David Fetter, I took another try at it.  I
> > hope someone finds this helpful.  I'm happy to answer any
> > questions.
>
> Thanks. I read through this patch some more tonight and I guess I
> am a bit confused about what it accomplishes. AIUI, the point here
> is to lay the groundwork for a future patch to allow writeable CTEs,
> and I guess I'm not understanding how it's going to do that.

There's another branch in the repository
<http://git.postgresql.org/gitweb?p=writeable_cte.git> called
actually_write which has the beginnings of an implementation based on
this. If you'd like, I can send along either the patch vs.
writeable_cte, or a patch against the main branch.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Robert Haas wrote:
> Thanks. I read through this patch some more tonight and I guess I am
> a bit confused about what it accomplishes. AIUI, the point here is to
> lay the groundwork for a future patch to allow writeable CTEs, and I
> guess I'm not understanding how it's going to do that.

The purpose of this patch is only to move INSERT, UPDATE and DELETE into
plan nodes because for writeable CTEs, we can't use the currently
existing way of handling those operations. My previous approach was to
only add a special-case node for RETURNING and use the top-level
INSERT/UPDATE/DELETE handling like now, but that would've lead to
copying a lot of code, as Tom pointed out in
http://archives.postgresql.org/pgsql-hackers/2009-07/msg01217.php . In
that same message, Tom suggested the current approach which to me seems
like the best way to tackle this. This patch alone doesn't accomplish
anything, but supporting writeable CTEs will be a lot easier as seen in
the git repo David pointed you to.

Regards,
Marko Tiikkaja

Robert Haas wrote:
> Now the point here is that I eventually want to be able to write
> something like this:
>
> with foo as (insert into project (name) values ('Writeable CTEs')
> returning id) select * from foo;
>
> ...but how does this get me any closer? It seems to me that the plan
> for THAT statement has to be a CTE scan over top of BOTH of the
> inserts, but here I have two insert nodes that comprise two separate
> plans. The DML node, as presently implemented, supports a list of
> plans, but they all have to be of the same type, so it's really only
> useful for handling append, and as previously discussed, it's not
> clear that the proposed handling is any better than what we already
> have.

I don't think you should be able to do this. I'm not too familiar with
rules, but in your example the rule doesn't modify the output of the
INSERT .. RETURNING so I think it shouldn't do that here either. How I
see it is that in your example the INSERT INTO shadow would be added to
the top level instead of the CTE and the plan would look something like
this:

------------------------------------------------
CTE Scan on foo (cost=0.01..0.03 rows=1 width=4)
CTE foo
-> Insert (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)

Insert (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)

so you would get the RETURNING output from the CTE and the INSERT to the
shadow table would be executed separately. I'm not saying that we don't
want to provide the means to do this, but writeable CTEs alone aren't
meant to handle this.

Regards,
Marko Tiikkaja

On Fri, Oct 2, 2009 at 7:37 AM, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:
> Robert Haas wrote:
>>
>> Now the point here is that I eventually want to be able to write
>> something like this:
>>
>> with foo as (insert into project (name) values ('Writeable CTEs')
>> returning id) select * from foo;
>>
>> ...but how does this get me any closer?  It seems to me that the plan
>> for THAT statement has to be a CTE scan over top of BOTH of the
>> inserts, but here I have two insert nodes that comprise two separate
>> plans.  The DML node, as presently implemented, supports a list of
>> plans, but they all have to be of the same type, so it's really only
>> useful for handling append, and as previously discussed, it's not
>> clear that the proposed handling is any better than what we already
>> have.
>
> I don't think you should be able to do this.  I'm not too familiar with
> rules, but in your example the rule doesn't modify the output of the
> INSERT .. RETURNING so I think it shouldn't do that here either.  How I
> see it is that in your example the INSERT INTO shadow would be added to
> the top level instead of the CTE and the plan would look something like
> this:
>
> ------------------------------------------------
>  CTE Scan on foo  (cost=0.01..0.03 rows=1 width=4)
>   CTE foo
>     ->  Insert  (cost=0.00..0.01 rows=1 width=0)
>           ->  Result  (cost=0.00..0.01 rows=1 width=0)
>
>  Insert  (cost=0.00..0.01 rows=1 width=0)
>   ->  Result  (cost=0.00..0.01 rows=1 width=0)
>
> so you would get the RETURNING output from the CTE and the INSERT to the
> shadow table would be executed separately.

Yeah, I think you're right.

> I'm not saying that we don't
> want to provide the means to do this, but writeable CTEs alone aren't
> meant to handle this.

Well, I think a patch to implement writeable CTEs is probably going to
have to handle this case - I don't think we can just ignore rewrite
rules when processing a CTE. But it does seem to be beyond the scope
of the current patch.

I'm going to go ahead and mark this Ready for Committer. Thanks for
your patience.

...Robert

Marko -

I noticed something a little odd about the new append-plan handling.

rhaas=# explain update parent set c = 1;
QUERY PLAN
-----------------------------------------------------------------------
Update (cost=0.00..60.80 rows=4080 width=12)
-> Seq Scan on parent (cost=0.00..31.40 rows=2140 width=10)
-> Seq Scan on child parent (cost=0.00..29.40 rows=1940 width=14)
(3 rows)

That may be OK, actually, but it does look a little weird.

...Robert

On Sun, Oct 4, 2009 at 8:14 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Marko -
>
> I noticed something a little odd about the new append-plan handling.
>
> rhaas=# explain update parent set c = 1;
>                              QUERY PLAN
> -----------------------------------------------------------------------
>  Update  (cost=0.00..60.80 rows=4080 width=12)
>   ->  Seq Scan on parent  (cost=0.00..31.40 rows=2140 width=10)
>   ->  Seq Scan on child parent  (cost=0.00..29.40 rows=1940 width=14)
> (3 rows)
>
> That may be OK, actually, but it does look a little weird.

Argh. Nevermind. It was like that before.

Sigh.

...Robert

Robert Haas wrote:
>> I'm not saying that we don't
>> want to provide the means to do this, but writeable CTEs alone aren't
>> meant to handle this.
>
> Well, I think a patch to implement writeable CTEs is probably going to
> have to handle this case - I don't think we can just ignore rewrite
> rules when processing a CTE. But it does seem to be beyond the scope
> of the current patch.

My use of "this" was a bit ambiguous here; what I meant was that
writeable CTEs are going to work just like a top-level INSERT ..
RETURNING would have, i.e. return only rows inserted to "project".

> I'm going to go ahead and mark this Ready for Committer. Thanks for
> your patience.

Thanks for reviewing!

Regards,
Marko Tiikkaja

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Well, I think a patch to implement writeable CTEs is probably going to
> have to handle this case - I don't think we can just ignore rewrite
> rules when processing a CTE. But it does seem to be beyond the scope
> of the current patch.

I hadn't been paying too much attention to this thread, but ... why
is anyone worrying about that? Rewrite rules are not the concern
of either the planner or the executor. A "do also" rule will result
in two entirely separate Query trees, which will each be planned
separately and executed separately. Any given executor run only
has to think about one type of DML command --- otherwise the executor
would be broken already, since it takes only one command-type argument.

regards, tom lane

Tom Lane wrote:
> Any given executor run only
> has to think about one type of DML command --- otherwise the executor
> would be broken already, since it takes only one command-type argument.

If I understood you correctly, this would imply that you wouldn't be
able to do for example:

INSERT INTO foo
WITH t AS ( DELETE FROM bar RETURNING * )
SELECT * FROM t;

which is probably the most useful thing you could do with this feature.
Am I misinterpreting what you said?

Regards,
Marko Tiikkaja

On Oct 4, 2009, at 11:47 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Well, I think a patch to implement writeable CTEs is probably going
>> to
>> have to handle this case - I don't think we can just ignore rewrite
>> rules when processing a CTE. But it does seem to be beyond the scope
>> of the current patch.
>
> I hadn't been paying too much attention to this thread, but ... why
> is anyone worrying about that? Rewrite rules are not the concern
> of either the planner or the executor. A "do also" rule will result
> in two entirely separate Query trees, which will each be planned
> separately and executed separately. Any given executor run only
> has to think about one type of DML command --- otherwise the executor
> would be broken already, since it takes only one command-type
> argument.

If an INSERT/UPDATE/DELETE appears within a CTE, it will still need to
be rewritten. But you're right that this is irrelevant to the present
patch; I just didn't see that at once.

...Robert

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> If I understood you correctly, this would imply that you wouldn't be
> able to do for example:

> INSERT INTO foo
> WITH t AS ( DELETE FROM bar RETURNING * )
> SELECT * FROM t;

Um ... forget what I said --- not enough caffeine yet, apparently.

Yeah, rewrite rules are going to be a *serious* stumbling block to
this whole concept. Maybe we should just punt the project until we
have a clear idea of how to do that.

regards, tom lane

On Sun, Oct 04, 2009 at 01:16:50PM -0400, Tom Lane wrote:
> Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> > If I understood you correctly, this would imply that you wouldn't
> > be able to do for example:
>
> > INSERT INTO foo
> > WITH t AS ( DELETE FROM bar RETURNING * )
> > SELECT * FROM t;
>
> Um ... forget what I said --- not enough caffeine yet, apparently.
>
> Yeah, rewrite rules are going to be a *serious* stumbling block to
> this whole concept.

> Maybe we should just punt the project until we have a clear idea of
> how to do that.

Maybe rewrite rules just don't fit with this feature, and should cause
an error. We have other things that don't work together, and the
world hasn't ended yet.

This leads me to another Modest Proposal, which I'll detail in another
post.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Oct 4, 2009, at 1:24 PM, David Fetter <david(at)fetter(dot)org> wrote:

> On Sun, Oct 04, 2009 at 01:16:50PM -0400, Tom Lane wrote:
>> Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
>>> If I understood you correctly, this would imply that you wouldn't
>>> be able to do for example:
>>
>>> INSERT INTO foo
>>> WITH t AS ( DELETE FROM bar RETURNING * )
>>> SELECT * FROM t;
>>
>> Um ... forget what I said --- not enough caffeine yet, apparently.
>>
>> Yeah, rewrite rules are going to be a *serious* stumbling block to
>> this whole concept.
>
>> Maybe we should just punt the project until we have a clear idea of
>> how to do that.

This was discussed a bit upthread and doesn't seem obviously
unmanageable to me.

> Maybe rewrite rules just don't fit with this feature, and should cause
> an error. We have other things that don't work together, and the
> world hasn't ended yet.

That doesn't seem very satisfactory.

...Robert
>

Tom Lane wrote:
> Yeah, rewrite rules are going to be a *serious* stumbling block to
> this whole concept. Maybe we should just punt the project until we
> have a clear idea of how to do that.

I've implemented rewrite rules for writeable CTEs, and at least now I
don't see any problems except one. I can't seem to think of what would
be the correct behaviour in this case:

=> CREATE rule foo_rule AS ON INSERT TO foo DO ALSO SELECT * FROM bar;
CREATE RULE

=> WITH t AS (INSERT INTO foo VALUES(0) RETURNING *) SELECT * FROM t;

If you rewrite the query as it is rewritten in the top-level case, you
get a plan such as this:

-------------------------------------------------------
CTE Scan ON t (cost=0.01..0.03 rows=1 width=4)
CTE t
-> INSERT (cost=0.00..0.01 rows=1 width=0)
-> Result (cost=0.00..0.01 rows=1 width=0)

Seq Scan ON bar (cost=0.00..34.00 rows=2400 width=4)

and now you have *two* SELECT statements. Currently the portal code
gives the output of the "Seq Scan ON bar" here which is IMO very
surprising. Does ignoring the rule here sound sane or should we error
out? Or does someone have a better idea? DO ALSO INSERT/UPDATE/DELETE
works as expected here.

Regards,
Marko Tiikkaja

I wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> BTW what was the conclusion of the idea about having three separate
>> nodes Insert, Delete, Update instead of a single Dml node?

> If we stick with a single node type then I'd suggest calling it
> something like ModifyTable.

I'm starting to work on this patch now. After looking at it a bit,
it seems to me that keeping it as one node is probably marginally
preferable to making three nodes; but I still do not like the "Dml"
name. Does anyone have a problem with the ModifyTable suggestion,
or a better idea?

regards, tom lane

Tom Lane escribió:
> I wrote:
> > Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> >> BTW what was the conclusion of the idea about having three separate
> >> nodes Insert, Delete, Update instead of a single Dml node?
>
> > If we stick with a single node type then I'd suggest calling it
> > something like ModifyTable.
>
> I'm starting to work on this patch now. After looking at it a bit,
> it seems to me that keeping it as one node is probably marginally
> preferable to making three nodes; but I still do not like the "Dml"
> name. Does anyone have a problem with the ModifyTable suggestion,
> or a better idea?

Does it affect how is it going to look in EXPLAIN output?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Tom Lane wrote:
> Does anyone have a problem with the ModifyTable suggestion,
> or a better idea?

Lacking a better idea, +1 for ModifyTable from me.

Regards,
Marko Tiikkaja

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane escribi:
> Does it affect how is it going to look in EXPLAIN output?

Hmm, I suppose there's not any law that says EXPLAIN has to print the
same name we use internally. The explain output could say "Insert",
"Update", or "Delete" independently of what we call the node type.
That would take away about 50% of my objection to the node name,
though on balance I still think "Dml" is a poor choice since plan
node names are typically verbs.

One issue is whether it would be confusing for implementors if the
explain output is completely unrelated to the internal name. We could
do something like "InsertTable" or "ModifyTable Insert", but both of
these look a bit ugly from a user's standpoint I think.

I notice also that the patch has chosen to represent Dml in XML/JSON
explain output as Node Type = Dml with an entirely new attribute
Operation to indicate Insert/Update/Delete. Do we really want to
go there? Adding single-purpose attributes doesn't seem like a great
idea.

While we're discussing explain output ... what about triggers?
An important aspect of this change is that at least the row-level
triggers are now going to be charged as runtime of the
Dml-or-whatever-we-call-it node. Should we rearrange the explain
output so that the printout for trigger runtimes is associated
with those nodes too? If so, what about statement-level triggers?

regards, tom lane

On Thu, Oct 8, 2009 at 3:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Tom Lane escribió:
>> Does it affect how is it going to look in EXPLAIN output?
>
> Hmm, I suppose there's not any law that says EXPLAIN has to print the
> same name we use internally.  The explain output could say "Insert",
> "Update", or "Delete" independently of what we call the node type.

It already does, in text mode.

> That would take away about 50% of my objection to the node name,
> though on balance I still think "Dml" is a poor choice since plan
> node names are typically verbs.

Agreed.

> One issue is whether it would be confusing for implementors if the
> explain output is completely unrelated to the internal name.  We could
> do something like "InsertTable" or "ModifyTable Insert", but both of
> these look a bit ugly from a user's standpoint I think.
>
> I notice also that the patch has chosen to represent Dml in XML/JSON
> explain output as Node Type = Dml with an entirely new attribute
> Operation to indicate Insert/Update/Delete.  Do we really want to
> go there?  Adding single-purpose attributes doesn't seem like a great
> idea.

Well, I was the one who suggested doing it that way, so you can blame
me for that, but it is consistent with how we've handled other things,
like setops and jointypes: the details get moved to another tag so as
to avoid an explosive growth in the number of node types that clients
must be prepared for.

> While we're discussing explain output ... what about triggers?
> An important aspect of this change is that at least the row-level
> triggers are now going to be charged as runtime of the
> Dml-or-whatever-we-call-it node.  Should we rearrange the explain
> output so that the printout for trigger runtimes is associated
> with those nodes too?  If so, what about statement-level triggers?

That's not a bad idea, though it wouldn't bother me much if we left it
for a follow-on patch.

...Robert

Robert Haas escribió:
> On Thu, Oct 8, 2009 at 3:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> > I notice also that the patch has chosen to represent Dml in XML/JSON
> > explain output as Node Type = Dml with an entirely new attribute
> > Operation to indicate Insert/Update/Delete.  Do we really want to
> > go there?  Adding single-purpose attributes doesn't seem like a great
> > idea.
>
> Well, I was the one who suggested doing it that way, so you can blame
> me for that, but it is consistent with how we've handled other things,
> like setops and jointypes: the details get moved to another tag so as
> to avoid an explosive growth in the number of node types that clients
> must be prepared for.

Perhaps how a join is implemented in a plan can be considered a
"detail", but I don't think the same holds true for insert vs. update.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Robert Haas escribi:
>> On Thu, Oct 8, 2009 at 3:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I notice also that the patch has chosen to represent Dml in XML/JSON
>>> explain output as Node Type = Dml with an entirely new attribute
>>> Operation to indicate Insert/Update/Delete. Do we really want to
>>> go there? Adding single-purpose attributes doesn't seem like a great
>>> idea.
>>
>> Well, I was the one who suggested doing it that way, so you can blame
>> me for that, but it is consistent with how we've handled other things,
>> like setops and jointypes: the details get moved to another tag so as
>> to avoid an explosive growth in the number of node types that clients
>> must be prepared for.

> Perhaps how a join is implemented in a plan can be considered a
> "detail", but I don't think the same holds true for insert vs. update.

Also, in all the other cases we stuck the detail into a common
attribute called Strategy. What bothers me about Operation is that
there is only one node type that it is good for. I would prefer to
keep the text and XML representations of this the same, which at the
moment seems to mean that the node type should be reported as
Insert/Update/Delete.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Oct 8, 2009 at 3:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> While we're discussing explain output ... what about triggers?
>> An important aspect of this change is that at least the row-level
>> triggers are now going to be charged as runtime of the
>> Dml-or-whatever-we-call-it node. Should we rearrange the explain
>> output so that the printout for trigger runtimes is associated
>> with those nodes too? If so, what about statement-level triggers?

> That's not a bad idea, though it wouldn't bother me much if we left it
> for a follow-on patch.

After cogitating on this for a few minutes, it seems to me that the
right thing to do is to push the calling of statement-level triggers
into the Dml node too. That is, ExecDml() would be responsible for
calling BEFORE STATEMENT triggers at the start of its first call,
and for calling AFTER STATEMENT triggers at the end of its last
call (just before it returns NULL). This would mean that all trigger
activity is now associated with a plan node and should be reported
that way by EXPLAIN. We could get rid of the rather warty top-level
XML item for triggers and make the trigger instrumentation outputs
just be another plan node property.

Aside from having a cleaner EXPLAIN output design, this would mean that
triggers in the planned WITH RETURNING scenario fire in the order you'd
expect if we are considering the WITH RETURNING queries to be done
sequentially and before the main query. From a user's standpoint it
would look just like you'd issued the queries sequentially, except that
the RETURNING data is held for use in the main query.

regards, tom lane

On Thu, Oct 8, 2009 at 3:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Robert Haas escribió:
>>> On Thu, Oct 8, 2009 at 3:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> I notice also that the patch has chosen to represent Dml in XML/JSON
>>>> explain output as Node Type = Dml with an entirely new attribute
>>>> Operation to indicate Insert/Update/Delete.  Do we really want to
>>>> go there?  Adding single-purpose attributes doesn't seem like a great
>>>> idea.
>>>
>>> Well, I was the one who suggested doing it that way, so you can blame
>>> me for that, but it is consistent with how we've handled other things,
>>> like setops and jointypes: the details get moved to another tag so as
>>> to avoid an explosive growth in the number of node types that clients
>>> must be prepared for.
>
>> Perhaps how a join is implemented in a plan can be considered a
>> "detail", but I don't think the same holds true for insert vs. update.
>
> Also, in all the other cases we stuck the detail into a common
> attribute called Strategy.  What bothers me about Operation is that
> there is only one node type that it is good for.  I would prefer to
> keep the text and XML representations of this the same, which at the
> moment seems to mean that the node type should be reported as
> Insert/Update/Delete.

Hmm... well, when you initially committed the code, you complained
that there were two different and unrelated things both using the
strategy tag - really, to mean different things. Now you're saying
that strategy is OK because it's grand-fathered, but we shouldn't add
any more.

I think there's value in keeping the types that we output in 1-1
correspondence with the internal node tags. We have other
node-specific properties that aren't handled that way like "Scan
Direction". The only thing that's different about strategy is that it
happens to be handled in the first "case" block rather than the second
one for reasons that are entirely cosmetic rather than substantive.

...Robert

On Thu, Oct 8, 2009 at 4:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Oct 8, 2009 at 3:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> While we're discussing explain output ... what about triggers?
>>> An important aspect of this change is that at least the row-level
>>> triggers are now going to be charged as runtime of the
>>> Dml-or-whatever-we-call-it node.  Should we rearrange the explain
>>> output so that the printout for trigger runtimes is associated
>>> with those nodes too?  If so, what about statement-level triggers?
>
>> That's not a bad idea, though it wouldn't bother me much if we left it
>> for a follow-on patch.
>
> After cogitating on this for a few minutes, it seems to me that the
> right thing to do is to push the calling of statement-level triggers
> into the Dml node too.  That is, ExecDml() would be responsible for
> calling BEFORE STATEMENT triggers at the start of its first call,
> and for calling AFTER STATEMENT triggers at the end of its last
> call (just before it returns NULL).  This would mean that all trigger
> activity is now associated with a plan node and should be reported
> that way by EXPLAIN.  We could get rid of the rather warty top-level
> XML item for triggers and make the trigger instrumentation outputs
> just be another plan node property.
>
> Aside from having a cleaner EXPLAIN output design, this would mean that
> triggers in the planned WITH RETURNING scenario fire in the order you'd
> expect if we are considering the WITH RETURNING queries to be done
> sequentially and before the main query.  From a user's standpoint it
> would look just like you'd issued the queries sequentially, except that
> the RETURNING data is held for use in the main query.

That seems like a good design. I hate to have you spend too much time
on any one patch at this stage of the game, though. Is this a big
enough change that we should send it back for the patch author (or
other interested parties) to do make that change, or are you just
going to knock it out?

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> That seems like a good design. I hate to have you spend too much time
> on any one patch at this stage of the game, though. Is this a big
> enough change that we should send it back for the patch author (or
> other interested parties) to do make that change, or are you just
> going to knock it out?

I was going to look at it and probably do it myself unless it turns out
to be a bigger change than I'm thinking. The only other stuff that's
Ready for Committer for me now is privileges stuff, which is not on the
critical path for other development work, and besides I'm a bit tired
of that topic right now.

regards, tom lane

On Thu, Oct 8, 2009 at 4:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> That seems like a good design.  I hate to have you spend too much time
>> on any one patch at this stage of the game, though.  Is this a big
>> enough change that we should send it back for the patch author (or
>> other interested parties) to do make that change, or are you just
>> going to knock it out?
>
> I was going to look at it and probably do it myself unless it turns out
> to be a bigger change than I'm thinking.  The only other stuff that's
> Ready for Committer for me now is privileges stuff, which is not on the
> critical path for other development work, and besides I'm a bit tired
> of that topic right now.

"Buffer usage in EXPLAIN and pg_stat_statements" might be another one
for you, unless someone else is already handling that.

"Reworks for Access Controls" is important for future work, so it
would be good to try to fit that one in if possible. It's not marked
as Ready for Committer yet because Stephen hasn't gotten around to
looking at the latest version, but I think he is fairly comfortable
with where it is at, so it is probably good for you to try to
determine whether there's a hole in the bottom of the ship before too
much more time is spend rearranging the deck furniture.

...Robert

Tom Lane wrote:
> I was going to look at it and probably do it myself unless it turns out
> to be a bigger change than I'm thinking. The only other stuff that's
> Ready for Committer for me now is privileges stuff, which is not on the
> critical path for other development work, and besides I'm a bit tired
> of that topic right now.

I'm sorry, it didn't occur to me that this is part of this patch, but I
made these changes in my writeable CTE repo, see
http://git.postgresql.org/gitweb?p=writeable_cte.git;a=commitdiff;h=41ad3d24af845c67a3866e0946129cf9809fe7e9

Regards,
Marko Tiikkaja

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> I'm sorry, it didn't occur to me that this is part of this patch, but I
> made these changes in my writeable CTE repo, see
> http://git.postgresql.org/gitweb?p=writeable_cte.git;a=commitdiff;h=41ad3d24af845c67a3866e0946129cf9809fe7e9

Could you pull out a patch that includes those changes, please?

regards, tom lane

Tom Lane wrote:
> Could you pull out a patch that includes those changes, please?

Sorry for the delay, my master was a bit behind :-( . I moved the
trigger code to nodeDml.c with minor changes and removed unused
resultRelation stuff from DML nodes completely. This also has the
README stuff in it.

Regards,
MArko Tiikkaja

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> Sorry for the delay, my master was a bit behind :-( . I moved the
> trigger code to nodeDml.c with minor changes and removed unused
> resultRelation stuff from DML nodes completely. This also has the
> README stuff in it.

Hm, I've not compared the two versions of the patch, but what I was
thinking was that I'd like to get the resultRelation stuff out of EState
and have it *only* in the DML nodes. It sounds like you went in the
other direction --- what was the reason for that?

regards, tom lane

Tom Lane wrote:
> Hm, I've not compared the two versions of the patch, but what I was
> thinking was that I'd like to get the resultRelation stuff out of EState
> and have it *only* in the DML nodes. It sounds like you went in the
> other direction --- what was the reason for that?

I've taken a compromise of that in the writeable CTE code; the DML nodes
have the index to the start of their result relation array which is part
of estate->es_result_relations. This way the code that currently
depends on estate->es_result_relations works normally. Also, we set
estate->es_result_relation_info only during ExecInitDml(). I didn't
want to break that either. The "index to es_result_relations" is a bit
kludgy, but I didn't see any better ways to do this and wanted to move
on with the code and not be stuck in a single place for too long. But
I'm thinking that this should be part of the writeable CTE patch.

Regards,
Marko Tiikkaja

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> Tom Lane wrote:
>> Hm, I've not compared the two versions of the patch, but what I was
>> thinking was that I'd like to get the resultRelation stuff out of EState
>> and have it *only* in the DML nodes. It sounds like you went in the
>> other direction --- what was the reason for that?

> I've taken a compromise of that in the writeable CTE code; the DML nodes
> have the index to the start of their result relation array which is part
> of estate->es_result_relations. This way the code that currently
> depends on estate->es_result_relations works normally.

OK, that will work I guess, though you'll need to consider how to combine
result_relations arrays from multiple DML nodes. A quick look shows me
that there are a couple of places that do want to look at all the result
relations. We could possibly get rid of that but it's not clear it'd
be much less ugly than what you suggest here.

regards, tom lane

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> Tom Lane wrote:
>> Could you pull out a patch that includes those changes, please?

> Sorry for the delay, my master was a bit behind :-( . I moved the
> trigger code to nodeDml.c with minor changes and removed unused
> resultRelation stuff from DML nodes completely. This also has the
> README stuff in it.

Applied with a moderate amount of editorialization. Notably, I didn't
like what you'd done with the EvalPlanQual stuff, and after a bit of
reflection decided that the right thing was to teach EvalPlanQual to
execute just the desired subplan. Also, I put back the marking of
the ModifyTuple node with its target relations, which you'd removed
in the v4 patch --- I'm convinced that that will be necessary in some
form or other later, so taking it out now seemed like moving backward.

I did not do anything about changing EXPLAIN's output of trigger
information. The stumbling block there is that EXPLAIN executes
queued AFTER trigger events only after finishing the main plan tree
execution. The part of that that is driven by ModifyTable is just
the *queuing* of the triggers, not their *execution*. So my previous
claim that all the trigger execution would now be part of ModifyTable
was wrong. There are several things we could do here:

1. Nothing. I don't care for this, though, because it will lead to
the inconsistent behavior that BEFORE triggers count as part of the
displayed runtime for ModifyTuple and AFTER triggers don't.

2. Move actual execution of (non-deferred) AFTER triggers inside
ModifyTuple. This might be a good idea in order to have the most
consistent results for a series of WITH queries, but I'm not sure.

3. Have EXPLAIN show BEFORE triggers as associated with ModifyTuple
while still showing AFTER triggers as "free standing". Seems a bit
inconsistent.

Comments?

Also, working on this patch made me really want to pull SELECT FOR
UPDATE/SHARE locking out as a separate node too. We've talked about
that before but never got round to it. It's really necessary to do
that in order to have something like
INSERT INTO foo SELECT * FROM bar FOR UPDATE;
behave sanely. I don't recall at the moment whether that worked
sanely before, but it is definitely broken as of CVS tip. Perhaps
I'll work on that this weekend.

regards, tom lane

Tom Lane wrote:
> Applied with a moderate amount of editorialization.

Thank you!

> Notably, I didn't
> like what you'd done with the EvalPlanQual stuff, and after a bit of
> reflection decided that the right thing was to teach EvalPlanQual to
> execute just the desired subplan.

I didn't really like that either but I didn't have any good ideas. This
is a lot better.

> Also, I put back the marking of
> the ModifyTuple node with its target relations, which you'd removed
> in the v4 patch --- I'm convinced that that will be necessary in some
> form or other later, so taking it out now seemed like moving backward.

Ok.

> 2. Move actual execution of (non-deferred) AFTER triggers inside
> ModifyTuple. This might be a good idea in order to have the most
> consistent results for a series of WITH queries, but I'm not sure.

This definitely seems like the best option to me.

Regards,
Marko Tiikkaja