Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jul 17, 2013 at 2:03 PM, Gurjeet Singh <gurjeet@singh.im> wrote:
>> Agreed that bushy/right-deep trees are a remote corner case, but we are
>> addressing a remote corner case in the first place (insanely long AND lists)
>> and why not handle another remote corner case right now if it doesn't cause
>> an overhead for common case.
> 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, but maybe you just haven't
thought about it the right way.
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 --- you have not even updated the comment for BoolExpr
(in primnodes.h) that this patch falsifies.
> It's worth noting that the change you're proposing is in
> fact user-visible, as demonstrated by the fact that you had to update
> the regression test output:
The point to worry about here is whether rule dump and reload is still
safe. In particular, the logic in ruleutils.c for deciding whether it's
safe to omit parentheses has only really been thought about/tested for
the binary AND/OR case. Although that code can dump N-way AND/OR
because it's also used to print post-planner expression trees in EXPLAIN,
that case has never been held to the standard of "is the parser
guaranteed to interpret this expression the same as before?". Perhaps
it's fine, but has anyone looked at that issue?
regards, tom lane