On Sun, Jun 29, 2014 at 4:18 PM, David Rowley <dgrowle...@gmail.com> wrote:
> I think I'm finally ready for a review again, so I'll update the
> commitfest app.
>
>
I have reviewed this on code level.
1. Patch gets applied cleanly.
2. make/make install/make check all are fine
No issues found till now.
However some cosmetic points:
1.
* The API of this function is identical to convert_ANY_sublink_to_join's,
* except that we also support the case where the caller has found NOT
EXISTS,
* so we need an additional input parameter "under_not".
Since now, we do support NOT IN handling in convert_ANY_sublink_to_join() we
do have "under_not" parameter there too. So above comments near
convert_EXISTS_sublink_to_join() function is NO more valid.
2. Following is the unnecessary change. Can be removed:
@@ -506,6 +560,7 @@ pull_up_sublinks_qual_recurse(PlannerInfo *root, Node
*node,
return NULL;
}
}
+
}
/* Else return it unmodified */
return node;
3. Typos:
a.
+ * queryTargetListListCanHaveNulls
...
+queryTargetListCanHaveNulls(Query *query)
Function name is queryTargetListCanHaveNulls() not
queryTargetListListCanHaveNulls()
b.
* For example the presense of; col IS NOT NULL, or col = 42 would
allow
presense => presence
4. get_attnotnull() throws an error for invalid relid/attnum.
But I see other get_att* functions which do not throw an error rather
returning some invalid value, eg. NULL in case of attname, InvalidOid in
case of atttype and InvalidAttrNumber in case of attnum. I have observed
that we cannot return such invalid value for type boolean and I guess that's
the reason you are throwing an error. But somehow looks unusual. You had put
a note there which is good. But yes, caller of this function should be
careful enough and should not assume its working like other get_att*()
functions.
However for this patch, I would simply accept this solution. But I wonder if
someone thinks other way round.
Testing more on SQL level.
However, assigning it to author to think on above cosmetic issues.
Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
