Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()

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.

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.

Greetings,

Andres Freund