Thread: Remove unused 'len' from pg_stat_recv_* functions
Hi all, While working on a patch adding new stats, houzj pointed out that 'len' function arguments of all pgstat_recv_* functions are not used at all. Looking at git history, pgstat_recv_* functions have been having ‘len’ since the stats collector was introduced by commit 140ddb78fe 20 years ago but it was not used at all even in the first commit. It seems like the improvements so far for the stats collector had pgstat_recv_* function have ‘len’ for consistency with the existing pgstat_recv_* functions. Is there any historical reason for having 'len' argument? Or can we remove them? I've attached the patch that removes 'len' from all pgstat_recv_* functions. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
At Tue, 3 Aug 2021 12:40:23 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > Hi all, > > While working on a patch adding new stats, houzj pointed out that > 'len' function arguments of all pgstat_recv_* functions are not used > at all. Looking at git history, pgstat_recv_* functions have been > having ‘len’ since the stats collector was introduced by commit > 140ddb78fe 20 years ago but it was not used at all even in the first > commit. It seems like the improvements so far for the stats collector > had pgstat_recv_* function have ‘len’ for consistency with the > existing pgstat_recv_* functions. Is there any historical reason for > having 'len' argument? Or can we remove them? > > I've attached the patch that removes 'len' from all pgstat_recv_* functions. I at the first look thought that giving "len" as a parameter is reasonable as message-processing functions, but the given message struct contains the same value and the functions can refer to the message length without the parameter if they want. So I'm +-0 for the removal. It applies cleanly on the master and compiled without an error. That being said, I'm not sure it is worthwhile to change parameters of going-to-be-removed functions (if shared-memory stats collector is successfully introduced). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Aug 3, 2021 at 3:17 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Tue, 3 Aug 2021 12:40:23 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > > Hi all, > > > > While working on a patch adding new stats, houzj pointed out that > > 'len' function arguments of all pgstat_recv_* functions are not used > > at all. Looking at git history, pgstat_recv_* functions have been > > having ‘len’ since the stats collector was introduced by commit > > 140ddb78fe 20 years ago but it was not used at all even in the first > > commit. It seems like the improvements so far for the stats collector > > had pgstat_recv_* function have ‘len’ for consistency with the > > existing pgstat_recv_* functions. Is there any historical reason for > > having 'len' argument? Or can we remove them? > > > > I've attached the patch that removes 'len' from all pgstat_recv_* functions. > > I at the first look thought that giving "len" as a parameter is > reasonable as message-processing functions, but the given message > struct contains the same value and the functions can refer to the > message length without the parameter if they want. So I'm +-0 for the > removal. > > It applies cleanly on the master and compiled without an error. > > That being said, I'm not sure it is worthwhile to change parameters of > going-to-be-removed functions (if shared-memory stats collector is > successfully introduced). Good point. I'm not going to insist on removing them but I got confused a bit whether or not I should add 'len' when writing a new pgstat_recv_* function. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
On 2021-08-03 16:56:12 +0900, Masahiko Sawada wrote: > On Tue, Aug 3, 2021 at 3:17 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > > At Tue, 3 Aug 2021 12:40:23 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > > > Hi all, > > > > > > While working on a patch adding new stats, houzj pointed out that > > > 'len' function arguments of all pgstat_recv_* functions are not used > > > at all. Looking at git history, pgstat_recv_* functions have been > > > having ‘len’ since the stats collector was introduced by commit > > > 140ddb78fe 20 years ago but it was not used at all even in the first > > > commit. It seems like the improvements so far for the stats collector > > > had pgstat_recv_* function have ‘len’ for consistency with the > > > existing pgstat_recv_* functions. Is there any historical reason for > > > having 'len' argument? Or can we remove them? > > > > > > I've attached the patch that removes 'len' from all pgstat_recv_* functions. > > > > I at the first look thought that giving "len" as a parameter is > > reasonable as message-processing functions, but the given message > > struct contains the same value and the functions can refer to the > > message length without the parameter if they want. So I'm +-0 for the > > removal. > > > > It applies cleanly on the master and compiled without an error. > > > > That being said, I'm not sure it is worthwhile to change parameters of > > going-to-be-removed functions (if shared-memory stats collector is > > successfully introduced). > > Good point. Indeed. It's already kinda painful to maintain the shared memory stats patch, no need to make it harder... > I'm not going to insist on removing them but I got confused a bit > whether or not I should add 'len' when writing a new pgstat_recv_* > function. I'd keep it in sync with what's there right now. Greetings, Andres Freund