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

Поиск
Список
Период
Сортировка
От Gurjeet Singh
Тема Re: review: Non-recursive processing of AND/OR lists
Дата
Msg-id CABwTF4U+7M7jtY0=_bu6nYhwidW1O-=tucoMLyh+WW0-pn4Cew@mail.gmail.com
обсуждение исходный текст
Ответ на Re: review: Non-recursive processing of AND/OR lists  (Gurjeet Singh <gurjeet@singh.im>)
Ответы Re: review: Non-recursive processing of AND/OR lists  (Tom Lane <tgl@sss.pgh.pa.us>)
Список pgsql-hackers
On Thu, Jul 18, 2013 at 1:54 PM, Gurjeet Singh <gurjeet@singh.im> wrote:
On Thu, Jul 18, 2013 at 10:19 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Because simpler code is less likely to have bugs and is easier to
> maintain.

I agree with that point, but one should also remember Polya's Inventor's
Paradox: the more general problem may be easier to solve.  That is, if
done right, code that fully flattens an AND tree might actually be
simpler than code that does just a subset of the transformation.  The
current patch fails to meet this expectation,

The current patch does completely flatten any type of tree (left/right-deep or bushy) without recursing, and right-deep and bushy tree processing is what Robert is recommending to defer to recursive processing. Maybe I haven't considered a case where it doesn't flatten the tree; do you have an example in mind.
 
but maybe you just haven't
thought about it the right way.

I tried to eliminate the 'pending' list, but I don't see a way around it. We need temporary storage somewhere to store the branches encountered on the right; in recursion case the call stack was serving that purpose.
 

My concerns about this patch have little to do with that, though, and
much more to do with the likelihood that it breaks some other piece of
code that is expecting AND/OR to be strictly binary operators, which
is what they've always been in parsetrees that haven't reached the
planner.  It doesn't appear to me that you've done any research on that
point whatsoever

No, I haven't, and I might not be able to research it for a few more weeks.

There are about 30 files (including optimizer and executor) that match case-insensitive search for BoolExpr, and I scanned those for the usage of the member 'args'. All the instances where BoolExpr.args is being accessed, it's being treated as a null-terminated list. There's one exception that I could find, and it was in context of NOT expression: not_clause() in clauses.c.
 
 
you have not even updated the comment for BoolExpr
(in primnodes.h) that this patch falsifies.

I will fix that.

I think this line in that comment already covers the fact that in some "special" cases a BoolExpr may have more than 2 arguments.

"There are also a few special cases where more arguments can appear before optimization."

I have updated the comment nevertheless, and removed another comment in parse_expr.c that claimed to be the only place where a BoolExpr with more than 2 args is generated.

I have isolated the code for right-deep and bushy tree processing via the macro PROCESS_BUSHY_TREES. Also, I have shortened some variable names while retaining their meaning.

Please find the updated patch attached (based on master).

Best regards,
Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Craig Ringer
Дата:
Сообщение: Re: API change advice: Passing plan invalidation info from the rewriter into the planner?
Следующее
От: Stephen Frost
Дата:
Сообщение: Re: API change advice: Passing plan invalidation info from the rewriter into the planner?