On Sat, Mar 2, 2019 at 5:29 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > James Coleman <jtc...@gmail.com> writes: > > On Fri, Mar 1, 2019 at 5:28 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > >> I also tweaked it to recognize the case where we can prove the > >> array, rather than the scalar, to be null. I'm not sure how useful > >> that is in practice, but it seemed weird that we'd exploit that > >> only if we can also prove the scalar to be null. > > > Just for my own understanding: I thought the "if > > (arrayconst->constisnull)" took care of the array constant being null? > > Yeah, but only after we've already matched the subexpr to the LHS, > otherwise we'd not be bothering to determine the array's size. > > On the other hand, if for some reason we know that the array side > is NULL, we can conclude that the ScalarArrayOp returns NULL > independently of what the LHS is.
Thanks for the detailed explanation. > > I don't see a check on the scalar node / lhs. I do see you added a > > check for the entire clause being null, but I'm not sure I understand > > when that would occur (unless it's only in the recursive case?) > > Yeah, it's to handle the case where we run into a constant NULL > below a strict node. Typically, that doesn't happen because > eval_const_expressions would have const-folded the upper node, but > it's not impossible given all the cases that clause_is_strict_for > can recurse through now. (The coverage bot does show that code being > reached, although perhaps that only occurs for null-below-ScalarArrayOp, > in which case it might be dead if we were to make eval_const_expressions > smarter.) Ah, that makes sense. James Coleman