Re: review: Non-recursive processing of AND/OR lists

On Tue, Jun 18, 2013 at 3:01 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

>
> related to
>
> https://commitfest.postgresql.org/action/patch_view?id=1130
>
> http://www.postgresql.org/message-id/CABwTF4V9rsjiBWE+87pK83Mmm7ACdrG7sZ08RQ-4qYMe8jvhbw@mail.gmail.com
>
>
> * motivation: remove recursive procession of AND/OR list (hangs with
> 10062 and more subexpressions)
>
> * patch is short, clean and respect postgresql source code requirements
> * patch was applied cleanly without warnings
> * all regression tests was passed
> * I successfully evaluated expression with 100000 subexpressions
> * there is no significant slowdown
>
> possible improvements
>
> a = (A_Expr*) list_nth(pending, 0);
>
> a = (A_Expr*) linitial(pending);
>

I made that change, hesitantly. The comments above definition of linitial()
macro describe the confusion that API causes. I wanted to avoid that
confusion for new code, so I used the newer API which makes the intention
quite clear. But looking at that code closely, list_nth() causes at least 2
function calls, and that's pretty heavy compared to the linitiali() macro.

>
> not well comment
>
> should be -- "If the right branch is also an SAME condition, append it to
> the"
>

I moved that comment above the outer bock, so that the intention of the
whole do-while code block is described in one place.

I don't see any other issues, so after fixing comments this patch is
> ready for commit
>

Thanks for the review Pavel.

Attached is the updated patch, v4. It has the above edits, and a few code
improvements, like not repeating the (root_kind == AEPR_AND ? .. : ..)
ternary expression.

Best regards,
--
Gurjeet Singh

http://gurjeet.singh.im/

EnterpriseDB Inc.