On Thu, Sep 5, 2013 at 02:14:39PM -0400, Tom Lane wrote: > Robert Haas <robertmh...@gmail.com> writes: > > On Wed, Sep 4, 2013 at 9:26 PM, Bruce Momjian <br...@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 <br...@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c new file mode 100644 index 76c032c..54e59dc *** a/src/backend/optimizer/util/clauses.c --- b/src/backend/optimizer/util/clauses.c *************** eval_const_expressions_mutator(Node *nod *** 3100,3105 **** --- 3100,3122 ---- return makeBoolConst(false, false); continue; } + + /* Do we have a ROW() expression in the row? */ + if (type_is_rowtype(exprType(relem))) + { + /* + * If this ROW() has an embedded ROW() value, + * our optimization of removing a level of ROW() + * nesting causes ROW(ROW(NULL)) IS NULL to return + * true, so just return the original clause. + */ + newntest = makeNode(NullTest); + newntest->arg = (Expr *) arg; + newntest->nulltesttype = ntest->nulltesttype; + newntest->argisrow = ntest->argisrow; + return (Node *) newntest; + } + newntest = makeNode(NullTest); newntest->arg = (Expr *) relem; newntest->nulltesttype = ntest->nulltesttype;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers