On Thu, Sep 5, 2013 at 02:14:39PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Sep 4, 2013 at 9:26 PM, Bruce Momjian <bruce@momjian.us> wrote:
> >> I have not heard any feedback on this patch, so I would like to apply it
> >> to give us a nested ROW/IS NULL API we can document. It would have to
> >> be marked in the release notes as a backward incompatibility.
>
> > I don't have time to look at this in detail right now, but I think
> > that's considerably premature. I'm not convinced that we understand
> > all of the problems in this area are yet, let alone the solutions.
> > And I notice that you haven't substantively responded to some of Tom's
> > concerns.
>
> In particular, I don't think it's a good idea to change
> eval_const_expression's behavior in an incompatible way that simply
> makes it differently inconsistent with other IS NULL code paths.
Let me summarize what eval_const_expressions_mutator() does: it takes a
ROW() IS NULL construct, and converts it to individual _value_ IS NULL
clauses so they can be better optimized. It changes:
ROW(x, ROW(y)) IS NULL
to
x IS NULL AND ROW(y) IS NULL
By removing this one level of ROW construct, it causes ROW(ROW(NULL)) IS
NULL to return true, while more nested cases do not. It also tries to
short-circuit the IS NULL clause if it finds a literal NULL constant
inside the row.
My patch uses this short-circuit logic to return false for IS NULL if it
finds an embedded ROW construct. This makes the code match the code in
ExecEvalNullTest(), which already treats embedded ROW values as
non-null, e.g. it already treats ROW(ROW(x)) IS NULL as false.
Specifically, any code path that doesn't pass through
eval_const_expressions_mutator() and gets to execQual.c will return
false for this.
> We should leave things alone until we have a full fix, otherwise we'll
> just be breaking people's apps repeatedly.
Yes, we don't want that.
> I would also say that allowing eval_const_expression to drive what we
> think the "right" behavior is is completely backwards, because it's
> about the least performance-critical case. You should be looking at
> execQual.c first.
I am making eval_const_expression match execQual.c, specifically
ExecEvalNullTest(), which calls heap_attisnull(), and looks at the NULL
indicator, which can't be true for an attribute containing a ROW value.
I am confused why others don't see what I am seeing.
Another possible fix would be to avoid the IS NULL value optimizer
expansion if a ROW construct is inside a ROW(). I have attached a patch
that does this for review.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +