Thread: Remove io prefix from pg_stat_io columns

On Wed, Apr 19, 2023 at 01:54:21PM -0300, Fabrízio de Royes Mello wrote:
> On Wed, Apr 19, 2023 at 1:27 PM Melanie Plageman <melanieplageman@gmail.com> wrote:
>> Over in [1], we discussed removing the "io" prefix from the columns
>> "io_context" and "io_object" in pg_stat_io since they seem redundant
>> given the view name
>
> LGTM. All tests passed and were built without warnings.

There are a lot of internal references to both of them mainly around
the buffer manager and the pgstat code, still I agree that the view
feels redundant as currently written, so agreed.  It does not seem
like you have missed any references here, from what I can see.
--
Michael

On Wed, Apr 19, 2023 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 19, 2023 at 01:54:21PM -0300, Fabrízio de Royes Mello wrote:
> > On Wed, Apr 19, 2023 at 1:27 PM Melanie Plageman <melanieplageman@gmail.com> wrote:
> >> Over in [1], we discussed removing the "io" prefix from the columns
> >> "io_context" and "io_object" in pg_stat_io since they seem redundant
> >> given the view name
> >
> > LGTM. All tests passed and were built without warnings.
>
> There are a lot of internal references to both of them mainly around
> the buffer manager and the pgstat code, still I agree that the view
> feels redundant as currently written, so agreed.  It does not seem
> like you have missed any references here, from what I can see.

I thought about changing parameter and local variable names to remove
the prefix, but in the original discussion folks seemed to think it made
sense to leave the "C level" references with an "io" prefix. I think we
could change many of them, but some of them may be required for clarity.

- Melanie

On Wed, Apr 19, 2023 at 08:50:13PM -0400, Melanie Plageman wrote:
> I thought about changing parameter and local variable names to remove
> the prefix, but in the original discussion folks seemed to think it made
> sense to leave the "C level" references with an "io" prefix. I think we
> could change many of them, but some of them may be required for clarity.

I agree with the feeling of not touching the internal variables.  It
makes them easier to grep, and it seems that these are mostly on lines
where there is little context about what they refer to..

Perhaps others have comments or objections, so let's wait a bit, but
I'd be OK to apply this one myself, with a catversion bump.  (Happy to
help.)
--
Michael

On Thu, Apr 20, 2023 at 10:13:04AM +0900, Michael Paquier wrote:
> On Wed, Apr 19, 2023 at 08:50:13PM -0400, Melanie Plageman wrote:
> > I thought about changing parameter and local variable names to remove
> > the prefix, but in the original discussion folks seemed to think it made
> > sense to leave the "C level" references with an "io" prefix. I think we
> > could change many of them, but some of them may be required for clarity.
> 
> I agree with the feeling of not touching the internal variables.  It
> makes them easier to grep, and it seems that these are mostly on lines
> where there is little context about what they refer to..
> 
> Perhaps others have comments or objections, so let's wait a bit, but
> I'd be OK to apply this one myself, with a catversion bump.  (Happy to
> help.)

Great, thanks! Once you feel an appropriate amount of time has passed,
it would be great if you could apply it. I forgot to add a note about
the catalog version bump. oops!

- Melanie

On Wed, Apr 19, 2023 at 09:45:32PM -0400, Melanie Plageman wrote:
> Great, thanks! Once you feel an appropriate amount of time has passed,
> it would be great if you could apply it.

Sure.  Probably on tomorrow morning, or Monday in the worst-case
scenario, I think..

> I forgot to add a note about the catalog version bump. oops!

No worries, committers should take care of that.
--
Michael

At Thu, 20 Apr 2023 10:13:04 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Apr 19, 2023 at 08:50:13PM -0400, Melanie Plageman wrote:
> > I thought about changing parameter and local variable names to remove
> > the prefix, but in the original discussion folks seemed to think it made
> > sense to leave the "C level" references with an "io" prefix. I think we
> > could change many of them, but some of them may be required for clarity.
> 
> I agree with the feeling of not touching the internal variables.  It
> makes them easier to grep, and it seems that these are mostly on lines
> where there is little context about what they refer to..

I find the names for local loop variables are a bit annoying, but I
don't feel strongly about removing the prifix there. I'm also not in
favor of removing the prefix in other cases, bacause it helps with
grep'ability.

>    if (backend_io->times[io_object][io_context][io_op] != 0 &&
>        backend_io->counts[io_object][io_context][io_op] <= 0)

> Perhaps others have comments or objections, so let's wait a bit, but
> I'd be OK to apply this one myself, with a catversion bump.  (Happy to
> help.)

So, I don't have any issues with the patch overall. From what I can
tell, there are no remaining instances of io_foobar that need to be
rewritten.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Apr 20, 2023 at 11:38:42AM +0900, Michael Paquier wrote:
> No worries, committers should take care of that.

Done as of 0ecb87e, as I can keep an eye on the buildfarm today, with
a catversion bump.
--
Michael

On 2023-04-21 07:38:01 +0900, Michael Paquier wrote:
> On Thu, Apr 20, 2023 at 11:38:42AM +0900, Michael Paquier wrote:
> > No worries, committers should take care of that.
> 
> Done as of 0ecb87e, as I can keep an eye on the buildfarm today, with
> a catversion bump.

Thanks!