Обсуждение: Cleanup - Removed unused function parameter in reorder buffer & parallel vacuum
Hi, While checking through the code I found that some of the function parameters in reorderbuffer & vacuumlazy are not used. I felt this could be removed. I'm not sure if it is kept for future use or not. Attached patch contains the changes for the same. Thoughts? Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
Вложения
Re: Cleanup - Removed unused function parameter in reorder buffer & parallel vacuum
От
Simon Riggs
Дата:
On Fri, 3 Jul 2020 at 09:07, vignesh C <vignesh21@gmail.com> wrote:
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?
Unused? To confirm that, everybody that has a logical decoding plugin needs to check their code so we are certain this is sensible.
Seems like a change with low utility.
Re: Cleanup - Removed unused function parameter in reorder buffer & parallel vacuum
От
Masahiko Sawada
Дата:
On Fri, 3 Jul 2020 at 17:07, vignesh C <vignesh21@gmail.com> wrote: > > Hi, > > While checking through the code I found that some of the function > parameters in reorderbuffer & vacuumlazy are not used. I felt this > could be removed. I'm not sure if it is kept for future use or not. > Attached patch contains the changes for the same. > Thoughts? > For the part of parallel vacuum change, it looks good to me. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Cleanup - Removed unused function parameter in reorder buffer & parallel vacuum
От
Amit Kapila
Дата:
On Fri, Jul 3, 2020 at 2:06 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Fri, 3 Jul 2020 at 09:07, vignesh C <vignesh21@gmail.com> wrote: > >> >> While checking through the code I found that some of the function >> parameters in reorderbuffer & vacuumlazy are not used. I felt this >> could be removed. I'm not sure if it is kept for future use or not. >> Attached patch contains the changes for the same. >> Thoughts? > > > Unused? To confirm that, everybody that has a logical decoding plugin needs to check their code so we are certain thisis sensible. > The changes proposed by Vignesh are in ReorderBuffer APIs and some of them are static functions, so not sure if decoding plugin comes into the picture. > Seems like a change with low utility. > Yeah, all or most of the ReorderBuffer APIs seem to take the "ReorderBuffer *" parameter, so not sure if removing from some of them is useful or not. At least in the current form, they look consistent. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes: > On Fri, Jul 3, 2020 at 2:06 PM Simon Riggs <simon@2ndquadrant.com> wrote: >> Seems like a change with low utility. > Yeah, all or most of the ReorderBuffer APIs seem to take the > "ReorderBuffer *" parameter, so not sure if removing from some of them > is useful or not. At least in the current form, they look consistent. Yeah, I agree with that. This makes things less consistent and it seems like it's not buying much. Are any of these code paths sufficiently hot that saving a couple of instructions would matter? In the long run, it seems like the fact that any of these functions are not using these parameters is an implementation artifact that could change at any time. So we might just be putting them back someday, with nothing except code churn and back-patch hazards to show for our trouble. Or, if you want to argue that a "ReorderBufferXXX" function is inherently never going to use the ReorderBuffer, why is it in that module with that name to begin with? regards, tom lane
Re: Cleanup - Removed unused function parameter in reorder buffer & parallel vacuum
От
Amit Kapila
Дата:
On Fri, Jul 3, 2020 at 5:18 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > On Fri, 3 Jul 2020 at 17:07, vignesh C <vignesh21@gmail.com> wrote: > > > > Hi, > > > > While checking through the code I found that some of the function > > parameters in reorderbuffer & vacuumlazy are not used. I felt this > > could be removed. I'm not sure if it is kept for future use or not. > > Attached patch contains the changes for the same. > > Thoughts? > > > > For the part of parallel vacuum change, it looks good to me. > Unlike ReorderBuffer, this change looks fine to me as well. This is a quite recent (PG13) change and it would be good to remove it now. So, I will push this part of change unless I hear any objection in a day or so. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sat, Jul 4, 2020 at 12:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Jul 3, 2020 at 5:18 PM Masahiko Sawada > <masahiko.sawada@2ndquadrant.com> wrote: > > > > On Fri, 3 Jul 2020 at 17:07, vignesh C <vignesh21@gmail.com> wrote: > > > > > > Hi, > > > > > > While checking through the code I found that some of the function > > > parameters in reorderbuffer & vacuumlazy are not used. I felt this > > > could be removed. I'm not sure if it is kept for future use or not. > > > Attached patch contains the changes for the same. > > > Thoughts? > > > > > > > For the part of parallel vacuum change, it looks good to me. > > > > Unlike ReorderBuffer, this change looks fine to me as well. This is a > quite recent (PG13) change and it would be good to remove it now. So, > I will push this part of change unless I hear any objection in a day > or so. Thanks all for your comments, attached patch has the changes that excludes the changes made in reorderbuffer. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com