On Fri, May 22, 2020 at 4:52 AM David Rowley <dgrowle...@gmail.com> wrote:
> On Thu, 14 May 2020 at 14:39, Andy Fan <zhihui.fan1...@gmail.com> wrote: > > > > On Thu, May 14, 2020 at 6:20 AM David Rowley <dgrowle...@gmail.com> > wrote: > >> Having the "onerow" flag was not how I intended it to work. > >> > > Thanks for the detailed explanation. So I think we do need to handle > onerow > > specially, (It means more things than adding each column as a UniqueKey). > > but we don't need the onerow flag since we can tell it by ukey->exprs == > NIL. > > > > During the developer of this feature, I added some Asserts as double > checking > > for onerow and exprs. it helps me to find some special cases. like > > SELECT FROM multirows union SELECT FROM multirows; where targetlist is > NIL. > > (I find the above case returns onerow as well just now). so onerow flag > allows us > > check this special things with more attention. Even this is not the > original intention > > but looks it is the one purpose now. > > But surely that special case should just go in > populate_unionrel_uniquekeys(). If the targetlist is empty, then add a > UniqueKey with an empty set of exprs. > > This is correct on this special case. > However I am feeling that removing onerow flag doesn't require much of > code > > changes. Almost all the special cases which are needed before are still > needed > > after that and all the functions based on that like relation_is_onerow > > /add_uniquekey_onerow is still valid, we just need change the > implementation. > > so do you think we need to remove onerow flag totally? > > Well, at the moment I'm not quite understanding why it's needed. If > it's not needed then we should remove it. If it turns out there is > some valid reason for it, then we should keep it. > Currently I uses it to detect more special case which we can't image at first, we can understand it as it used to debug/Assert purpose only. After the mainly code is reviewed, that can be removed (based on the change is tiny). -- Best Regards Andy Fan
