On Sat, May 18, 2019 at 1:34 AM Andres Freund <andres@anarazel.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@2ndquadrant.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 CopyMultiInsertBufferNextFreeSlot() instead of CopyMultiInsertInfoNextFreeSlot(). What do you think, Andres, David, Alvaro ?
