Re: GSOC13 proposal - extend RETURNING syntax

Hi,

2013-08-21 20:52 keltezéssel, Karol Trzcionka írta:
> 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)

Others may have also a word in this topic, but consider that
this is *not* a regular alias and for those, RTE_ALIAS is not used,
like in

UPDATE table AS alias SET ...

Maybe RTE_RETURNING_ALIAS takes a little more typing, but
it becomes obvious when reading and new code won't confuse
it with regular aliases.

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

I deduced it in the meantime. I still think it worth a comment
in setrefs.c.

I think your v9 patch can be looked at by a more seasoned reviewer
or a committer.

Best regards,
Zoltán Böszörményi

> Regards,
> Karol Trzcionka
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/