subject:"Re\: \[HACKERS\] GSOC13 proposal \- extend RETURNING syntax"

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2014-05-15 Thread Arthur Axel 'fREW' Schmidt

Andres Freund andres at 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

2014-04-04 Thread Andres Freund

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

2014-02-11 Thread David Fetter

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 da...@fetter.org 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 ] replaceable 
class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable 
class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,12 +302,12 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = 
temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
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 =

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2014-02-02 Thread David Fetter

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 da...@fetter.org 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 ] replaceable 
class=PARAMETERtable_name/replaceable [ * ] [
  termreplaceable 
class=PARAMETERoutput_expression/replaceable/term
  listitem
   para
!   An expression to be computed and returned by the commandUPDATE/
!   command after each row is updated.  The expression can use any
!   column names of the table named by replaceable 
class=PARAMETERtable_name/replaceable
!   or table(s) listed in literalFROM/.
!   Write literal*/ to return all columns.
   /para
  /listitem
 /varlistentry
  
--- 194,220 
  termreplaceable 
class=PARAMETERoutput_expression/replaceable/term
  listitem
   para
!   An expression to be computed and returned by the
!   commandUPDATE/ command either before or after (prefixed with
!   literalBEFORE./literal and literalAFTER./literal,
!   respectively) each row is updated.  The expression can use any
!   column names of the table named by replaceable
!   class=PARAMETERtable_name/replaceable or table(s) listed in
!   literalFROM/.  Write literalAFTER.*/literal to return all 
!   columns after the update. Write literalBEFORE.*/literal for all
!   columns before the update. Write literal*/literal 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.
   /para
+  warningpara
+  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 literalbefore/literal or
+  literalafter/literal, alias it to something else when using
+  RETURNING.
+  /para/warning
+ 
  /listitem
 /varlistentry
  
***
*** 287,301  UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, 
prcp = DEFAULT
/para
  
para
!Perform the same operation and return the updated entries:
  
  programlisting
  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;
  /programlisting
/para
  
para
 Use the alternative column-list syntax to do the same update:
  programlisting
--- 302,317 
/para
  
para
!Perform the same operation and return information on the changed entries:
  
  programlisting
  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;
  /programlisting
/para
  
+ 
para
 Use the alternative column-list syntax to do the same update:
  programlisting
*** 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
***

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-11-21 Thread David Fetter

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 da...@fetter.org 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

2013-10-04 Thread Karol Trzcionka

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

2013-10-04 Thread Marko Tiikkaja


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

2013-10-04 Thread Andres Freund

On 2013-10-03 20:51:08 -0400, Robert Haas wrote:
 On Thu, Oct 3, 2013 at 7:54 PM, Karol Trzcionka karl...@gmail.com 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

2013-10-04 Thread Robert Haas

On Fri, Oct 4, 2013 at 4:42 AM, Karol Trzcionka karl...@gmail.com 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

2013-10-04 Thread Robert Haas

On Fri, Oct 4, 2013 at 5:04 AM, Marko Tiikkaja ma...@joh.to 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

2013-10-03 Thread Robert Haas

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

2013-10-03 Thread Karol Trzcionka

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

2013-10-03 Thread Robert Haas

On Thu, Oct 3, 2013 at 7:54 PM, Karol Trzcionka karl...@gmail.com 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

2013-08-21 Thread Boszormenyi Zoltan


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 ; inoal; 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.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block.


* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

Yes.

* Are the comments sufficient

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-21 Thread Boszormenyi Zoltan


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 ; inoal; 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.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block.


* Are there portability issues?

No.

* Will it work

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-21 Thread Boszormenyi Zoltan


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 ; inoal; 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.

In setrefs.c, bind_returning_variables():
+   Var *var = NULL;
+   foreach(temp, rlist){
Add an empty line after the declaration block.

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-21 Thread Karol Trzcionka

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 ; inoal; 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(itlist);
  
 return rlist;
 

 Why is it enough to keep the last before_index and after_index values?
 What if

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-21 Thread Boszormenyi Zoltan


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 ; inoal; 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)


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;

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-20 Thread Boszormenyi Zoltan


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 | bar1 | bar2 | bar 
!

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-20 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-20 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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;

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-20 Thread Boszormenyi Zoltan


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

2013-08-20 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-19 Thread Boszormenyi Zoltan


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

2013-08-19 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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 @@

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-08-19 Thread Boszormenyi Zoltan


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

2013-07-23 Thread Karol Trzcionka

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

2013-07-22 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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 @@

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-07-22 Thread David Fetter

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 da...@fetter.org 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

2013-07-19 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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
---

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-07-13 Thread David Fetter

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 da...@fetter.org 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

2013-07-12 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-07-05 Thread David Fetter

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 da...@fetter.org 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

2013-07-05 Thread Karol Trzcionka

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 ] replaceable class=PARAMETERtable_name/replaceable [ * ] [
 termreplaceable class=PARAMETERoutput_expression/replaceable/term
 listitem
  para
-  An expression to be computed and returned by the commandUPDATE/
-  command after each row is updated.  The expression can use any
-  column names of the table named by replaceable class=PARAMETERtable_name/replaceable
-  or table(s) listed in literalFROM/.
-  Write literal*/ to return all columns.
+  An expression to be computed and returned by the
+  commandUPDATE/ command either before or after (prefixed with
+  literalBEFORE./literal and literalAFTER./literal,
+  respectively) each row is updated.  The expression can use any
+  column names of the table named by replaceable
+  class=PARAMETERtable_name/replaceable or table(s) listed in
+  literalFROM/.  Write literalAFTER.*/literal to return all 
+  columns after the update. Write literalBEFORE.*/literal for all
+  columns before the update. Write literal*/literal 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.
  /para
+ warningpara
+ 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 literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
 /listitem
/varlistentry
 
@@ -287,15 +302,16 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
   /para
 
   para
-   Perform the same operation and return the updated entries:
+   Perform the same operation and return information on the changed entries:
 
 programlisting
 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;
 /programlisting
   /para
 
+
   para
Use the alternative column-list syntax to do the same update:
 programlisting
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
+++

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-07-04 Thread Karol Trzcionka

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-varno1  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 *bef;
 
+	TargetEntry *tr;
+
+	int index_rel=1;
+	int index_var=0;
+
+
+

Re: [HACKERS] GSOC13 proposal - extend RETURNING syntax

2013-05-02 Thread David Fetter

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 da...@fetter.org 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

2013-05-02 Thread Tom Lane

David Fetter da...@fetter.org 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

2013-05-02 Thread Karol Trzcionka

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

2013-05-02 Thread Marko Tiikkaja


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

2013-05-02 Thread Tom Lane

Marko Tiikkaja ma...@joh.to 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

2013-05-02 Thread Andres Freund

On 2013-05-02 12:23:19 -0400, Tom Lane wrote:
 Marko Tiikkaja ma...@joh.to 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

2013-05-02 Thread David Fetter

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 ma...@joh.to 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 da...@fetter.org 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

2013-05-02 Thread Pavel Stehule

2013/5/2 Tom Lane t...@sss.pgh.pa.us

 Marko Tiikkaja ma...@joh.to 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

2013-05-02 Thread Tom Lane

David Fetter da...@fetter.org 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-05-02 Thread Karol Trzcionka

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

2013-05-02 Thread David Fetter

On Thu, May 02, 2013 at 01:40:59PM -0400, Tom Lane wrote:
 David Fetter da...@fetter.org 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 da...@fetter.org 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

2013-05-02 Thread Atri Sharma



Sent from my iPad

On 03-May-2013, at 0:07, David Fetter da...@fetter.org wrote:

 On Thu, May 02, 2013 at 01:40:59PM -0400, Tom Lane wrote:
 David Fetter da...@fetter.org 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

2013-05-02 Thread Tom Lane

David Fetter da...@fetter.org 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

2013-05-02 Thread Tom Lane

Karol Trzcionka karl...@gmail.com 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

2013-05-02 Thread Karol Trzcionka

W dniu 02.05.2013 20:45, Tom Lane pisze:
 David Fetter da...@fetter.org 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$ x 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

2013-05-02 Thread Gavin Flower


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 ma...@joh.to 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

2013-05-02 Thread Karol Trzcionka

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

2013-05-02 Thread Tom Lane

Karol Trzcionka karl...@gmail.com 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

53 matches

Mail list logo