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

<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">W dniu 20.08.2013 20:55, Boszormenyi
Zoltanpisze:<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
uninitializedin 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 />  
intafter_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 patch in a patch format which has context? (eg: context diff format)<br /><br /> Yes.<br /><br /> * Does it
applycleanly to 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 /><br /><br /><br /> * Does it 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 coherently with other features/modules?<br /><br /> I think
so,mostly. Comments below.<br /><br /> * Are there interdependencies that can cause problems?<br /><br /> I don't think
so.<br/><br /> Other comments:<br /><br /> This #define in pg_class:<br /><br /> diff --git
a/src/include/catalog/pg_class.hb/src/include/catalog/pg_class.h<br /> index 49c4f6f..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 /><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 what the 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 fixup by 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_indexvalues?<br /> What if there are more than one matching RangeTblEntry for "before"<br /> and/or for "after"?
Isit an error condition or of them should be fixed?<br /><br /> I think that's all for now.<br /><br /> Best
regards,<br/> Zoltán Bö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">http://www.postgresql-support.de</a>
<aclass="moz-txt-link-freetext" href="http://www.postgresql.at/">http://www.postgresql.at/</a>
 
</pre>