Review for EXPLAIN and nfiltered

Here is my review for EXPLAIN and nfiltered
(http://archives.postgresql.org/message-id/4E6E9E83.7070303@2ndquadrant.com)

- Is the patch in context diff format?
It's in git diff format

- Does it apply cleanly to the current git master?
Yes

- Does it include reasonable tests, necessary doc patches, etc?
I think so.

Comments in execnodes.h are not updated with new fields
(ss_qualnremoved and others). They are commented directly in the
definition. Very minor, comments should just be moved around.

I have the same problem as explained in the original mail finding
an appropriate place in the documentation.
It could be added on EXPLAIN's page, but that would mean changing the
example statement for one with a filter.

- Does the patch actually implement that?
Yes

- Do we want that?
I'd like to have that. It makes it easier to determine if an index is
selective enough. Some people might prefer having it through an option
of EXPLAIN (such as EXPLAIN (filters on) ), I don't really know.

- Do we already have it?
No.

- Does it follow SQL spec, or the community-agreed behavior?
Nothing about this in the SQL spec, obviously, and I didn't see anyone
disagreeing with the proposal.

- Does it include pg_dump support (if applicable)?
Not applicable

- Are there dangers?
Not that I think of

- Have all the bases been covered?
Yes, I think so.

- Does the feature work as advertised?
Yes

- Are there corner cases the author has failed to consider?
I didn't find any

- Are there any assertion failures or crashes?
No

- Does the patch slow down simple tests?
No

- If it claims to improve performance, does it?
Not applicable

- Does it slow down other things?
No

- Does it follow the project coding guidelines?
Yes

- Are there portability issues?
No

- Will it work on Windows/BSD etc?
Yes

- Are the comments sufficient and accurate?
Yes, except for the minor inconsistancy in execnodes.h

- 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?
Yes

- Are there interdependencies that can cause problems?
Not that I can see

On Mon, Sep 19, 2011 at 12:51 PM, Marc Cousin <cousinmarc(at)gmail(dot)com> wrote:
> Here is my review for EXPLAIN and nfiltered
> (http://archives.postgresql.org/message-id/4E6E9E83.7070303@2ndquadrant.com)

Please add this review to the CommitFest app here:

https://commitfest.postgresql.org/action/patch_view?id=638

...and also update the patch status as appropriate. Sounds like Ready
for Committer would be the way to go in this case, since your review
found only trivial issues.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/9/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Sep 19, 2011 at 12:51 PM, Marc Cousin <cousinmarc(at)gmail(dot)com> wrote:
>> Here is my review for EXPLAIN and nfiltered
>> (http://archives.postgresql.org/message-id/4E6E9E83.7070303@2ndquadrant.com)
>
> Please add this review to the CommitFest app here:
>
> https://commitfest.postgresql.org/action/patch_view?id=638
>
> ...and also update the patch status as appropriate.  Sounds like Ready
> for Committer would be the way to go in this case, since your review
> found only trivial issues.
>

Sorry, I wanted to, but I didn't get the email id fast enough, and had
to go. It's done now.

On Mon, Sep 19, 2011 at 3:10 PM, Marc Cousin <cousinmarc(at)gmail(dot)com> wrote:
> 2011/9/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Sep 19, 2011 at 12:51 PM, Marc Cousin <cousinmarc(at)gmail(dot)com> wrote:
>>> Here is my review for EXPLAIN and nfiltered
>>> (http://archives.postgresql.org/message-id/4E6E9E83.7070303@2ndquadrant.com)
>>
>> Please add this review to the CommitFest app here:
>>
>> https://commitfest.postgresql.org/action/patch_view?id=638
>>
>> ...and also update the patch status as appropriate.  Sounds like Ready
>> for Committer would be the way to go in this case, since your review
>> found only trivial issues.
>>
>
> Sorry, I wanted to, but I didn't get the email id fast enough, and had
> to go. It's done now.

No problem, didn't mean to nag. Thanks!

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company