On Sat, May 18, 2019 at 1:34 AM Andres Freund <and...@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 <alvhe...@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 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/>*
