Re: GSOC13 proposal - extend RETURNING syntax - Mailing list pgsql-hackers

<div class="moz-cite-prefix">2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:<br /></div><blockquote
cite="mid:5214F63F.5050608@cybertec.at"type="cite"><div class="moz-cite-prefix">Hi,<br /><br /> 2013-08-20 21:06
keltezéssel,Karol Trzcionka írta:<br /></div><blockquote cite="mid:5213BE1D.1000705@gmail.com" type="cite"><div
class="moz-cite-prefix">Wdniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:<br /></div><blockquote
cite="mid:5213BB9D.4020204@cybertec.at"type="cite"> Here's a new one, for v7:<br /><br /> setrefs.c: In function
‘set_plan_refs’:<br/> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
[-Wmaybe-uninitialized]<br/>   bind_returning_variables(rlist, before_index, after_index);<br />
                         ^<br /> setrefs.c:1957:21: note: ‘before_index’ was declared here<br />   int after_index=0,
before_index;<br/>                      ^<br /></blockquote> Right, my mistake. Sorry and thanks. Fixed.<br />
Regards,<br/> Karol Trzcionka<br /></blockquote><br /> With this fixed, a more complete review:<br /><br /> * Is the
patchin a patch format which has context? (eg: context diff format)<br /><br /> Yes.<br /><br /> * Does it apply
cleanlyto the current git master?<br /><br /> Yes.<br /><br /> * Does it include reasonable tests, necessary doc
patches,etc? <br /><br /> There is a new regression test (returning_before_after.sql) covering<br /> this feature.
However,I think it should be added to the group<br /> where "returning.sql" resides currently. There is a value in
runningit<br /> in parallel to other tests. Sometimes a subtle bug is uncovered<br /> because of this and your v2 patch
fixedsuch a bug already.<br /><br /> doc/src/sgml/ref/update.sgml describes this feature.<br /><br />
doc/src/sgml/dml.sgmlshould also be touched to cover this feature.<br /><br /> * Does the patch actually implement what
it'ssupposed to do?<br /><br /> Yes.<br /><br /> * Do we want that?<br /><br /> Yes.<br /><br /> * Do we already have
it?<br/><br /> No.<br /><br /> * Does it follow SQL spec, or the community-agreed behavior?<br /><br /> RETURNING is a
PostgreSQLextension, so the SQL-spec part<br /> of the question isn't applicable.<br /><br /> It implements the
community-agreedbehaviour, according to<br /> the new regression test coverage.<br /><br /> * Does it include pg_dump
support(if applicable)?<br /><br /> n/a<br /><br /> * Are there dangers?<br /><br /> I don't think so.<br /><br /> *
Haveall the bases been covered?<br /><br /> It seems so. I have also tried mixing before/after columns in<br />
differentorders and the query didn't fail:<br /><br /> zozo=# create table t1 (id serial primary key, i1 int4, i2 int4,
t1text, t2 text);<br /> CREATE TABLE<br /> zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');<br /> INSERT
01<br /> zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');<br /> INSERT 0 1<br /> zozo=# insert into t1
(i1,i2, t1, t2) values (3, 3, 'c', 'c');<br /> INSERT 0 1<br /> zozo=# select * from t1;<br />  id | i1 | i2 | t1 | t2
<br/> ----+----+----+----+----<br />   1 |  1 |  1 | a  | a<br />   2 |  2 |  2 | b  | b<br />   3 |  3 |  3 | c  |
c<br/> (3 rows)<br /><br /> zozo=# begin;<br /> BEGIN<br /> zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id =
2returning before.i1, after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;<br />  i1 | i1 | i2 | i2
|t1 | t1 | t2 | t2  <br /> ----+----+----+----+----+----+----+-----<br />   2 |  2 |  2 |  4 | b  | b  | b  | bx2<br />
(1row)<br /><br /> UPDATE 1<br /> 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  <br /> ----+----+----+----+----+----+-----+-----<br />   3 |  3 |  9 |  6 | c  | c  | cx3 | cx2<br
/>(1 row)<br /><br /> UPDATE 1<br /><br /><br /><br /> * Does the feature work as advertised?<br /><br /> Yes.<br /><br
/>* Are there corner cases the author has failed to consider?<br /><br /> I don't know.<br /><br /> * Are there any
assertionfailures or crashes?<br /><br /> No.<br /><br /> * Does the patch slow down simple tests?<br /><br /> No.<br
/><br/> * If it claims to improve performance, does it?<br /><br /> n/a<br /><br /> * Does it slow down other things?
<br/><br /> No.<br /><br /> * Does it follow the project coding guidelines?<br /><br /> Mostly.<br /><br /> In the
src/test/regress/parallel_schedulecontains an extra<br /> new line at the end, it shouldn't.<br /><br /> In
b/src/backend/optimizer/plan/setrefs.c:<br/><br /> +static void bind_returning_variables(List *rlist, int bef, int
aft);<br/><br /> but later it becomes not public:<br /><br /> + */<br /> +void bind_returning_variables(List *rlist,
intbef, int aft)<br /> +{<br /><br /> Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward<br /> declaration
isnot needed, the function is called only from<br /> later functions.<br /><br /> Similar for parse_clause.c:<br /><br
/>+extern void addAliases(ParseState *pstate);<br />  <br /> +void addAliases(ParseState *pstate)<br /><br /> This
externaldeclaration is not needed since it is already<br /> in src/include/parser/parse_clause.h<br /><br /> In
setrefs.c,bind_returning_variables() I would also rename<br /> the function arguments, so "before" and "after" are
spelledout.<br /> These are not C keywords.<br /><br /> Some assignments, like:<br /><br /> +                      
var=(Var*)tle;<br/> and<br /> +       int index_rel=1;<br /><br /> in setrefs.c need spaces.<br /><br /> "if()"
statementsneed a space before the "(" and not after.<br /><br /> Add spaces in the {} list in addAliases():<br />
+      char    *aliases[] = {"before","after"};<br /> like this: { "before", "after" }<br /><br /> Spaces are needed
here,too:<br /> +       for(i=0 ; i<noal; i++)<br /><br /> This "noal" should be "naliases" or "n_aliases" if you
reallywant<br /> a variable. I would simply use the constant "2" for the two for()<br /> loops in addAliases() instead,
itspurpose is obvious enough.<br /><br /> In setrefs.c, bind_returning_variables():<br /> +       Var *var = NULL;<br
/>+       foreach(temp, rlist){<br /> Add an empty line after the declaration block.<br /><br /><br /> * Are there
portabilityissues?<br /><br /> No.<br /><br /> * Will it work on Windows/BSD etc?<br /><br /> Yes.<br /><br /> * Are
thecomments sufficient and accurate?<br /></blockquote><br /> There should be more comments, especially regarding<br />
myquestion at the end.<br /><br /><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"><br /><br /><br /> *
Doesit do what it says, correctly?<br /><br /> Yes.<br /><br /> * Does it produce compiler warnings?<br /><br /> No.<br
/><br/> * Can you make it crash?<br /><br /> No.<br /><br /> * Is everything done in a way that fits together
coherentlywith other features/modules?<br /><br /> I think so, mostly. Comments below.<br /><br /> * Are there
interdependenciesthat can cause problems?<br /><br /> I don't think so.<br /><br /> Other comments:<br /><br /> This
#definein pg_class:<br /><br /> diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h<br />
index49c4f6f..1b09994 100644<br /> --- a/src/include/catalog/pg_class.h<br /> +++ b/src/include/catalog/pg_class.h<br
/>@@ -154,6 +154,7 @@ DESCR("");<br />  #define                  RELKIND_COMPOSITE_TYPE  'c'           /* composite
type*/<br />  #define                  RELKIND_FOREIGN_TABLE   'f'           /* foreign table */<br />
 #define                 RELKIND_MATVIEW                 'm'           /* materialized view */<br />
+#define                 RELKIND_BEFORE                  'b'           /* virtual table for before/after statements
*/<br/>  <br />  #define                  RELPERSISTENCE_PERMANENT      'p'             /* regular table */<br />
 #define                 RELPERSISTENCE_UNLOGGED       'u'             /* unlogged permanent table */<br /><br /> The
"RELKIND_*"values all show up in the pg_class table except<br /> this new one. I don't think pg_class.h should be
modifiedat all.<br /> addAliases() should use RELKIND_RELATION together with<br /> RTE_BEFORE. Then checks like:<br
/><br/> +               if (rte->relkind == RELKIND_BEFORE)<br /> +                       continue;<br /><br />
shouldbecome<br /><br /> +               if (rte->relkind == RELKIND_RELATION && rte->rtekind ==
RTE_BEFORE)<br/> +                       continue;<br /></blockquote><br /> Thinking about it more,<br />
                   if (rte->rtekind == RTE_BEFORE)<br /> would be enough, as no other kinds of rte's can have
rtekind== RTE_BEFORE.<br /><br /><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"><br /> Speaking of
which,RTE_BEFORE is more properly named<br /> RTE_RETURNING_ALIAS or something like that because it<br /> covers both
"before"and "after". Someone may have a better<br /> idea for naming this symbol.<br /><br /> I feel like I understand
whatthe code does and it looks sane to me.<br /><br /> One question, though, about this part:<br /><br />
----------------------------------------<br/> @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo
*root,<br/>                                                                 int rtoffset)<br />  {<br />        
indexed_tlist*itlist;<br /> +       int after_index=0, before_index=0;<br /> +       Query      *parse =
root->parse;<br/>  <br /> +       ListCell   *rt;<br /> +       RangeTblEntry *bef;<br /> +<br /> +       int
index_rel=1;<br/> +<br /> +       foreach(rt,parse->rtable)<br /> +       {<br /> +               bef =
(RangeTblEntry*)lfirst(rt);<br /> +               if(strcmp(bef->eref->aliasname,"after") == 0 &&
bef->rtekind== RTE_BEFORE )<br /> +               {<br /> +                       after_index = index_rel;<br />
+              }<br /> +               if(strcmp(bef->eref->aliasname,"before") == 0 && bef->rtekind
==RTE_BEFORE )<br /> +               {<br /> +                       before_index = index_rel;<br /> +              
}<br/> +               index_rel++;<br /> +       }<br />         /*<br />          * We can perform the desired Var
fixupby abusing the fix_join_expr<br />          * machinery that formerly handled inner indexscan fixup.  We search
the<br/> @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,<br />
                                                 resultRelation,<br />
                                                 rtoffset);<br />  <br /> +       bind_returning_variables(rlist,
before_index,after_index);<br />         pfree(itlist);<br />  <br />         return rlist;<br />
----------------------------------------<br/><br /> Why is it enough to keep the last before_index and after_index
values?<br/> What if there are more than one matching RangeTblEntry for "before"<br /> and/or for "after"? Is it an
errorcondition or of them should be fixed?<br /><br /> I think that's all for now.<br /><br /> Best regards,<br />
ZoltánBöszörményi<br /><br /><pre class="moz-signature" cols="90">-- 
 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: <a class="moz-txt-link-freetext" href="http://www.postgresql-support.de"
moz-do-not-send="true">http://www.postgresql-support.de</a>   <a class="moz-txt-link-freetext"
href="http://www.postgresql.at/"moz-do-not-send="true">http://www.postgresql.at/</a>
 
</pre></blockquote><br /><br /><pre class="moz-signature" cols="90">-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: <a class="moz-txt-link-freetext" href="http://www.postgresql-support.de">http://www.postgresql-support.de</a>
<aclass="moz-txt-link-freetext" href="http://www.postgresql.at/">http://www.postgresql.at/</a>
 
</pre>