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

<div class="moz-cite-prefix">Hi,<br /><br /> 2013-08-21 20:52 keltezéssel, Karol Trzcionka írta:<br /></div><blockquote
cite="mid:52150C69.8030608@gmail.com"type="cite"><div class="moz-cite-prefix">W dniu 21.08.2013 19:17, Boszormenyi
Zoltanpisze:<br /></div><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"> With this fixed, a more
completereview:<br /></blockquote> Thanks.<br /><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"> There
isa new regression test (returning_before_after.sql) covering<br /> this feature. However, I think it should be added
tothe group<br /> where "returning.sql" resides currently. There is a value in running it<br /> in parallel to other
tests.Sometimes a subtle bug is uncovered<br /> because of this and your v2 patch fixed such a bug already.<br
/></blockquote>Modified to work correct in parallel testing<br /><blockquote cite="mid:5214F63F.5050608@cybertec.at"
type="cite">doc/src/sgml/ref/update.sgml describes this feature.<br /><br /> doc/src/sgml/dml.sgml should also be
touchedto cover this feature.<br /></blockquote> I don't think so, there is not any info about returning feature, I
thinkit shouldn't be part of my patch.<br /><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"> In the
src/test/regress/parallel_schedulecontains an extra<br /> new line at the end, it shouldn't.<br /></blockquote>
Fixed<br/><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"><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 /></blockquote> I've change to static in the definition.<br /><blockquote
cite="mid:5214F63F.5050608@cybertec.at"type="cite"><br /> +extern void addAliases(ParseState *pstate);<br />  <br />
+voidaddAliases(ParseState *pstate)<br /><br /> This external declaration is not needed since it is already<br /> in
src/include/parser/parse_clause.h<br/></blockquote> Removed.<br /><blockquote cite="mid:5214F63F.5050608@cybertec.at"
type="cite"><br/> In setrefs.c, bind_returning_variables() I would also rename<br /> the function arguments, so
"before"and "after" are spelled out.<br /> These are not C keywords.<br /></blockquote> Changed.<br /><blockquote
cite="mid:5214F63F.5050608@cybertec.at"type="cite"> 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 /></blockquote> Added some whitespaces.<br /><blockquote
cite="mid:5214F63F.5050608@cybertec.at"type="cite"> In setrefs.c, bind_returning_variables():<br /> +       Var *var =
NULL;<br/> +       foreach(temp, rlist){<br /> Add an empty line after the declaration block.<br /></blockquote>
Added.<br/><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"> Other comments:<br /><br /> This #define in
pg_class:<br/><br /> diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h<br /> index
49c4f6f..1b09994100644<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
/></blockquote>RELKIND_BEFORE removed - it probably left over previous work and/or I needed it because RTE_BEFORE
wasn'tavailable (not included?).<br /><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"> 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 /></blockquote> Renamed to
RTE_ALIAS- similar to addAliases (not addReturningAliases)<br /></blockquote><br /> Others may have also a word in this
topic,but consider that<br /> this is *not* a regular alias and for those, RTE_ALIAS is not used,<br /> like in<br
/><br/>     UPDATE table AS alias SET ...<br /><br /> Maybe RTE_RETURNING_ALIAS takes a little more typing, but<br />
itbecomes obvious when reading and new code won't confuse<br /> it with regular aliases.<br /><br /><blockquote
cite="mid:52150C69.8030608@gmail.com"type="cite"><blockquote cite="mid:5214F63F.5050608@cybertec.at" type="cite"> One
question,though, about this part:<br /><br /> ----------------------------------------<br /> @@ -1900,7 +1954,27 @@
set_returning_clause_references(PlannerInfo*root,<br />                                                                
intrtoffset)<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
thedesired 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_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 /></blockquote> I think it is safe, it is the first and the last index.
Oneach level of statement there can be (at most) the only one "before" and one "after" alias.<br /></blockquote><br />
Ideduced it in the meantime. I still think it worth a comment<br /> in setrefs.c.<br /><br /> I think your v9 patch can
belooked at by a more seasoned reviewer<br /> or a committer.<br /><br /> Best regards,<br /> Zoltán Böszörményi<br
/><br/><blockquote cite="mid:52150C69.8030608@gmail.com" type="cite"> Regards,<br /> Karol Trzcionka<br /><br
/><fieldsetclass="mimeAttachmentHeader"></fieldset><br /><pre wrap="">
 
</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>