Обсуждение: 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

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

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

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