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/
