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

With this fixed, a more complete review:

Thanks.

There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where "returning.sql" resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.

Modified to work correct in parallel testing

doc/src/sgml/ref/update.sgml describes this feature.

doc/src/sgml/dml.sgml should also be touched to cover this feature.

I don't think so, there is not any info about returning feature, I think it shouldn't be part of my patch.

In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.

Fixed

In b/src/backend/optimizer/plan/setrefs.c:

+static void bind_returning_variables(List *rlist, int bef, int aft);

but later it becomes not public:

+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{

I've change to static in the definition.

+extern void addAliases(ParseState *pstate);
 
+void addAliases(ParseState *pstate)

This external declaration is not needed since it is already
in src/include/parser/parse_clause.h

Removed.

In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so "before" and "after" are spelled out.
These are not C keywords.

Changed.

Some assignments, like:

+                       var=(Var*)tle;
and
+       int index_rel=1;

in setrefs.c need spaces.

"if()" statements need a space before the "(" and not after.

Add spaces in the {} list in addAliases():
+       char    *aliases[] = {"before","after"};
like this: { "before", "after" }

Spaces are needed here, too:
+       for(i=0 ; i<noal; 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 there are more than one matching RangeTblEntry for "before"
and/or for "after"? Is it an error condition or of them should be fixed?

I think it is safe, it is the first and the last index. On each level of statement there can be (at most) the only one "before" and one "after" alias.
Regards,
Karol Trzcionka