Re: Evaluate expression at planning time for two more cases

Hi Tom

On Tue, Sep 8, 2020 at 4:46 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Surafel Temesgen <surafel3000(at)gmail(dot)com> writes:
> > [ null_check_on_pkey_optimization_v1.patch ]
>
> I took a very brief look at this.
>
> > I don’t add NOT NULL constraint optimization to the patch because cached
> > plan is not invalidation in case of a change in NOT NULL constraint
>
> That's actually not a problem, even though some people (including me)
> have bandied about such suppositions in the past. Relying on attnotnull
> in the planner is perfectly safe [1]. Plus it'd likely be cheaper as
> well as more general than looking up pkey information. If we did need
> to explicitly record the plan's dependency on a constraint, this patch
> would be wrong anyhow because it fails to make any such notation about
> the pkey constraint it relied on.
>
>
ok thank you. I will change my next patch accordingly

> The "check_null_side" code you're proposing seems really horrid.
> For one thing, it seems quite out of place for eval_const_expressions
> to be doing that. For another, it's wrong in principle because
> eval_const_expressions doesn't know which part of the query tree
> it's being invoked on, so it cannot know whether outer-join
> nullability is an issue. For another, doing that work over again
> from scratch every time we see a potentially optimizable NullTest
> looks expensive. (I wonder whether you have tried to measure the
> performance penalty imposed by this patch in cases where it fails
> to make any proof.)
>
>
I was thinking about collecting data about joins only once at the start of
eval_const_expressions but I assume most queries don't have NULL check
expressions and postpone it until we find one. Thinking about it again I
think it can be done better by storing check_null_side_state into
eval_const_expressions_context to use it for subsequent evaluation.

I'm not sure what I think about Ashutosh's ideas about doing this
> somewhere else than eval_const_expressions. I do not buy the argument
> that it's interesting to do this separately for each child partition.
> Child partitions that have attnotnull constraints different from their
> parent's are at best a tiny minority use-case, if indeed we allow them
> at all (I tend to think we shouldn't). On the other hand it's possible
> that postponing the check would allow bypassing the outer-join problem,
> ie if we only do it for quals that have dropped down to the relation
> scan level then we don't need to worry about outer join effects.
>
>
At eval_const_expressions we check every expression and optimize it if
possible. Introducing other check and optimization mechanism to same other
place just for this optimization seems expensive with respect to
performance penalty to me

> Another angle here is that eval_const_expressions runs before
> reduce_outer_joins, meaning that if it's doing things that depend
> on outer-join-ness then it will sometimes fail to optimize cases
> that could be optimized. As a not incidental example, consider
>
> select ... from t1 left join t2 on (...) where t2.x is not null;
>
> reduce_outer_joins will realize that the left join can be reduced
> to a plain join, whereupon (if t2.x is attnotnull) the WHERE clause
> really is constant-true --- and this seems like a poster-child case
> for it being useful to optimize away the WHERE clause. But
> we won't be able to detect that if we apply the optimization during
> eval_const_expressions. So maybe that's a good reason to do it
> somewhere later.
>

In this case the expression not changed to constant-true because the
relation is on nullable side of outer join

regards
Surafel