Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()

On Sat, May 18, 2019 at 1:34 AM Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2019-05-17 11:09:41 +0530, Ashutosh Sharma wrote:
> > On Fri, May 17, 2019 at 3:10 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com
> >
> > wrote:
> >
> > > On 2019-May-14, Michael Paquier wrote:
> > >
> > > > On Tue, May 14, 2019 at 01:19:30PM +1200, David Rowley wrote:
> > > > > When I wrote the code I admit that I was probably wearing my
> > > > > object-orientated programming hat. I had in mind that the whole
> > > > > function series would have made a good class. Passing the
> > > > > CopyMultiInsertInfo was sort of the non-OOP equivalent to having
> > > > > this/Me/self available, as it would be for any instance method of a
> > > > > class. Back to reality, this isn't OOP, so I was wearing the wrong
> > > > > hat. I think the unused parameter should likely be removed. It's
> > > > > probably not doing a great deal of harm since the function is
> static
> > > > > inline and the compiler should be producing any code for the unused
> > > > > param, but for the sake of preventing confusion, it should be
> removed.
> > > > > Ashutosh had to ask about it, so it wasn't immediately clear what
> the
> > > > > purpose of it was. Since there's none, be gone with it, I say.
> > > >
> > > > Sounds fair to me. This has been introduced by 86b8504, so let's see
> > > > what's Andres take.
> > >
> > > If this were up to me, I'd leave the function signature alone, and
> just add
> > > (void) miinfo; /* unused parameter */
> > > to the function code. It seems perfectly reasonable to have that
> > > function argument, and a little weird not to have it.
>
> I'd do that, or simply nothing. We've plenty of unused args, so I don't
> see much point in these kind of "unused parameter" warning suppressions
> in isolated places.
>
>
> > I think, we should only do that if at all there is any circumstances
> under
> > which 'miinfo' might be used otherwise it would good to remove it unless
> > there is some specific reason for having it.
>
> It seems like it could entirely be reasonable to e.g. reuse slots across
> partitions, which'd require CopyMultiInsertInfo to be around.
>
>
Considering that we can have MAX_BUFFERED_TUPLES slots in each multi-insert
buffer and we do flush the buffer after MAX_BUFFERED_TUPLES tuples have
been stored, it seems unlikely that we would ever come across a situation
where one partition would need to reuse the slot of another partition.

Also, from a consistency point, it seems the caller doesn't need to know
> whether all the necessary information is in the ResultRelInfo and not in
> CopyMultiInsertInfo for *some* of the CopyMultiInsertInfo functions.
>
>
I actually feel that the function name itself is not correct here, it
appears to be confusing and inconsistent considering the kind of work that
it is doing. I think, the function name should have been CopyMultiInsert
*Buffer*NextFreeSlot() instead of CopyMultiInsert*Info*NextFreeSlot(). What
do you think, Andres, David, Alvaro ?

Thanks,

--
With Regards,
Ashutosh Sharma
EnterpriseDB:*http://www.enterprisedb.com <http://www.enterprisedb.com/>*