Using results from INSERT ... RETURNING

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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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.tiikkaja@cs.helsinki.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@systemguards.com.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@gmail.com> wrote:

On Sat, Jul 18, 2009 at 5:21 PM, Jaime
Casanova<jcasanov@systemguards.com.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@systemguards.com.ec> writes:

On Sat, Jul 18, 2009 at 6:25 PM, Merlin Moncure<mmoncure@gmail.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@systemguards.com.ec> wrote:

On Tue, Jul 7, 2009 at 3:31 PM, Marko Tiikkaja<marko.tiikkaja@cs.helsinki.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@gmail.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@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.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@sss.pgh.pa.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.tiikkaja@cs.helsinki.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.tiikkaja@cs.helsinki.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.tiikkaja@cs.helsinki.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@sss.pgh.pa.us> wrote:

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.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@gmail.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