Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)

On Tue, Feb 16, 2021 at 12:01 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> On Fri, 12 Feb 2021 at 15:18, Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 12, 2021 at 9:02 AM David Rowley <dgrowleyml(at)gmail(dot)com>
> wrote:
> >> The reason I don't really like this is that it really depends where
> >> you want to use RelOptInfo.notnullattrs. If someone wants to use it
> >> to optimise something before the base quals are evaluated then they
> >> might be unhappy that they found some NULLs.
> >>
> >
> > Do you mean the notnullattrs is not set correctly before the base quals
> are
> > evaluated? I think we have lots of data structures which are set just
> after some
> > stage. but notnullattrs is special because it is set at more than 1
> stage. However
> > I'm doubtful it is unacceptable, Some fields ever change their meaning
> at different
> > stages like Var->varno. If a user has a misunderstanding on it, it
> probably will find it
> > at the testing stage.
>
> You're maybe focusing too much on your use case for notnullattrs. It
> only cares about NULLs in the result for each query level.
>
> .... thinks of an example...
>
> OK, let's say I decided that COUNT(*) is faster than COUNT(id) so
> decided that I might like to write a patch which rewrite the query to
> use COUNT(*) when it was certain that "id" could not contain NULLs.
>
> The query is:
>
> SELECT p.partid, p.partdesc,COUNT(s.saleid) FROM part p LEFT OUTER
> JOIN sales s ON p.partid = s.partid GROUP BY p.partid;
>
> sale.saleid is marked as NOT NULL in pg_attribute. As the writer of
> the patch, I checked the comment for notnullattrs and it says "Not
> null attrs, start from -FirstLowInvalidHeapAttributeNumber", so I
> should be ok to assume since sales.saleid is marked in notnullattrs
> that I can rewrite the query?!
>
> The documentation about the RelOptInfo.notnullattrs needs to be clear
> what exactly it means. I'm not saying your representation of how to
> record NOT NULL in incorrect. I'm saying that you need to be clear
> what exactly is being recorded in that field.
>
> If you want it to mean "attribute marked here cannot output NULL
> values at this query level", then you should say something along those
> lines.
>

I think I get what you mean. You are saying notnullattrs is only correct
at the *current* stage, namely set_rel_size. It would not be true after
that, but the data is still there. That would cause some confusion. I
admit
that is something I didn't realize before. I checked other fields of
RelOptInfo,
looks no one filed works like this, so I am not really happy with this
design
now. I'm OK with saying more things along these lines. That can be done
we all understand each other well. Any better design is welcome as well.
I think the "Var represents null stuff" is good, until I see your comments
below.

> However, having said that, because this is a Bitmapset of
> pg_attribute.attnums, it's only possible to record Vars from base
> relations. It does not seem like you have any means to record
> attributes that are normally NULLable, but cannot produce NULL values
> due to a strict join qual.
>
> e.g: SELECT t.nullable FROM t INNER JOIN j ON t.nullable = j.something;
>
> I'd expect the RelOptInfo for t not to contain a bit for the
> "nullable" column, but there's no way to record the fact that the join
> RelOptInfo for {t,j} cannot produce a NULL for that column. It might
> be quite useful to know that for the UniqueKeys patch.
>
>
The current patch can detect t.nullable is not null correctly. That
is done by find_nonnullable_vars(qual) and deconstruct_recure stage.

> I know there's another discussion here between Ashutosh and Tom about
> PathTarget's and Vars. I had the Var idea too once myself [1] but it
> was quickly shot down. Tom's reasoning there in [1] seems legit. I
> guess we'd need some sort of planner version of Var and never confuse
> it with the Parse version of Var. That sounds like quite a big
> project which would have quite a lot of code churn. I'm not sure how
> acceptable it would be to have Var represent both these things. It
> gets complex when you do equal(var1, var2) and expect that to return
> true when everything matches apart from the notnull field. We
> currently have this issue with the "location" field and we even have a
> special macro which just ignores those in equalfuncs.c. I imagine not
> many people would like to expand that to other fields.
>
>
Thanks for sharing this.

> It would be good to agree on the correct representation for Vars that
> cannot produce NULLs so that we don't shut the door on classes of
> optimisation that require something other than what you need for your
> case.
>
>
Agreed. The simplest way is just adding some comments. If go a
step further, how about just reset the notnullattrs when it is nullable
later like outer join? I have added this logic in the attached patch.
(comment for the notnullattrs is still not touched). I think we only
need to handle this in build_join_rel stage. With the v2 commit 2,
notnullattrs might be unset too early, but if the value is there, then
it is correct.

--
Best Regards
Andy Fan (https://www.aliyun.com/)