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