Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Andres Freund anarazel.de> writes: > > Hi, > > Some comments about the patch: > * Coding Style: > * multiline comments have both /* and */ on their own lines. > * I think several places indent by two tabs. > * Spaces around operators > * ... > * Many of the new comments would enjoy a bit TLC by a native speaker. > > * The way RTE_ALIAS creeps in many place where it doesn't seem to belong > seems to indicate that the design isn't yet ready. I share Robert's > suspicion that this would be better solved by referring to a special > range table entry. > > Based on the lack of activity around this and the fact that this needs a > *significant* chunk of work before being committable, I am going to mark > this as returned with feedback. I'm actively working towards converting our software at work to use Pg instead of SQL Server and before we switch we'll need this feature to be merged. I'll do what I can to get the verbage and style whipped into shape, though I doubt I can do much with the actual code. Hopefully I can get something in soon. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hi, Some comments about the patch: * Coding Style: * multiline comments have both /* and */ on their own lines. * I think several places indent by two tabs. * Spaces around operators * ... * Many of the new comments would enjoy a bit TLC by a native speaker. * The way RTE_ALIAS creeps in many place where it doesn't seem to belong seems to indicate that the design isn't yet ready. I share Robert's suspicion that this would be better solved by referring to a special range table entry. Based on the lack of activity around this and the fact that this needs a *significant* chunk of work before being committable, I am going to mark this as returned with feedback. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Sun, Feb 02, 2014 at 02:52:42PM -0800, David Fetter wrote:
> On Wed, Aug 21, 2013 at 08:52:25PM +0200, Karol Trzcionka wrote:
> > W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
> > > With this fixed, a more complete review:
> > Thanks.
>
> I've done some syntactic and white space cleanup, here attached.
>
> Karol, would you care to help with commenting the sections that need
> same?
Karol,
Thanks for the updates :)
Other folks,
Next version attached.
Cheers,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..5addfc1 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,12 +302,12 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi =
temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high,
BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS
new_ratio prcp;
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 86449a6..ad4eecb 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo
*relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
-ItemPointer tupleid, TupleTableSlot
*slot)
+ItemPointer tupleid, TupleTableSlot
*slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2378,10 +2378,15 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
* junkfilter's output slot, so we are clobbering the original value of
* slottuple by doing the filtering. This is OK since neither we nor
our
* caller have any more interest in the prior contents of that slot.
+*
+* Execution plan is changed so it is reported up by planSlot,
+* it is needed to get correct value for BEFORE/AFTER statements
+* in RETURNING syntax.
*/
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 6f0f47e..926a80b 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -604,12 +604,17 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo = estate->es_result_relation_info;
resultRelationDesc = resultRelInfo->ri_RelationDesc;
- /* BEFORE ROW UPDATE Triggers */
+ /* BEFORE ROW UPDATE Triggers
+*
+* Caution: planSlot would change here since the target row
+* can be modified after planner but before execution
+* (READ COMMITTED and above)
+* */
if (resultRelInfo->ri_TrigDesc &&
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Wed, Aug 21, 2013 at 08:52:25PM +0200, Karol Trzcionka wrote:
> W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
> > With this fixed, a more complete review:
> Thanks.
I've done some syntactic and white space cleanup, here attached.
Karol, would you care to help with commenting the sections that need
same?
Cheers,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
*** a/doc/src/sgml/ref/update.sgml
--- b/doc/src/sgml/ref/update.sgml
***
*** 194,205 UPDATE [ ONLY ] table_name [ * ] [
output_expression
! An expression to be computed and returned by the UPDATE
! command after each row is updated. The expression can use any
! column names of the table named by table_name
! or table(s) listed in FROM.
! Write * to return all columns.
--- 194,220
output_expression
! An expression to be computed and returned by the
! UPDATE command either before or after (prefixed with
! BEFORE. and AFTER.,
! respectively) each row is updated. The expression can use any
! column names of the table named by table_name or table(s) listed in
! FROM. Write AFTER.* to return all
! columns after the update. Write BEFORE.* for all
! columns before the update. Write * to return all
! columns after update and all triggers fired (these values are in table
! after command). You may combine BEFORE, AFTER and raw columns in the
! expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
***
*** 287,301 UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15,
prcp = DEFAULT
!Perform the same operation and return the updated entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
! RETURNING temp_lo, temp_hi, prcp;
Use the alternative column-list syntax to do the same update:
--- 302,317
!Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
! RETURNING temp_lo AS new_low, temp_hi AS new_high,
BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS
new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***
*** 2335,2341 ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
!ItemPointer tupleid, TupleTableSlot
*slot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
--- 2335,2341
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
!ItemPointer tupleid, TupleTableSlot
*slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
***
*** 2382,2387 ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
--- 2382,2388
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 609,615 ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
!
tupleid, slot);
if (slot == NULL) /* "do nothing" */
return NULL;
--- 609,615
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
!
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Fri, Oct 04, 2013 at 10:42:32AM +0200, Karol Trzcionka wrote: > W dniu 04.10.2013 02:51, Robert Haas pisze: > > Do you have a link to previous discussion on the mailing list? > Sorry, most of discussion was at IRC channel. > > I'm not positive there's enough information available > > at that stage, but if p_target_rangetblentry is populated at that > > point, you should be able to make AFTER.x translate to a Var > > referencing that range table entry. > It's not enough. Even if we know "where we are", there are more issues. > The main question is: how should we pass information about "hello, I'm > specific Var, don't evaluate me like others"? We can add two fields to > Var structure (flag - normal/before/after and no. column) - however it > needs to modify copyObject for Vars (at now it's done e.g. in > flatten_join_alias_vars_mutator for varoattno and varnoold). If > copyObject is modified, sure code in > flatten_join_alias_vars_mutator/pullup_replace_vars_callback will be > useless. I don't know if modifying pg at the low-level (changing > structure of Var and behaviour of copyObject) is good idea. Yes if the > community really want it but it needs more "votes". There is "medium" > solution: changing Var structure and do the "copy" like now (in mutator > and callback) but w/o the condition statement (for the new fields). I > think it might need to modify more places in code because of "comparing" > vars (maybe we'd need to include new fields while comparision). > Regards, > Karol Trzcionka Karol, Do you plan to continue this work for the current commitfest? A lot of people really want the feature :) Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Fri, Oct 4, 2013 at 5:04 AM, Marko Tiikkaja wrote: > I might be completely in the woods here, but I believe something like this > was attempted by Karol earlier, and it failed if two concurrent transactions > did something similar to: > > UPDATE foo SET a = a + 1 RETURNING BEFORE.a; > > Both of them would see BEFORE.a = 0, because that's what the "a" evaluated > to from the tuple we got before EvalPlanQual. > > But maybe what you're suggesting doesn't have this problem? Hmm, it probably does. That explains why there are executor changes here; I guess they need some comments to explain their purpose. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Fri, Oct 4, 2013 at 4:42 AM, Karol Trzcionka wrote: > W dniu 04.10.2013 02:51, Robert Haas pisze: >> Do you have a link to previous discussion on the mailing list? > Sorry, most of discussion was at IRC channel. >> I'm not positive there's enough information available >> at that stage, but if p_target_rangetblentry is populated at that >> point, you should be able to make AFTER.x translate to a Var >> referencing that range table entry. > It's not enough. Even if we know "where we are", there are more issues. > The main question is: how should we pass information about "hello, I'm > specific Var, don't evaluate me like others"? My point is that AFTER.x doesn't appear to need any special marking; it means the same thing as target_table.x. BEFORE.x *does* need some kind of special marking, and I admit I'm not sure what that should look like. Maybe an RTE is OK, but letting that RTE get into the join planning machinery does not seem good; that's going to result in funky special cases all over the place. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On 2013-10-03 20:51:08 -0400, Robert Haas wrote: > On Thu, Oct 3, 2013 at 7:54 PM, Karol Trzcionka wrote: > > Compare EXPLAIN ANALYZE VERBOSE on your statement and on "patched" > > workflow. I can see significant difference. And your "after" returns the > > value after whole the work (after trigger fired) as I know (I don't know > > if it is needed or not, I only point at the difference). > > Sure, I'm not saying we should implement it that way. I'm just > pointing out that the ability already exists, at the executor level, > to return either tuple. So I think the executor itself shouldn't need > to be changed; it's just a matter of getting the correct plan tree to > pop out. Note what pullups ExecDelete is doing to return the old tuple though... So, based on precedent special executor support is not an unlikely thing to be required for a proper implemenation. As Marko mentions, any trivial implementation not doing playing dirty like that will refer to the wrong version of the tuple. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On 10/4/13 12:28 AM, Robert Haas wrote: In fact, we can already get approximately the desired effect already: rhaas=# update foo as after set a = before.a + 1 from foo as before where before.a = after.a returning before.a, after.a; a | a ---+--- 1 | 2 (1 row) Now this is a hack, because we don't really want to add an extra scan/join just to get the behavior we want. But it seems to me significant that this processing makes Vars that refer to the target table refer to the new values, and if we got rid of it, they'd refer to the old values. Can't we contrive to make AFTER.x parse into the same Var node that x currently does? Then we don't need an RTE for it. This part sounds reasonable from here. And maybe BEFORE.x ought to parse to the same node that just plain x does but with some marking, or some other node wrapped around it (like a TargetEntry with some flag set?) that suppresses this processing. I'm just shooting from the hip here; that might be wrong in detail, or even in broad strokes, but it just looks to me like the additional RTE kind is going to bleed into a lot of places. I might be completely in the woods here, but I believe something like this was attempted by Karol earlier, and it failed if two concurrent transactions did something similar to: UPDATE foo SET a = a + 1 RETURNING BEFORE.a; Both of them would see BEFORE.a = 0, because that's what the "a" evaluated to from the tuple we got before EvalPlanQual. But maybe what you're suggesting doesn't have this problem? Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 04.10.2013 02:51, Robert Haas pisze: > Do you have a link to previous discussion on the mailing list? Sorry, most of discussion was at IRC channel. > I'm not positive there's enough information available > at that stage, but if p_target_rangetblentry is populated at that > point, you should be able to make AFTER.x translate to a Var > referencing that range table entry. It's not enough. Even if we know "where we are", there are more issues. The main question is: how should we pass information about "hello, I'm specific Var, don't evaluate me like others"? We can add two fields to Var structure (flag - normal/before/after and no. column) - however it needs to modify copyObject for Vars (at now it's done e.g. in flatten_join_alias_vars_mutator for varoattno and varnoold). If copyObject is modified, sure code in flatten_join_alias_vars_mutator/pullup_replace_vars_callback will be useless. I don't know if modifying pg at the low-level (changing structure of Var and behaviour of copyObject) is good idea. Yes if the community really want it but it needs more "votes". There is "medium" solution: changing Var structure and do the "copy" like now (in mutator and callback) but w/o the condition statement (for the new fields). I think it might need to modify more places in code because of "comparing" vars (maybe we'd need to include new fields while comparision). Regards, Karol Trzcionka -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Thu, Oct 3, 2013 at 7:54 PM, Karol Trzcionka wrote: > Compare EXPLAIN ANALYZE VERBOSE on your statement and on "patched" > workflow. I can see significant difference. And your "after" returns the > value after whole the work (after trigger fired) as I know (I don't know > if it is needed or not, I only point at the difference). Sure, I'm not saying we should implement it that way. I'm just pointing out that the ability already exists, at the executor level, to return either tuple. So I think the executor itself shouldn't need to be changed; it's just a matter of getting the correct plan tree to pop out. > While planning/analyzing the problem there were many ideas about hot to > solve it. Do you have a link to previous discussion on the mailing list? > I think we can do it by modify the Var structure (add some kind of flag > while generating the vars in parser?) but I'm not sure if it is good > idea. The major issue is to know if the Var/TargetEntry references to > the real alias "BEFORE" (named with "AS" syntax or even the real > table-name - I can see there is no difference in code) or the virtual > (from feature patch) "BEFORE". Doing it in parser (more "low-level") > would be very awful - we'd need to check in which part of statement > BEFORE/AFTER is placed (it is not allowed to use it in the other places > than in "RETURNING"). We don't want to make "BEFORE" and "AFTER" > restricted keywords. You're right, it can't happen actually in the parser. But maybe it can happen during parse analysis. I'd spend some time looking at transformColumnRef(), because that's where we translate things x.y into Var nodes. I'm not positive there's enough information available at that stage, but if p_target_rangetblentry is populated at that point, you should be able to make AFTER.x translate to a Var referencing that range table entry. It's a bit less clear how we know that we're inside the returning-list at that point; I'm not sure how much work it would be to pass that information down. But I think it's worth looking at. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 04.10.2013 00:28, Robert Haas pisze: > I wonder if we shouldn't be trying to handle resolution of these names > at an earlier processing stage, closer to the processor. Maybe it can be done in parser (in flex?) but at now it seems to be more isolated feature. > In fact, we can already get approximately the > desired effect already: > > rhaas=# update foo as after set a = before.a + 1 from foo as before > where before.a = after.a returning before.a, after.a; > a | a > ---+--- > 1 | 2 > (1 row) Compare EXPLAIN ANALYZE VERBOSE on your statement and on "patched" workflow. I can see significant difference. And your "after" returns the value after whole the work (after trigger fired) as I know (I don't know if it is needed or not, I only point at the difference). > Now this is a hack, because we don't really want to add an extra > scan/join just to get the behavior we want. But it seems to me > significant that this processing makes Vars that refer to the target > table refer to the new values, and if we got rid of it, they'd refer > to the old values. Can't we contrive to make AFTER.x parse into the > same Var node that x currently does? Then we don't need an RTE for > it. And maybe BEFORE.x ought to parse to the same node that just > plain x does but with some marking, or some other node wrapped around > it (like a TargetEntry with some flag set?) that suppresses this > processing. I'm just shooting from the hip here; that might be wrong > in detail, or even in broad strokes, but it just looks to me like the > additional RTE kind is going to bleed into a lot of places. While planning/analyzing the problem there were many ideas about hot to solve it. I was trying to avoid adding new RTE and referencing to "core" table. However it makes more and more issues. You can see some PoC on the https://github.com/davidfetter/postgresql_projects/compare/returning_before_after (other ideas I revert w/o commit because I couldn't get expected result). The other major reason was that we can avoid touching executor and/or parser's core (flex) this way. One observation: why shouldn't we use the values computed at the moment (it would be computed again if we want to do it later, in executor)? I think we can do it by modify the Var structure (add some kind of flag while generating the vars in parser?) but I'm not sure if it is good idea. The major issue is to know if the Var/TargetEntry references to the real alias "BEFORE" (named with "AS" syntax or even the real table-name - I can see there is no difference in code) or the virtual (from feature patch) "BEFORE". Doing it in parser (more "low-level") would be very awful - we'd need to check in which part of statement BEFORE/AFTER is placed (it is not allowed to use it in the other places than in "RETURNING"). We don't want to make "BEFORE" and "AFTER" restricted keywords. Now most of the code means "don't touch these because they are not real" :) If anyone has the fresh idea to it better, please write it by mail, I don't have more ideas how to solve it. > This patch also has significant style issues. I'll try to fix it soon. Regards, Karol Trzcionka -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
I took a look at this patch today, and I'm pretty skeptical about whether it's on the right track. It adds a new kind of RTE called RTE_ALIAS, which doesn't seem particularly descriptive and alias is used elsewhere to mean something fairly different. More generally, I'm not convinced that adding a new type of RTE is the right way to handle this. The changes in pull_up_subqueries_recurse, pullup_replace_vars_callback, preprocess_targetlist, and build_joinrel_tlist seem like weird hacks. Those functions aren't directly related to this feature; why do they need to know about it? I wonder if we shouldn't be trying to handle resolution of these names at an earlier processing stage, closer to the processor. I notice that set_returning_clause_references() seems to have already solved the hard part of this problem, which is frobbing target list entries to return values from the new row rather than, as they naturally would, the old row. In fact, we can already get approximately the desired effect already: rhaas=# update foo as after set a = before.a + 1 from foo as before where before.a = after.a returning before.a, after.a; a | a ---+--- 1 | 2 (1 row) Now this is a hack, because we don't really want to add an extra scan/join just to get the behavior we want. But it seems to me significant that this processing makes Vars that refer to the target table refer to the new values, and if we got rid of it, they'd refer to the old values. Can't we contrive to make AFTER.x parse into the same Var node that x currently does? Then we don't need an RTE for it. And maybe BEFORE.x ought to parse to the same node that just plain x does but with some marking, or some other node wrapped around it (like a TargetEntry with some flag set?) that suppresses this processing. I'm just shooting from the hip here; that might be wrong in detail, or even in broad strokes, but it just looks to me like the additional RTE kind is going to bleed into a lot of places. This patch also has significant style issues. Conforming to PostgreSQL coding style is essential; if neither the author nor the reviewer fixes problems in this area, then that is essentially making it the committer's job, and the committer may not feel like taking time to do that. Here's a selection of issues that I noticed while reading this through: we use spaces around operators; the patch adds two blank lines that shouldn't be there to the middle of the variable declarations section; variables should be declared in the innermost possible scope; single-statement blocks shouldn't have curly braces; there shouldn't be whitespace before a closing parenthesis; there should be a space after if and before the subsequent parenthesis; braces should be uncuddled; code that does non-obvious things isn't commented. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hi,
2013-08-21 20:52 keltezéssel, Karol Trzcionka írta:
W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
With this fixed, a more complete review:
Thanks.
There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where "returning.sql" resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.
Modified to work correct in parallel testing
doc/src/sgml/ref/update.sgml describes this feature.
doc/src/sgml/dml.sgml should also be touched to cover this feature.
I don't think so, there is not any info about returning feature, I think it shouldn't be
part of my patch.
In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.
Fixed
In b/src/backend/optimizer/plan/setrefs.c:
+static void bind_returning_variables(List *rlist, int bef, int aft);
but later it becomes not public:
+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{
I've change to static in the definition.
+extern void addAliases(ParseState *pstate);
+void addAliases(ParseState *pstate)
This external declaration is not needed since it is already
in src/include/parser/parse_clause.h
Removed.
In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so "before" and "after" are spelled out.
These are not C keywords.
Changed.
Some assignments, like:
+ var=(Var*)tle;
and
+ int index_rel=1;
in setrefs.c need spaces.
"if()" statements need a space before the "(" and not after.
Add spaces in the {} list in addAliases():
+ char*aliases[] = {"before","after"};
like this: { "before", "after" }
Spaces are needed here, too:
+ for(i=0 ; i
Added some whitespaces.
In setrefs.c, bind_returning_variables():
+ Var *var = NULL;
+ foreach(temp, rlist){
Add an empty line after the declaration block.
Added.
Other comments:
This #define in pg_class:
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 49c4f6f..1b09994 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -154,6 +154,7 @@ DESCR("");
#define RELKIND_COMPOSITE_TYPE 'c' /* composite type */
#define RELKIND_FOREIGN_TABLE 'f' /* foreign table */
#define RELKIND_MATVIEW 'm' /* materialized view */
+#define RELKIND_BEFORE 'b' /* virtual table for
before/after statements */
#define RELPERSISTENCE_PERMANENT 'p' /* regular
table */
#define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent
table */
RELKIND_BEFORE removed - it probably left over previous work and/or I needed it because
RTE_BEFORE wasn't available (not included?).
Speaking of which, RTE_BEFORE is more properly named
RTE_RETURNING_ALIAS or something like that because it
covers both "before" and "after". Someone may have a better
idea for naming this symbol.
Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases)
Others may have also a word in this topic, but consider that
this is *not* a regular alias and for those, RTE_ALIAS is not used,
like in
UPDATE table AS alias SET ...
Maybe RTE_RETURNING_ALIAS takes a little more typing, but
it becomes obvious when reading and new code won't confuse
it with regular aliases.
One question, though, about this part:
@@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
int rtoffset)
{
indexed_tlist *itlist;
+ int after_index=0, before_index=0;
+ Query *parse = root->parse;
+ ListCell *rt;
+ RangeTblEntry *bef;
+
+ int index_rel=1;
+
+ foreach(rt,parse->rtable)
+ {
+ bef = (RangeTblEntry *)lfirst(rt);
+ if(strcmp(bef->eref->aliasname,"after") == 0 && bef->rtekind ==
RTE_BEFORE )
+ {
+ after_index = index_rel;
+ }
+ if(strcmp(bef->eref->aliasname,"before") == 0 && bef->rtekind ==
RTE_BEFORE )
+ {
+ before_index = index_rel;
+ }
+ index_rel++;
+ }
/*
* We can perform the desired Var fixup by abusing the fix_join_expr
* machinery that formerly handled inner indexscan fixup. We search the
@@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
resultRelation,
rtoffset);
+ bind_returning_variables(rlist, before_index, after_index);
pfree(itlist);
return rlist;
Why is it enough to keep the last before_index and after_index values?
What if there are more than one matching RangeTblEntry for
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
> With this fixed, a more complete review:
Thanks.
> There is a new regression test (returning_before_after.sql) covering
> this feature. However, I think it should be added to the group
> where "returning.sql" resides currently. There is a value in running it
> in parallel to other tests. Sometimes a subtle bug is uncovered
> because of this and your v2 patch fixed such a bug already.
Modified to work correct in parallel testing
> doc/src/sgml/ref/update.sgml describes this feature.
>
> doc/src/sgml/dml.sgml should also be touched to cover this feature.
I don't think so, there is not any info about returning feature, I think
it shouldn't be part of my patch.
> In the src/test/regress/parallel_schedule contains an extra
> new line at the end, it shouldn't.
Fixed
>
> In b/src/backend/optimizer/plan/setrefs.c:
>
> +static void bind_returning_variables(List *rlist, int bef, int aft);
>
> but later it becomes not public:
>
> + */
> +void bind_returning_variables(List *rlist, int bef, int aft)
> +{
I've change to static in the definition.
>
> +extern void addAliases(ParseState *pstate);
>
> +void addAliases(ParseState *pstate)
>
> This external declaration is not needed since it is already
> in src/include/parser/parse_clause.h
Removed.
>
> In setrefs.c, bind_returning_variables() I would also rename
> the function arguments, so "before" and "after" are spelled out.
> These are not C keywords.
Changed.
> Some assignments, like:
>
> + var=(Var*)tle;
> and
> + int index_rel=1;
>
> in setrefs.c need spaces.
>
> "if()" statements need a space before the "(" and not after.
>
> Add spaces in the {} list in addAliases():
> + char*aliases[] = {"before","after"};
> like this: { "before", "after" }
>
> Spaces are needed here, too:
> + for(i=0 ; i
> This "noal" should be "naliases" or "n_aliases" if you really want
> a variable. I would simply use the constant "2" for the two for()
> loops in addAliases() instead, its purpose is obvious enough.
Added some whitespaces.
> In setrefs.c, bind_returning_variables():
> + Var *var = NULL;
> + foreach(temp, rlist){
> Add an empty line after the declaration block.
Added.
> Other comments:
>
> This #define in pg_class:
>
> diff --git a/src/include/catalog/pg_class.h
> b/src/include/catalog/pg_class.h
> index 49c4f6f..1b09994 100644
> --- a/src/include/catalog/pg_class.h
> +++ b/src/include/catalog/pg_class.h
> @@ -154,6 +154,7 @@ DESCR("");
> #define RELKIND_COMPOSITE_TYPE 'c' /*
> composite type */
> #define RELKIND_FOREIGN_TABLE 'f' /*
> foreign table */
> #define RELKIND_MATVIEW
> 'm' /* materialized view */
> +#define RELKIND_BEFORE
> 'b' /* virtual table for before/after statements */
>
> #define RELPERSISTENCE_PERMANENT
> 'p' /* regular table */
> #define RELPERSISTENCE_UNLOGGED
> 'u' /* unlogged permanent table */
>
RELKIND_BEFORE removed - it probably left over previous work and/or I
needed it because RTE_BEFORE wasn't available (not included?).
> Speaking of which, RTE_BEFORE is more properly named
> RTE_RETURNING_ALIAS or something like that because it
> covers both "before" and "after". Someone may have a better
> idea for naming this symbol.
Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases)
> One question, though, about this part:
>
>
> @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
> int
> rtoffset)
> {
> indexed_tlist *itlist;
> + int after_index=0, before_index=0;
> + Query *parse = root->parse;
>
> + ListCell *rt;
> + RangeTblEntry *bef;
> +
> + int index_rel=1;
> +
> + foreach(rt,parse->rtable)
> + {
> + bef = (RangeTblEntry *)lfirst(rt);
> + if(strcmp(bef->eref->aliasname,"after") == 0 &&
> bef->rtekind == RTE_BEFORE )
> + {
> + after_index = index_rel;
> + }
> + if(strcmp(bef->eref->aliasname,"before") == 0 &&
> bef->rtekind == RTE_BEFORE )
> + {
> + before_index = index_rel;
> + }
> + index_rel++;
> + }
> /*
> * We can perform the desired Var fixup by abusing the
> fix_join_expr
> * machinery that formerly handled inner indexscan fixup. We
> search the
> @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
> resultRelation,
> rtoffset);
>
> + bind_returning_variables(rlist, before_index, after_index);
> pfree(it
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
2013-08-21 20:00 keltezéssel, Boszormenyi Zoltan írta:
2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:
Hi,
2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
Here's a new one, for v7:
setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
bind_returning_variables(rlist, before_index, after_index);
^
setrefs.c:1957:21: note: ‘before_index’ was declared here
int after_index=0, before_index;
^
Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka
With this fixed, a more complete review:
* Is the patch in a patch format which has context? (eg: context diff format)
Yes.
* Does it apply cleanly to the current git master?
Yes.
* Does it include reasonable tests, necessary doc patches, etc?
There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where "returning.sql" resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.
doc/src/sgml/ref/update.sgml describes this feature.
doc/src/sgml/dml.sgml should also be touched to cover this feature.
* Does the patch actually implement what it's supposed to do?
Yes.
* Do we want that?
Yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.
It implements the community-agreed behaviour, according to
the new regression test coverage.
* Does it include pg_dump support (if applicable)?
n/a
* Are there dangers?
I don't think so.
* Have all the bases been covered?
It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:
zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2
text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
id | i1 | i2 | t1 | t2
++++
1 | 1 | 1 | a | a
2 | 2 | 2 | b | b
3 | 3 | 3 | c | c
(3 rows)
zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
+++++++-
2 | 2 | 2 | 4 | b | b | b | bx2
(1 row)
UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id
= 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1,
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2
++++++-+-
3 | 3 | 9 | 6 | c | c | cx3 | cx2
(1 row)
UPDATE 1
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
I don't know.
* Are there any assertion failures or crashes?
No.
* Does the patch slow down simple tests?
No.
* If it claims to improve performance, does it?
n/a
* Does it slow down other things?
No.
* Does it follow the project coding guidelines?
Mostly.
In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.
In b/src/backend/optimizer/plan/setrefs.c:
+static void bind_returning_variables(List *rlist, int bef, int aft);
but later it becomes not public:
+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{
Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.
Similar for parse_clause.c:
+extern void addAliases(ParseState *pstate);
+void addAliases(ParseState *pstate)
This external declaration is not needed since it is already
in src/include/parser/parse_clause.h
In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so "before" and "after" are spelled out.
These are not C keywords.
Some assignments, like:
+ var=(Var*)tle;
and
+ int index_rel=1;
in setrefs.c need spaces.
"if()" statements need a space before the "(" and not after.
Add spaces in the {} list in addAliases():
+ char*aliases[] = {"before","after"};
like this: { "before", "after" }
Spaces are needed here, too:
+ for(i=0 ; i
There should be more comments, especially regarding
my question at the end.
* Does it do what it says, correctly?
Yes.
* Does it produce compiler warnings?
No.
* Can you make it crash?
No.
* Is everything done in a way that fits together coherently with other
features/modules?
I think so, mostly. Comments below
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:
Hi,
2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
Here's a new one, for v7:
setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
bind_returning_variables(rlist, before_index, after_index);
^
setrefs.c:1957:21: note: ‘before_index’ was declared here
int after_index=0, before_index;
^
Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka
With this fixed, a more complete review:
* Is the patch in a patch format which has context? (eg: context diff format)
Yes.
* Does it apply cleanly to the current git master?
Yes.
* Does it include reasonable tests, necessary doc patches, etc?
There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where "returning.sql" resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.
doc/src/sgml/ref/update.sgml describes this feature.
doc/src/sgml/dml.sgml should also be touched to cover this feature.
* Does the patch actually implement what it's supposed to do?
Yes.
* Do we want that?
Yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.
It implements the community-agreed behaviour, according to
the new regression test coverage.
* Does it include pg_dump support (if applicable)?
n/a
* Are there dangers?
I don't think so.
* Have all the bases been covered?
It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:
zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2
text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
id | i1 | i2 | t1 | t2
++++
1 | 1 | 1 | a | a
2 | 2 | 2 | b | b
3 | 3 | 3 | c | c
(3 rows)
zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
+++++++-
2 | 2 | 2 | 4 | b | b | b | bx2
(1 row)
UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id =
3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1,
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2
++++++-+-
3 | 3 | 9 | 6 | c | c | cx3 | cx2
(1 row)
UPDATE 1
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
I don't know.
* Are there any assertion failures or crashes?
No.
* Does the patch slow down simple tests?
No.
* If it claims to improve performance, does it?
n/a
* Does it slow down other things?
No.
* Does it follow the project coding guidelines?
Mostly.
In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.
In b/src/backend/optimizer/plan/setrefs.c:
+static void bind_returning_variables(List *rlist, int bef, int aft);
but later it becomes not public:
+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{
Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.
Similar for parse_clause.c:
+extern void addAliases(ParseState *pstate);
+void addAliases(ParseState *pstate)
This external declaration is not needed since it is already
in src/include/parser/parse_clause.h
In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so "before" and "after" are spelled out.
These are not C keywords.
Some assignments, like:
+ var=(Var*)tle;
and
+ int index_rel=1;
in setrefs.c need spaces.
"if()" statements need a space before the "(" and not after.
Add spaces in the {} list in addAliases():
+ char*aliases[] = {"before","after"};
like this: { "before", "after" }
Spaces are needed here, too:
+ for(i=0 ; i
There should be more comments, especially regarding
my question at the end.
* Does it do what it says, correctly?
Yes.
* Does it produce compiler warnings?
No.
* Can you make it crash?
No.
* Is everything done in a way that fits together coherently with other
features/modules?
I think so, mostly. Comments below.
* Are there interdependencies that can cause problems
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hi,
2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
Here's a new one, for v7:
setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
bind_returning_variables(rlist, before_index, after_index);
^
setrefs.c:1957:21: note: ‘before_index’ was declared here
int after_index=0, before_index;
^
Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka
With this fixed, a more complete review:
* Is the patch in a patch format which has context? (eg: context diff format)
Yes.
* Does it apply cleanly to the current git master?
Yes.
* Does it include reasonable tests, necessary doc patches, etc?
There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where "returning.sql" resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.
doc/src/sgml/ref/update.sgml describes this feature.
doc/src/sgml/dml.sgml should also be touched to cover this feature.
* Does the patch actually implement what it's supposed to do?
Yes.
* Do we want that?
Yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.
It implements the community-agreed behaviour, according to
the new regression test coverage.
* Does it include pg_dump support (if applicable)?
n/a
* Are there dangers?
I don't think so.
* Have all the bases been covered?
It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:
zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2
text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
id | i1 | i2 | t1 | t2
++++
1 | 1 | 1 | a | a
2 | 2 | 2 | b | b
3 | 3 | 3 | c | c
(3 rows)
zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
+++++++-
2 | 2 | 2 | 4 | b | b | b | bx2
(1 row)
UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 3
returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1,
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2
++++++-+-
3 | 3 | 9 | 6 | c | c | cx3 | cx2
(1 row)
UPDATE 1
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
I don't know.
* Are there any assertion failures or crashes?
No.
* Does the patch slow down simple tests?
No.
* If it claims to improve performance, does it?
n/a
* Does it slow down other things?
No.
* Does it follow the project coding guidelines?
Mostly.
In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.
In b/src/backend/optimizer/plan/setrefs.c:
+static void bind_returning_variables(List *rlist, int bef, int aft);
but later it becomes not public:
+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{
Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.
Similar for parse_clause.c:
+extern void addAliases(ParseState *pstate);
+void addAliases(ParseState *pstate)
This external declaration is not needed since it is already
in src/include/parser/parse_clause.h
In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so "before" and "after" are spelled out.
These are not C keywords.
Some assignments, like:
+ var=(Var*)tle;
and
+ int index_rel=1;
in setrefs.c need spaces.
"if()" statements need a space before the "(" and not after.
Add spaces in the {} list in addAliases():
+ char*aliases[] = {"before","after"};
like this: { "before", "after" }
Spaces are needed here, too:
+ for(i=0 ; i+#define RELKIND_BEFORE 'b' /* virtual table for before/after
statements */
#define RELPERSISTENCE_PERMANENT 'p' /* regular
table */
#define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent
table */
The "RELKIND_*" values all show up in the pg_class table except
this new one. I don't think pg_class.h should be modified at all.
addAliases() sh
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
> Here's a new one, for v7:
>
> setrefs.c: In function ‘set_plan_refs’:
> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized
> in this function [-Wmaybe-uninitialized]
> bind_returning_variables(rlist, before_index, after_index);
> ^
> setrefs.c:1957:21: note: ‘before_index’ was declared here
> int after_index=0, before_index;
> ^
Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 908f397..461ec4f 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1987,6 +1987,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2701,6 +2702,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy eref, aliases, etc; OK? */
break;
case RTE_SUBQUERY:
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index cff4734..3c4e045 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2368,6 +2368,7 @@
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
2013-08-20 17:30 keltezéssel, Karol Trzcionka írta: W dniu 20.08.2013 16:47, Karol Trzcionka pisze: Thank you for the review and tests. New version introduce a lot of improvements: - Fix regression test for view (wrong table_name) - Add regression test for inheritance - Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the uninitialized issue - Revert changing varno in add_vars_to_targetlist - Add all "before" variables to targetlist - Avoid adding variables to slot for AFTER. - Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE - All before/after are now set on OUTER_VAR - Rename fix_varno_varattno to bind_returning_variables - Add comment about bind_returning_variables - Remove unneeded code in fix_join_expr_mutator (it was changing varno of RTE_BEFORE - now there is not any var with varno assigned to it) I've just realized the prepare_returning_before() is unneeded right now so I've removed it. Version 7, hopefully the last. ;) Here's a new one, for v7: setrefs.c: In function ‘set_plan_refs’: setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function [-Wmaybe-uninitialized] bind_returning_variables(rlist, before_index, after_index); ^ setrefs.c:1957:21: note: ‘before_index’ was declared here int after_index=0, before_index; ^ Best regards, Zoltán Böszörményi Regards, Karol Trzcionka -- -- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 20.08.2013 16:47, Karol Trzcionka pisze:
> Thank you for the review and tests. New version introduce a lot of
> improvements:
> - Fix regression test for view (wrong table_name)
> - Add regression test for inheritance
> - Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
> uninitialized issue
> - Revert changing varno in add_vars_to_targetlist
> - Add all "before" variables to targetlist
> - Avoid adding variables to slot for AFTER.
> - Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
> - All before/after are now set on OUTER_VAR
> - Rename fix_varno_varattno to bind_returning_variables
> - Add comment about bind_returning_variables
> - Remove unneeded code in fix_join_expr_mutator (it was changing varno
> of RTE_BEFORE - now there is not any var with varno assigned to it)
I've just realized the prepare_returning_before() is unneeded right now
so I've removed it. Version 7, hopefully the last. ;)
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 908f397..461ec4f 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1987,6 +1987,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* noth
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Thank you for the review and tests. New version introduce a lot of
improvements:
- Fix regression test for view (wrong table_name)
- Add regression test for inheritance
- Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
uninitialized issue
- Revert changing varno in add_vars_to_targetlist
- Add all "before" variables to targetlist
- Avoid adding variables to slot for AFTER.
- Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
- All before/after are now set on OUTER_VAR
- Rename fix_varno_varattno to bind_returning_variables
- Add comment about bind_returning_variables
- Remove unneeded code in fix_join_expr_mutator (it was changing varno
of RTE_BEFORE - now there is not any var with varno assigned to it)
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 908f397..461ec4f 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1987,6 +1987,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2701,6 +2702,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy e
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hi,
2013-08-19 21:52 keltezéssel, Boszormenyi Zoltan írta:
2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:
W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
* Does it apply cleanly to the current git master?
No. There's a reject in src/backend/optimizer/plan/initsplan.c
Thank you, merged in attached version.
* Does it include reasonable tests?
Yes but the test fails after trying to fix the rejected chunk of the
patch.
It fails because the "HINT" was changed, fixed.
That version merges some nested "ifs" left over from earlier work.
I tried to compile your v5 patch and I got:
initsplan.c: In function ‘add_vars_to_targetlist’:
initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
rel->reltargetlist = lappend(rel->reltargetlist,
^
You shouldn't change the assignment at declaration:
- RelOptInfo *rel = find_base_rel(root, var->varno);
+ RelOptInfo *rel;
...
+ if (root->parse->commandType == CMD_UPDATE)
+ {
... (code using rel)
+ }
+ rel = find_base_rel(root, varno);
Let me say it again: the new code in initsplan.c::add_vars_to_targetlist() is
fishy.
The compiler says that "rel" used on line 216 may be uninitialized.
Keeping it that way passes "make check", perhaps "rel" was initialized
in a previous iteration of "foreach(temp, vars)", possibly in the
else if (IsA(node, PlaceHolderVar))
branch, which means that "PlaceHolderInfo *phinfo" may be interpreted
as RelOptInfo *, stomping on memory.
Moving the assignment back to the declaration makes "make check"
fail with the attached regression.diffs file.
Initializing it as "RelOptInfo *rel = NULL;" makes the regression check
die with a segfault, obviously.
Change the code to avoid the warning and still produce the wanted effect.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
***
/home/zozo/crosscolumn/review/returning-before-after/postgresql/src/test/regress/expected/returning_before_after.out
2013-08-20 15:01:03.365314482 +0200
---
/home/zozo/crosscolumn/review/returning-before-after/postgresql/src/test/regress/results/returning_before_after.out
2013-08-20 15:30:21.091641454 +0200
***
*** 6,47
);
INSERT INTO foo VALUES (1, 'x'),(2,'y');
UPDATE foo SET bar1=bar1+1 RETURNING before.*, bar1, bar2;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 1 | x|2 | x
! 2 | y|3 | y
! (2 rows)
!
UPDATE foo SET bar1=bar1-1 RETURNING after.bar1, before.bar1*2;
! bar1 | ?column?
! --+--
! 1 |4
! 2 |6
! (2 rows)
!
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'z' RETURNING before.*, after.*;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 1 | x|2 | xz
! 2 | y|3 | yz
! (2 rows)
!
-- check single after
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'a' RETURNING after.*;
! bar1 | bar2
! --+--
! 3 | xza
! 4 | yza
! (2 rows)
!
-- check single before
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'b' RETURNING before.*;
! bar1 | bar2
! --+--
! 3 | xza
! 4 | yza
! (2 rows)
!
-- it should fail
UPDATE foo SET bar1=bar1+before.bar1 RETURNING before.*;
ERROR: missing FROM-clause entry for table "before"
--- 6,22
);
INSERT INTO foo VALUES (1, 'x'),(2,'y');
UPDATE foo SET bar1=bar1+1 RETURNING before.*, bar1, bar2;
! ERROR: no relation entry for relid 2
UPDATE foo SET bar1=bar1-1 RETURNING after.bar1, before.bar1*2;
! ERROR: no relation entry for relid 3
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'z' RETURNING before.*, after.*;
! ERROR: no relation entry for relid 2
-- check single after
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'a' RETURNING after.*;
! ERROR: no relation entry for relid 3
-- check single before
UPDATE foo SET bar1=bar1+1, bar2=bar2 || 'b' RETURNING before.*;
! ERROR: no relation entry for relid 2
-- it should fail
UPDATE foo SET bar1=bar1+before.bar1 RETURNING before.*;
ERROR: missing FROM-clause entry for table "before"
***
*** 53,90
^
-- test before/after aliases
UPDATE foo AS before SET bar1=bar1+1 RETURNING before.*,after.*;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 5 | xzab |5 | xzab
! 6 | yzab |6 | yzab
! (2 rows)
!
UPDATE foo AS after SET bar1=bar1-1 RETURNING before.*,after.*;
! bar1 | bar2 | bar1 | bar2
! --+--+--+--
! 5 | xzab |4 | xzab
! 6 | yzab |5 | yzab
! (2 rows)
!
-- test inheritance
CREATE TABLE foo2 (bar INTEGER) INHERITS(foo);
INSERT INTO foo2 VALUES (1,'b',5);
UPDATE foo2 SET bar1=bar1*2, bar=bar1+5, bar2=bar1::text || bar::text
RETURNING before.*, after.*, *;
! bar1 | bar2 | bar | bar1 | bar2 | bar |
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:
W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
* Does it apply cleanly to the current git master?
No. There's a reject in src/backend/optimizer/plan/initsplan.c
Thank you, merged in attached version.
* Does it include reasonable tests?
Yes but the test fails after trying to fix the rejected chunk of the
patch.
It fails because the "HINT" was changed, fixed.
That version merges some nested "ifs" left over from earlier work.
I tried to compile your v5 patch and I got:
initsplan.c: In function ‘add_vars_to_targetlist’:
initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
rel->reltargetlist = lappend(rel->reltargetlist,
^
You shouldn't change the assignment at declaration:
- RelOptInfo *rel = find_base_rel(root, var->varno);
+ RelOptInfo *rel;
...
+ if (root->parse->commandType == CMD_UPDATE)
+ {
... (code using rel)
+ }
+ rel = find_base_rel(root, varno);
Best regards,
Zoltán Böszörményi
Regards,
Karol Trzcionka
--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
> * Does it apply cleanly to the current git master?
>
> No. There's a reject in src/backend/optimizer/plan/initsplan.c
Thank you, merged in attached version.
>
> * Does it include reasonable tests?
>
> Yes but the test fails after trying to fix the rejected chunk of the
> patch.
It fails because the "HINT" was changed, fixed.
That version merges some nested "ifs" left over from earlier work.
Regards,
Karol Trzcionka
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 908f397..461ec4f 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1987,6 +1987,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2701,6 +2702,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy eref, aliases, etc; OK? */
break;
case RTE_SUBQUERY:
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index cff4734..3c4e045 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2368,6 +2368,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEn
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hi, mini-review follows. 2013-07-22 21:52 keltezéssel, Karol Trzcionka írta: I've noticed problem with "UPDATE ... FROM" statement. Fix in new version. Regards, Karol * Does it apply cleanly to the current git master? No. There's a reject in src/backend/optimizer/plan/initsplan.c * Does it include reasonable tests? Yes but the test fails after trying to fix the rejected chunk of the patch. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 23.07.2013 06:22, David Fetter pisze: > What problem or problems did you notice, and what did you change to > fix them? "UPDATE ... FROM" generated "ERROR: variable not found in subplan target lists". I've added some workaround in add_vars_to_targetlist: - if it is an "after" - simple change var->varno to base RTE (it should always exists, the value is temporary, it will change to OUTER_VAR) - if it is a "before" - add to targetlist new var independently from rel->attr_needed[attno]. Additionally I've change build_joinrel_tlist to ignore any "BEFORE" RTEs. The regression tests are updated. Regards, Karol -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Mon, Jul 22, 2013 at 09:52:14PM +0200, Karol Trzcionka wrote: > I've noticed problem with "UPDATE ... FROM" statement. Fix in new version. > Regards, > Karol What problem or problems did you notice, and what did you change to fix them? Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
I've noticed problem with "UPDATE ... FROM" statement. Fix in new version.
Regards,
Karol
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index a896d76..409b4d1 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1928,6 +1928,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2642,6 +2643,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy eref, aliases, etc; OK? */
break;
case RTE_SUBQUERY:
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 48cd9dc..79b03af 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2366,6 +2366,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
switch (node->rtekind)
{
case RTE_RELATION:
+ case RTE_BEFORE:
WRITE_OID_FIELD(relid);
WRITE_CHAR_FIELD(relkind);
break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index dc9cb3e..2ca29da 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1207,6 +1207,7 @@ _readRangeTblEntry(void)
swit
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
New version:
- fix returning "after" values if there are not "before"
- add more regression tests
I'd like to hear/read any feedback ;)
Regards,
Karol
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..06ebaf3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -603,7 +603,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -737,6 +737,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index a896d76..409b4d1 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1928,6 +1928,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2642,6 +2643,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy eref, aliases, etc; OK? */
break;
case RTE_SUBQUERY:
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 48cd9dc..79b03af 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2366,6 +2366,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
switch (node->rtekind)
{
case RTE_RELATION:
+ case RTE_BEFORE:
WRITE_OID_FIELD(relid);
WRITE_CHAR_FIELD(relkind);
break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index dc9cb3e..2ca29da 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/read
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Sat, Jul 13, 2013 at 12:49:45AM +0200, Karol Trzcionka wrote: > Next version: > - cleanup > - regression test > - fix issue reported by johto (invalid values in parallel transactions) > I would like more feedback and comments about the patch, as some parts > may be too hacky. > In particular, is it a problem that I update a pointer to planSlot? In > my patch, it points to tuple after all updates done between planner and > executor (in fact it is not planSlot right now). I don't know whether > the tuple could be deleted in the intervening time and if the pointer > doesn't point to "unreserved" memory (I mean - memory which may be > overwritten by something meanwhile). Thanks for the updated patch! Anybody care to look this over for vulnerabilities as described above? Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Next version:
- cleanup
- regression test
- fix issue reported by johto (invalid values in parallel transactions)
I would like more feedback and comments about the patch, as some parts
may be too hacky.
In particular, is it a problem that I update a pointer to planSlot? In
my patch, it points to tuple after all updates done between planner and
executor (in fact it is not planSlot right now). I don't know whether
the tuple could be deleted in the intervening time and if the pointer
doesn't point to "unreserved" memory (I mean - memory which may be
overwritten by something meanwhile).
Regards,
Karol
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index d86e9ad..fafd311 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
- ItemPointer tupleid, TupleTableSlot *slot)
+ ItemPointer tupleid, TupleTableSlot *slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2381,6 +2381,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index e934c7b..27859dd 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -599,7 +599,7 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo->ri_TrigDesc->trig_update_before_row)
{
slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
- tupleid, slot);
+ tupleid, slot, &planSlot);
if (slot == NULL) /* "do nothing" */
return NULL;
@@ -733,6 +733,7 @@ lreplace:;
hufd.xmax);
if (!TupIsNull(epqslot))
{
+ planSlot = epqslot;
*tupleid = hufd.ctid;
slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 42d6621..cc68568 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1920,6 +1920,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2622,6 +2623,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy eref, aliases, etc; OK? */
break;
case RTE_SUBQUERY:
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index b2183f4..2698535 100644
--- a/src
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Updated patch:
- include sgml
- fix all compiler warnings
- some cleanup
- fix correctness of feature
Regards,
Karol
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..eba35f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] table_name [ * ] [
output_expression
- An expression to be computed and returned by the UPDATE
- command after each row is updated. The expression can use any
- column names of the table named by table_name
- or table(s) listed in FROM.
- Write * to return all columns.
+ An expression to be computed and returned by the
+ UPDATE command either before or after (prefixed with
+ BEFORE. and AFTER.,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by table_name or table(s) listed in
+ FROM. Write AFTER.* to return all
+ columns after the update. Write BEFORE.* for all
+ columns before the update. Write * to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
+
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called before or
+ after, alias it to something else when using
+ RETURNING.
+
+
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high, BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS new_ratio prcp;
+
Use the alternative column-list syntax to do the same update:
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 42d6621..cc68568 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1920,6 +1920,7 @@ range_table_walker(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* nothing to do */
break;
case RTE_SUBQUERY:
@@ -2622,6 +2623,7 @@ range_table_mutator(List *rtable,
{
case RTE_RELATION:
case RTE_CTE:
+ case RTE_BEFORE:
/* we don't bother to copy eref, aliases, etc; OK? */
break;
case RTE_SUBQUERY:
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index b2183f4..2698535 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2351,6 +2351,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
switch (node->rtekind)
{
case RTE_RELATION:
+ case RTE_BEFORE:
WRITE_OID_FIELD(relid);
WRITE_CHAR_FIELD(relkind);
break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 3a16e9d..04931ee 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1189,6 +1189,7 @@ _readRangeTblEntry(void)
switch (local_node->rtekind)
{
case RTE_RELATION:
+ case RTE_BEFORE:
READ_OID_FIELD(relid);
READ_CHAR_FIELD(relkind);
break;
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index 839ed9d..7fc6ea1 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -174,9 +174,16 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars,
if (IsA(node, Var))
{
Var *var = (Var *) node;
- RelOptInfo *rel = find_base_rel(root, var->varno);
+ RelOptInfo *rel;
int attno = var->varattno;
+ RangeTblEntry *rte;
+ if (root->parse->commandType == CMD_UPDATE){
+rte = ((RangeTblEntry *) list_nth(root->parse->rtable, (var->varno)-1));
+if(rte->rtekind == RTE_BEFORE)
+ continue;
+ }
+ rel = find_base_rel(root, var->varno);
Assert(attno >= rel->min_attr && attno <= rel->max_attr);
attno -= rel->min_attr;
if (rel->attr_needed[attno] == NULL)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index d80c264..77b0c38 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1989,6 +1989,9 @@ preprocess_rowmarks(PlannerInfo *root)
if (rte->relkind == RELKIND_FOREIGN_TABLE)
continue;
+ if (rte->relkind == RELKIND_BEFORE)
+ continue;
+
rels = bms_del_member(rels, rc->rti);
newrc = makeNode(PlanRowMark);
@@ -2028,6 +2031,9 @@ preprocess_rowmarks(PlannerInfo *root)
if (!bms_is_member(i, rels))
continue;
+ if (rte->relkind == RELKIND_BEFORE)
+ continue;
+
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Fri, Jul 05, 2013 at 03:22:33AM +0200, Karol Trzcionka wrote: > Hello, > according to my mentor's suggestion, I send first PoC patch of > "RETURNING AFTER/BEFORE" statement. Some info: > - it is early version - more hack PoC than RC patch > - AFTER in this version works as decribed before but it returns row > after update but before post-update before (to be fixed or switch with > current "*" behaviour - I don't know yet) > - patch passes all regression tests > - the code is uncommented already, to be fixed > I'm not sure what may fail so you use it on your risk ;) > Regards, > Karol Karol, Per discussion in IRC, please follow up with your patch that includes such documentation, new regression tests, etc., you've written for the feature. Thanks! :) Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Hello,
according to my mentor's suggestion, I send first PoC patch of
"RETURNING AFTER/BEFORE" statement. Some info:
- it is early version - more hack PoC than RC patch
- AFTER in this version works as decribed before but it returns row
after update but before post-update before (to be fixed or switch with
current "*" behaviour - I don't know yet)
- patch passes all regression tests
- the code is uncommented already, to be fixed
I'm not sure what may fail so you use it on your risk ;)
Regards,
Karol
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index b2183f4..2698535 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2351,6 +2351,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
switch (node->rtekind)
{
case RTE_RELATION:
+ case RTE_BEFORE:
WRITE_OID_FIELD(relid);
WRITE_CHAR_FIELD(relkind);
break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 3a16e9d..04931ee 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1189,6 +1189,7 @@ _readRangeTblEntry(void)
switch (local_node->rtekind)
{
case RTE_RELATION:
+ case RTE_BEFORE:
READ_OID_FIELD(relid);
READ_CHAR_FIELD(relkind);
break;
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index 839ed9d..7fc6ea1 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -174,9 +174,16 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars,
if (IsA(node, Var))
{
Var *var = (Var *) node;
- RelOptInfo *rel = find_base_rel(root, var->varno);
+ RelOptInfo *rel;
int attno = var->varattno;
+ RangeTblEntry *rte;
+ if (root->parse->commandType == CMD_UPDATE){
+rte = ((RangeTblEntry *) list_nth(root->parse->rtable, (var->varno)-1));
+if(rte->rtekind == RTE_BEFORE)
+ continue;
+ }
+ rel = find_base_rel(root, var->varno);
Assert(attno >= rel->min_attr && attno <= rel->max_attr);
attno -= rel->min_attr;
if (rel->attr_needed[attno] == NULL)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index d80c264..77b0c38 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1989,6 +1989,9 @@ preprocess_rowmarks(PlannerInfo *root)
if (rte->relkind == RELKIND_FOREIGN_TABLE)
continue;
+ if (rte->relkind == RELKIND_BEFORE)
+ continue;
+
rels = bms_del_member(rels, rc->rti);
newrc = makeNode(PlanRowMark);
@@ -2028,6 +2031,9 @@ preprocess_rowmarks(PlannerInfo *root)
if (!bms_is_member(i, rels))
continue;
+ if (rte->relkind == RELKIND_BEFORE)
+ continue;
+
newrc = makeNode(PlanRowMark);
newrc->rti = newrc->prti = i;
newrc->rowmarkId = ++(root->glob->lastRowMarkId);
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b78d727..6828a7d 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1691,6 +1691,12 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
if (IsA(node, Var))
{
Var *var = (Var *) node;
+ if (var->varno<=list_length(context->root->parse->rtable) && var->varno>1 && context->root->parse->commandType == CMD_UPDATE){
+ RangeTblEntry *rte_a,*rte_b;
+ rte_a = (RangeTblEntry *)list_nth(context->root->parse->rtable,var->varno-1);
+ rte_b = (RangeTblEntry *)list_nth(context->root->parse->rtable,var->varno-2);
+ if (rte_a->rtekind == RTE_BEFORE && rte_b->rtekind == RTE_BEFORE) var->varno-=1;
+ }
/* First look for the var in the input tlists */
newvar = search_indexed_tlist_for_var(var,
@@ -1892,6 +1898,37 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
*
* Note: resultRelation is not yet adjusted by rtoffset.
*/
+
+void fix_varno_varattno(List *rlist, int begin, int bef, int aft){
+ ListCell *temp;
+ ListCell *temp2;
+ Var *var;
+ foreach(temp, rlist){
+ TargetEntry *tle = (TargetEntry *)lfirst(temp);
+
+ if(IsA(tle, TargetEntry)){
+ var = (Var*)tle->expr;
+ }else if(IsA(tle, Var)) var=(Var*)tle;
+ if( IsA(var, Var) ){
+ if(var->varnoold == bef){
+var->varno = OUTER_VAR;
+var->varattno = var->varoattno + begin;
+ }
+ else if(var->varnoold == aft)
+ {
+ var->varno = OUTER_VAR;
+ var->varattno = var->varoattno;
+ }
+ }
+ else if( IsA(var, OpExpr )){
+ fix_varno_varattno(((OpExpr*)var)->args, begin, bef, aft);
+ }
+ else if( IsA(var, FuncExpr )){
+ fix_varno_varattno(((FuncExpr*)var)->args, begin, bef, aft);
+ }
+ }
+}
+
static List *
set_returning_clause_references(PlannerInfo *root,
List *rlist,
@@ -1900,7 +1937,48 @@ set_returning_clause_references(PlannerInfo *root,
int rtoffset)
{
indexed_tlist *itlist;
+ int before_index=0, after_index=0, var_index=0;
+ Query *parse = root->parse;
+
+ ListCell *rt;
+ RangeTblEntry *b
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Karol Trzcionka writes: > It will not solve the problems: > 1. How to access to old rows if the table is named "BEFORE"? The user can simply choose to use a different table alias, as Andres explained upthread. If any table's active alias is "before" (or "after"), we just don't activate the corresponding implicit alias. > 2. Should AFTER for DELETE and BEFORE for INSERT be allowed? If yes what > should they return? These cases should certainly fail. Now, IMO there's no very good reason to alter the behavior at all for INSERT/DELETE; only UPDATE has an issue here. But if we were going to support the extra aliases in those commands, only the ones that actually make sense should be allowed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 02.05.2013 23:34, Gavin Flower pisze: > I prefer 'PRIOR & 'AFTER' as the both have the same length > - but perhaps that is just me! :-) I think it doesn't matter at now because PRIOR has the same problems as BEFORE ;) Regards, Karol
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On 03/05/13 04:52, David Fetter wrote: On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote: On 2013-05-02 12:23:19 -0400, Tom Lane wrote: Marko Tiikkaja writes: What I'm more interested in is: how can we make this feature work in PL/PgSQL where OLD means something different? That's a really good point: if you follow this approach then you're creating fundamental conflicts for use of the feature in trigger functions or rules, which will necessarily have conflicting uses of those names. Yeah, we could define scoping rules such that there's an unambiguous interpretation, but then the user is just out of luck if he wants to reference the other definition. (This problem is probably actually worse if you implement with reserved words rather than aliases.) I'm thinking it would be better to invent some other notation for access to old-row values. prior/after? Both are unreserved keywords atm and it seems far less likely to have conflicts than new/old. BEFORE/AFTER seems more logical to me. Yes, those words both have meaning in, for example, a trigger definition, but they're clearly separable by context. Yay, bike-shedding! Cheers, David. I prefer 'PRIOR & 'AFTER' as the both have the same length - but perhaps that is just me! :-) Cheers, Gavin
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 02.05.2013 20:45, Tom Lane pisze: > David Fetter writes: >> Maybe we can make BEFORE and AFTER implied aliases rather than >> keywords. What say? > Well, they still have to be unreserved keywords for their use in > trigger-related commands, but as far as this feature is concerned > I think they're best off being handled as automatically-supplied > aliases. IOW the patch needn't touch gram.y at all. Well... the syntax which wouldn't conflict with Pl/PgSQL is (draft): INSERT INTO foo ... RETURNING AFTER.* UPDATE foo SET ... RETURNING AFTER.*, BEFORE.* DELETE FROM foo ... RETURNING BEFORE.* It will not solve the problems: 1. How to access to old rows if the table is named "BEFORE"? 2. Should AFTER for DELETE and BEFORE for INSERT be allowed? If yes what should they return? I mean: what should be result of: INSERT INTO foo ... RETURNING BEFORE.* and DELETE FROM foo ... RETURNING AFTER.* ? The best summarize of dropping NEW/OLD keywords for this proposal was proposed while IRC meeting: create or replace function ft1(integer) returns integer language plpgsql as $f$ <> declare r rt1; begin select 1 as a into r; update rt1 r set a = a + 1 returning XXX into r; raise notice 'r = %', r; return null; end; $f$; Regards, Karol Trzcionka -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Karol Trzcionka writes: > What do you think about function- or cast-like syntax. I mean: > RETURNING OLD(foo.bar) > or RETURNING OLD(foo).bar > or RETURNING (foo::OLD).bar ? > I think none of them should conflict with any other statements. I think you'll find those alternatives are at least as ugly to implement as they are to look at ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
David Fetter writes: > Maybe we can make BEFORE and AFTER implied aliases rather than > keywords. What say? Well, they still have to be unreserved keywords for their use in trigger-related commands, but as far as this feature is concerned I think they're best off being handled as automatically-supplied aliases. IOW the patch needn't touch gram.y at all. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Sent from my iPad On 03-May-2013, at 0:07, David Fetter wrote: > On Thu, May 02, 2013 at 01:40:59PM -0400, Tom Lane wrote: >> David Fetter writes: >>> On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote: prior/after? Both are unreserved keywords atm and it seems far less likely to have conflicts than new/old. >> >>> BEFORE/AFTER seems more logical to me. >> >> Works for me. >> >>regards, tom lane > > Maybe we can make BEFORE and AFTER implied aliases rather than > keywords. What say? > > I agree.Overall,I like the concept. Regards, Atri -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Thu, May 02, 2013 at 01:40:59PM -0400, Tom Lane wrote: > David Fetter writes: > > On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote: > >> prior/after? Both are unreserved keywords atm and it seems far less > >> likely to have conflicts than new/old. > > > BEFORE/AFTER seems more logical to me. > > Works for me. > > regards, tom lane Maybe we can make BEFORE and AFTER implied aliases rather than keywords. What say? Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 02.05.2013 19:40, Tom Lane pisze: >> BEFORE/AFTER seems more logical to me. > Works for me. > What do you think about function- or cast-like syntax. I mean: RETURNING OLD(foo.bar) or RETURNING OLD(foo).bar or RETURNING (foo::OLD).bar ? I think none of them should conflict with any other statements. Regards, Karol Trzcionka -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
David Fetter writes: > On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote: >> prior/after? Both are unreserved keywords atm and it seems far less >> likely to have conflicts than new/old. > BEFORE/AFTER seems more logical to me. Works for me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
2013/5/2 Tom Lane > Marko Tiikkaja writes: > > What I'm more interested in is: how can we make this feature work in > > PL/PgSQL where OLD means something different? > > That's a really good point: if you follow this approach then you're > creating fundamental conflicts for use of the feature in trigger > functions or rules, which will necessarily have conflicting uses of > those names. Yeah, we could define scoping rules such that there's > an unambiguous interpretation, but then the user is just out of luck > if he wants to reference the other definition. (This problem is > probably actually worse if you implement with reserved words rather > than aliases.) > > I'm thinking it would be better to invent some other notation for access > to old-row values. > I am not sure, but I am thinking so NEW and OLD are used in some statements and features ANSI SQL 2012, so probably we should to do keywords from these words if we would to support modern ANSI SQL Regards Pavel > > regards, tom lane > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote: > On 2013-05-02 12:23:19 -0400, Tom Lane wrote: > > Marko Tiikkaja writes: > > > What I'm more interested in is: how can we make this feature work in > > > PL/PgSQL where OLD means something different? > > > > That's a really good point: if you follow this approach then you're > > creating fundamental conflicts for use of the feature in trigger > > functions or rules, which will necessarily have conflicting uses of > > those names. Yeah, we could define scoping rules such that there's > > an unambiguous interpretation, but then the user is just out of luck > > if he wants to reference the other definition. (This problem is > > probably actually worse if you implement with reserved words rather > > than aliases.) > > > > I'm thinking it would be better to invent some other notation for access > > to old-row values. > > prior/after? Both are unreserved keywords atm and it seems far less > likely to have conflicts than new/old. BEFORE/AFTER seems more logical to me. Yes, those words both have meaning in, for example, a trigger definition, but they're clearly separable by context. Yay, bike-shedding! Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On 2013-05-02 12:23:19 -0400, Tom Lane wrote: > Marko Tiikkaja writes: > > What I'm more interested in is: how can we make this feature work in > > PL/PgSQL where OLD means something different? > > That's a really good point: if you follow this approach then you're > creating fundamental conflicts for use of the feature in trigger > functions or rules, which will necessarily have conflicting uses of > those names. Yeah, we could define scoping rules such that there's > an unambiguous interpretation, but then the user is just out of luck > if he wants to reference the other definition. (This problem is > probably actually worse if you implement with reserved words rather > than aliases.) > > I'm thinking it would be better to invent some other notation for access > to old-row values. prior/after? Both are unreserved keywords atm and it seems far less likely to have conflicts than new/old. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
Marko Tiikkaja writes: > What I'm more interested in is: how can we make this feature work in > PL/PgSQL where OLD means something different? That's a really good point: if you follow this approach then you're creating fundamental conflicts for use of the feature in trigger functions or rules, which will necessarily have conflicting uses of those names. Yeah, we could define scoping rules such that there's an unambiguous interpretation, but then the user is just out of luck if he wants to reference the other definition. (This problem is probably actually worse if you implement with reserved words rather than aliases.) I'm thinking it would be better to invent some other notation for access to old-row values. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On 2013-05-02 17:32, Karol Trzcionka wrote: W dniu 02.05.2013 17:17, Tom Lane pisze: It should in any case be possible to do this without converting them to reserved words; rather the implementation could be that those table aliases are made available when parsing the UPDATE RETURNING expressions. (This is, in fact, the way that rules use these names now.) Probably it should work something like "add these aliases if they don't already exist in the query", so as to avoid breaking existing applications. I don't really see a lot of value in hacking the behavior of either INSERT RETURNING or DELETE RETURNING. what should happened in statement: UPDATE old SET foo=foo+1 RETURNING old.foo; If it returns old value, it'll break capability. If it returns new value (as now), there is no possibility to get old value (and user cries because of broken feature). In Tom's proposal that would give you the "new" value. Personally I would maybe prefer reserving NEW/OLD, but if we go through with Tom's idea, this should work: UPDATE old myold SET foo=foo+1 RETURNING myold.foo, old.foo; What I'm more interested in is: how can we make this feature work in PL/PgSQL where OLD means something different? Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
W dniu 02.05.2013 17:17, Tom Lane pisze: > It should in any case be possible to do this without converting them > to reserved words; rather the implementation could be that those table > aliases are made available when parsing the UPDATE RETURNING > expressions. (This is, in fact, the way that rules use these names > now.) Probably it should work something like "add these aliases if > they don't already exist in the query", so as to avoid breaking > existing applications. I don't really see a lot of value in hacking > the behavior of either INSERT RETURNING or DELETE RETURNING. regards, > tom lane I'm not sure about it. If it is not reserved keyword how can user get old value from table named "old". The new value is not a problem (doesn't conflict) but what should happened in statement: UPDATE old SET foo=foo+1 RETURNING old.foo; If it returns old value, it'll break capability. If it returns new value (as now), there is no possibility to get old value (and user cries because of broken feature). Regards, Karol Trzcionka -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
David Fetter writes: > 1. As the SQL standard mandates that OLD and NEW be reserved words, we'll > re-reserve them. As I mentioned yesterday, I'm not exactly thrilled with re-reserving those, and especially not NEW as it is utterly unnecessary (since the default would already be to return the NEW column). It should in any case be possible to do this without converting them to reserved words; rather the implementation could be that those table aliases are made available when parsing the UPDATE RETURNING expressions. (This is, in fact, the way that rules use these names now.) Probably it should work something like "add these aliases if they don't already exist in the query", so as to avoid breaking existing applications. I don't really see a lot of value in hacking the behavior of either INSERT RETURNING or DELETE RETURNING. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax
On Thu, May 02, 2013 at 11:04:15AM +0200, Karol Trzcionka wrote: > Hello, > I'm student who want to participate in Google Summer of Code. I want to > implement feature which allows to get old values directly from update > statement. I mean there should be possibility to choose the value > immedietly before or after update in RETURNING statement. The syntax may > be realized as "aliases". That means: OLD keywordswould be alias to row > before update and NEW to row after update. The conclusion of syntax is: > UPDATE foo SET bar=bar+1 RETURNING OLD.bar AS old_bar, NEW.bar AS new_bar; > UPDATE foo SET ... RETURNING NEW.* will be equivalent to UPDATE foo SET > ... RETURNING foo.* > It may be possible to add similar syntax to DELETE and INSERT statements > but I'm not sure if it makes a sense (OLD for DELETE will be alias to > row before delete, NEW for INSERT will be alias to row after insert and > all triggers - however what about NEW for delete and OLD for INSERT?). > Additionally NEW and OLD values will be reserved keywords (it might be > some capability problem since in new PostgreSQL it isn't reserved - > however standard says it is and in old PgSQL it was). > I'd like to hear (read) yours feedback about syntax and/or implement > issues related to this proposal. > Regards, > Karol Trzcionka I would like to include the proposal as we've hammered it out together on IRC and on GSoC site below. Cheers, David. 1. As the SQL standard mandates that OLD and NEW be reserved words, we'll re-reserve them. 2. Let's make OLD and NEW have the same meaning that INSERT/UPDATE/DELETE have when returning rows from the changed table. In particular INSERT INTO foo (...) RETURNING NEW.* will be equivalent to INSERT INTO foo(...) RETURNING foo.* Similarly for UPDATE and DELETE: UPDATE foo SET ... RETURNING NEW.* will be equivalent to UPDATE foo SET ... RETURNING foo.* and DELETE FROM foo ... RETURNING OLD.* will be equivalent to DELETE FROM foo ... RETURNING foo.* As RETURNING clauses have access to everything in the FROM/USING clause, it is important to limit the NEW/OLD rows as being only those in the table being written to in the statement. 3. Let's add an option to UPDATE so that it can RETURN OLD with the same characteristics as above, namely that it refers only to constants and columns in the updated table and not to everything available from the USING clause if included. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GSOC13 proposal - extend RETURNING syntax
Hello, I'm student who want to participate in Google Summer of Code. I want to implement feature which allows to get old values directly from update statement. I mean there should be possibility to choose the value immedietly before or after update in RETURNING statement. The syntax may be realized as "aliases". That means: OLD keywordswould be alias to row before update and NEW to row after update. The conclusion of syntax is: UPDATE foo SET bar=bar+1 RETURNING OLD.bar AS old_bar, NEW.bar AS new_bar; UPDATE foo SET ... RETURNING NEW.* will be equivalent to UPDATE foo SET ... RETURNING foo.* It may be possible to add similar syntax to DELETE and INSERT statements but I'm not sure if it makes a sense (OLD for DELETE will be alias to row before delete, NEW for INSERT will be alias to row after insert and all triggers - however what about NEW for delete and OLD for INSERT?). Additionally NEW and OLD values will be reserved keywords (it might be some capability problem since in new PostgreSQL it isn't reserved - however standard says it is and in old PgSQL it was). I'd like to hear (read) yours feedback about syntax and/or implement issues related to this proposal. Regards, Karol Trzcionka -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
