On Mon, Oct 28, 2019 at 1:52 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Oct 28, 2019 at 12:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Oct 27, 2019 at 12:52 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Fri, Oct 25, 2019 at 9:19 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > >
> > > I haven't yet read the new set of the patch. But, I have noticed one
> > > thing. That we are getting the size of the statistics using the AM
> > > routine. But, we are copying those statistics from local memory to
> > > the shared memory directly using the memcpy. Wouldn't it be a good
> > > idea to have an AM specific routine to get it copied from the local
> > > memory to the shared memory? I am not sure it is worth it or not but
> > > my thought behind this point is that it will give AM to have local
> > > stats in any form ( like they can store a pointer in that ) but they
> > > can serialize that while copying to shared stats. And, later when
> > > shared stats are passed back to the Am then it can deserialize in its
> > > local form and use it.
> > >
> >
> > You have a point, but after changing the gist index, we don't have any
> > current usage for indexes that need something like that. So, on one
> > side there is some value in having an API to copy the stats, but on
> > the other side without having clear usage of an API, it might not be
> > good to expose a new API for the same. I think we can expose such an
> > API in the future if there is a need for the same.
> I agree with the point. But, the current patch exposes an API for
> estimating the size for the statistics. So IMHO, either we expose
> both APIs for estimating the size of the stats and copy the stats or
> none. Am I missing something here?
>
I think the first one is a must as the things stand today because
otherwise, we won't be able to copy the stats. The second one (expose
an API to copy stats) is good to have but there is no usage of it
immediately. We can expose the second API considering the future need
but as there is no valid case as of now, it will be difficult to test
and we are also not sure whether in future any IndexAM will require
such an API.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com