Re: GSOC13 proposal - extend RETURNING syntax

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?

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

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/