Hi Tom,
On Mon, Sep 23, 2024 at 2:10 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Mon, Sep 16, 2024 at 3:44 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> > On 9/9/2024 12:36, Alexander Korotkov wrote:
> > > Also, I agree it get it's wrong to directly copy RestrictInfo struct
> > > in group_similar_or_args(). Instead, I've renamed
> > > make_restrictinfo_internal() to make_plain_restrictinfo(), which is
> > > intended to handle non-recursive cases when you've children already
> > > wrapped with RestrictInfos. make_plain_restrictinfo() now used in
> > > group_similar_or_args().
> > Great work. Thanks for doing this!
> >
> > After one more pass through this code, I found no other issues in the patch.
> > Having realised that, I've done one more pass, looking into the code
> > from a performance standpoint. It looks mostly ok, but In my opinion, in
> > the cycle:
> >
> > foreach(lc, orclause->args)
> > {
> > }
> >
> > we should free the consts list before returning NULL on unsuccessful
> > attempt. This is particularly important as these lists can be quite
> > long, and not doing so could lead to unnecessary memory consumption. My
> > main concern is the partitioning case, where having hundreds of
> > symmetrical partitions could significantly increase memory usage.
>
> Makes sense. Please, check the attached patch freeing the consts list
> while returning NULL from match_orclause_to_indexcol().
I think this patchset got much better, and it could possible be
committed after another round of cleanup and comment/docs improvement.
It would be very kind if you share your view on the decisions made in
this patchset.
------
Regards,
Alexander Korotkov
Supabase
