Re: GSOC13 proposal - extend RETURNING syntax

2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:
> Hi,
>
> 2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
>> W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
>>> Here's a new one, for v7:
>>>
>>> setrefs.c: In function ‘set_plan_refs’:
>>> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
>>> [-Wmaybe-uninitialized]
>>> bind_returning_variables(rlist, before_index, after_index);
>>> ^
>>> setrefs.c:1957:21: note: ‘before_index’ was declared here
>>> int after_index=0, before_index;
>>> ^
>> Right, my mistake. Sorry and thanks. Fixed.
>> Regards,
>> Karol Trzcionka
>
> With this fixed, a more complete review:
>
> * Is the patch in a patch format which has context? (eg: context diff format)
>
> Yes.
>
> * Does it apply cleanly to the current git master?
>
> Yes.
>
> * Does it include reasonable tests, necessary doc patches, etc?
>
> 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.
>
> doc/src/sgml/ref/update.sgml describes this feature.
>
> doc/src/sgml/dml.sgml should also be touched to cover this feature.
>
> * Does the patch actually implement what it's supposed to do?
>
> Yes.
>
> * Do we want that?
>
> Yes.
>
> * Do we already have it?
>
> No.
>
> * Does it follow SQL spec, or the community-agreed behavior?
>
> RETURNING is a PostgreSQL extension, so the SQL-spec part
> of the question isn't applicable.
>
> It implements the community-agreed behaviour, according to
> the new regression test coverage.
>
> * Does it include pg_dump support (if applicable)?
>
> n/a
>
> * Are there dangers?
>
> I don't think so.
>
> * Have all the bases been covered?
>
> It seems so. I have also tried mixing before/after columns in
> different orders and the query didn't fail:
>
> zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text);
> CREATE TABLE
> zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
> INSERT 0 1
> zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
> INSERT 0 1
> zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
> INSERT 0 1
> zozo=# select * from t1;
> id | i1 | i2 | t1 | t2
> ----+----+----+----+----
> 1 | 1 | 1 | a | a
> 2 | 2 | 2 | b | b
> 3 | 3 | 3 | c | c
> (3 rows)
>
> zozo=# begin;
> BEGIN
> zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
> after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
> i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
> ----+----+----+----+----+----+----+-----
> 2 | 2 | 2 | 4 | b | b | b | bx2
> (1 row)
>
> UPDATE 1
> 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
> ----+----+----+----+----+----+-----+-----
> 3 | 3 | 9 | 6 | c | c | cx3 | cx2
> (1 row)
>
> UPDATE 1
>
>
>
> * Does the feature work as advertised?
>
> Yes.
>
> * Are there corner cases the author has failed to consider?
>
> I don't know.
>
> * Are there any assertion failures or crashes?
>
> No.
>
> * Does the patch slow down simple tests?
>
> No.
>
> * If it claims to improve performance, does it?
>
> n/a
>
> * Does it slow down other things?
>
> No.
>
> * Does it follow the project coding guidelines?
>
> Mostly.
>
> In the src/test/regress/parallel_schedule contains an extra
> new line at the end, it shouldn't.
>
> 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)
> +{
>
> Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
> declaration is not needed, the function is called only from
> later functions.
>
> Similar for parse_clause.c:
>
> +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
>
> 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.
>
> 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.
>
> In setrefs.c, bind_returning_variables():
> + Var *var = NULL;
> + foreach(temp, rlist){
> Add an empty line after the declaration block.
>
>
> * Are there portability issues?
>
> No.
>
> * Will it work on Windows/BSD etc?
>
> Yes.
>
> * Are the comments sufficient and accurate?

There should be more comments, especially regarding
my question at the end.

>
>
>
> * Does it do what it says, correctly?
>
> Yes.
>
> * Does it produce compiler warnings?
>
> No.
>
> * Can you make it crash?
>
> No.
>
> * Is everything done in a way that fits together coherently with other features/modules?
>
> I think so, mostly. Comments below.
>
> * Are there interdependencies that can cause problems?
>
> I don't think so.
>
> 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 */
>
> The "RELKIND_*" values all show up in the pg_class table except
> this new one. I don't think pg_class.h should be modified at all.
> addAliases() should use RELKIND_RELATION together with
> RTE_BEFORE. Then checks like:
>
> + if (rte->relkind == RELKIND_BEFORE)
> + continue;
>
> should become
>
> + if (rte->relkind == RELKIND_RELATION && rte->rtekind == RTE_BEFORE)
> + continue;

Thinking about it more,
if (rte->rtekind == RTE_BEFORE)
would be enough, as no other kinds of rte's can have rtekind == RTE_BEFORE.

>
> 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.
>
> I feel like I understand what the code does and it looks sane to me.
>
> 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 that's all for now.
>
> Best regards,
> Zoltán Böszörményi
>
> --
> ----------------------------------
> 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/

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