Обсуждение: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
Hi Andres, Hari, David,
In the latest PostgreSQL code, I could see that we are passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot() although it is not being used anywhere in that function. Could you please let me know if it has been done intentionally or it is just an overlook that needs to be corrected. AFAIU, CopyMultiInsertInfoNextFreeSlot() is just intended to return the next free slot available in the multi insert buffer and we already have that buffer stored in ResultRelInfo structure which is also being passed to that function so not sure what could be the purpose of passing CopyMultiInsertInfo structure as well. Please let me know if i am missing something here. Thank you.
In the latest PostgreSQL code, I could see that we are passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot() although it is not being used anywhere in that function. Could you please let me know if it has been done intentionally or it is just an overlook that needs to be corrected. AFAIU, CopyMultiInsertInfoNextFreeSlot() is just intended to return the next free slot available in the multi insert buffer and we already have that buffer stored in ResultRelInfo structure which is also being passed to that function so not sure what could be the purpose of passing CopyMultiInsertInfo structure as well. Please let me know if i am missing something here. Thank you.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
David Rowley
Дата:
On Mon, 13 May 2019 at 23:20, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > In the latest PostgreSQL code, I could see that we are passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()although it is not being used anywhere in that function. Could you please let me know ifit has been done intentionally or it is just an overlook that needs to be corrected. AFAIU, CopyMultiInsertInfoNextFreeSlot()is just intended to return the next free slot available in the multi insert buffer and wealready have that buffer stored in ResultRelInfo structure which is also being passed to that function so not sure whatcould be the purpose of passing CopyMultiInsertInfo structure as well. Please let me know if i am missing something here.Thank you. There's likely no good reason for that. The attached removes it. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
Ashutosh Sharma
Дата:
On Mon, 13 May 2019 at 23:20, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> In the latest PostgreSQL code, I could see that we are passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot() although it is not being used anywhere in that function. Could you please let me know if it has been done intentionally or it is just an overlook that needs to be corrected. AFAIU, CopyMultiInsertInfoNextFreeSlot() is just intended to return the next free slot available in the multi insert buffer and we already have that buffer stored in ResultRelInfo structure which is also being passed to that function so not sure what could be the purpose of passing CopyMultiInsertInfo structure as well. Please let me know if i am missing something here. Thank you.
There's likely no good reason for that. The attached removes it.
Thanks for the confirmation David. The patch looks good to me.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: Passing CopyMultiInsertInfo structure toCopyMultiInsertInfoNextFreeSlot()
От
Michael Paquier
Дата:
On Mon, May 13, 2019 at 08:17:49PM +0530, Ashutosh Sharma wrote: > Thanks for the confirmation David. The patch looks good to me. It looks to me that it can be a matter a consistency with the other APIs dealing with multi-inserts in COPY. For now I have added an open item on that. -- Michael
Вложения
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
David Rowley
Дата:
On Tue, 14 May 2019 at 13:00, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, May 13, 2019 at 08:17:49PM +0530, Ashutosh Sharma wrote: > > Thanks for the confirmation David. The patch looks good to me. > > It looks to me that it can be a matter a consistency with the other > APIs dealing with multi-inserts in COPY. For now I have added an open > item on that. 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. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Passing CopyMultiInsertInfo structure toCopyMultiInsertInfoNextFreeSlot()
От
Michael Paquier
Дата:
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. -- Michael
Вложения
Re: Passing CopyMultiInsertInfo structure toCopyMultiInsertInfoNextFreeSlot()
От
Alvaro Herrera
Дата:
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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
Ashutosh Sharma
Дата:
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 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. As an example, please refer to the following code in printTableAddHeader() or printTableAddCell().
#ifndef ENABLE_NLS
(void) translate; /* unused parameter */
#endif
The function argument *translate* has been marked as unsed but only for non-nls build which means it will be used if it is nls enabled build. But, I do not see any such requirement in our case. Please let me know if I missing anything here.
Thanks,
Re: Passing CopyMultiInsertInfo structure toCopyMultiInsertInfoNextFreeSlot()
От
Andres Freund
Дата:
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. 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
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
Ashutosh Sharma
Дата:
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 ?
Thanks,
Re: Passing CopyMultiInsertInfo structure toCopyMultiInsertInfoNextFreeSlot()
От
Andres Freund
Дата:
Hi, On 2019-05-18 06:14:15 +0530, Ashutosh Sharma wrote: > On Sat, May 18, 2019 at 1:34 AM Andres Freund <andres@anarazel.de> wrote: > 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. I don't think this is right. Obviously it'd not be without a bit more changes, but we definitely *should* try to reuse slots from other partitions (including the root partition if compatible). Creating them isn't that cheap, compared to putting slots onto a freelist for a wihle. > 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 ? Unless somebody else presses back hard against doing so *soon*, I'm going to close this open issue. I don't think it's worth spending further time arguing about a few characters. Greetings, Andres Freund
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
David Rowley
Дата:
On Sat, 18 May 2019 at 12:49, Andres Freund <andres@anarazel.de> wrote: > > On 2019-05-18 06:14:15 +0530, Ashutosh Sharma wrote: > > 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 ? > > Unless somebody else presses back hard against doing so *soon*, I'm > going to close this open issue. I don't think it's worth spending > further time arguing about a few characters. I'd say if we're not going to bother removing the unused param that there's not much point in renaming the function. The proposed name might make sense if the function was: static inline TupleTableSlot * CopyMultiInsertBufferNextFreeSlot(CopyMultiInsertBuffer *buffer, Relation rel) then that might be worth a commit, but giving it that name without changing the signature to that does not seem like an improvement to me. I'm personally about +0.1 for making the above change, which is well below my threshold for shouting and screaming. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes: > On Sat, 18 May 2019 at 12:49, Andres Freund <andres@anarazel.de> wrote: >> Unless somebody else presses back hard against doing so *soon*, I'm >> going to close this open issue. I don't think it's worth spending >> further time arguing about a few characters. > I'd say if we're not going to bother removing the unused param that > there's not much point in renaming the function. FWIW, I'm on the side of "we shouldn't change this". There's lots of unused parameters in PG functions, and in most of those cases the API is reasonable as it stands. regards, tom lane
Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()
От
Ashutosh Sharma
Дата:
On Sat, May 18, 2019 at 6:44 AM David Rowley <david.rowley@2ndquadrant.com> wrote:
On Sat, 18 May 2019 at 12:49, Andres Freund <andres@anarazel.de> wrote:
>
> On 2019-05-18 06:14:15 +0530, Ashutosh Sharma wrote:
> > 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 ?
>
> Unless somebody else presses back hard against doing so *soon*, I'm
> going to close this open issue. I don't think it's worth spending
> further time arguing about a few characters.
I'd say if we're not going to bother removing the unused param that
there's not much point in renaming the function. The proposed name
might make sense if the function was:
static inline TupleTableSlot *
CopyMultiInsertBufferNextFreeSlot(CopyMultiInsertBuffer *buffer, Relation rel)
Well, that's what I suggested but seems like Andres is not in favour of it because he feels that in the future we *should* add a facility to reuse the slots across the partitions because reusing a free slot is quite cheaper than creating a new one and in that sense we would in future need to pass miinfo to *NextFreeSlot function
then that might be worth a commit, but giving it that name without
changing the signature to that does not seem like an improvement to
me.
That's right, we cannot have this name without changing it's signature.
I'm personally about +0.1 for making the above change, which is well
below my threshold for shouting and screaming.
I think, as Andres pointed out in his earlier reply, we should probably stop this discussion here, if in future we add the support to reuse the slots across the partition then probably we will have to undo the changes that we will be doing here. Anyways, thanks to Andres and David for clearing my doubts.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services