Thread: Use pgstat_kind_infos to read fixed shared stats structs
Instead of needing to be explicit, we can just iterate the pgstat_kind_infos array to find the memory locations to read into. This was originally thought of by Andres in 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd. Not a fix, per se, but it does remove a comment. Perhaps the discussion will just lead to someone deleting the comment, and keeping the code as is. Either way, a win :). -- Tristan Partin Neon (https://neon.tech)
Attachment
On Mon, May 06, 2024 at 02:07:53PM -0500, Tristan Partin wrote: > This was originally thought of by Andres in > 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd. +1 because you are removing a duplication between the order of the items in PgStat_Kind and the order when these are read. I suspect that nobody would mess up with the order if adding a stats kind with a fixed number of objects, but that makes maintenance slightly easier in the long-term :) > Not a fix, per se, but it does remove a comment. Perhaps the discussion will > just lead to someone deleting the comment, and keeping the code as is. > Either way, a win :). Yup. Let's leave that as something to do for v18. -- Michael
Attachment
On Mon May 6, 2024 at 9:50 PM CDT, Michael Paquier wrote: > On Mon, May 06, 2024 at 02:07:53PM -0500, Tristan Partin wrote: > > This was originally thought of by Andres in > > 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd. > > +1 because you are removing a duplication between the order of the > items in PgStat_Kind and the order when these are read. I suspect > that nobody would mess up with the order if adding a stats kind with a > fixed number of objects, but that makes maintenance slightly easier in > the long-term :) > > > Not a fix, per se, but it does remove a comment. Perhaps the discussion will > > just lead to someone deleting the comment, and keeping the code as is. > > Either way, a win :). > > Yup. Let's leave that as something to do for v18. Thanks for the feedback Michael. Should I just throw the patch in the next commitfest so it doesn't get left behind? -- Tristan Partin Neon (https://neon.tech)
On Tue, May 07, 2024 at 12:44:51AM -0500, Tristan Partin wrote: > Thanks for the feedback Michael. Should I just throw the patch in the next > commitfest so it doesn't get left behind? Better to do so, yes. I have noted this thread in my TODO list, but we're a couple of weeks away from the next CF and things tend to get easily forgotten. -- Michael
Attachment
On Tue May 7, 2024 at 1:01 AM CDT, Michael Paquier wrote: > On Tue, May 07, 2024 at 12:44:51AM -0500, Tristan Partin wrote: > > Thanks for the feedback Michael. Should I just throw the patch in the next > > commitfest so it doesn't get left behind? > > Better to do so, yes. I have noted this thread in my TODO list, but > we're a couple of weeks away from the next CF and things tend to get > easily forgotten. Added here: https://commitfest.postgresql.org/48/4978/ -- Tristan Partin Neon (https://neon.tech)
Hi, On 2024-05-06 14:07:53 -0500, Tristan Partin wrote: > Instead of needing to be explicit, we can just iterate the > pgstat_kind_infos array to find the memory locations to read into. > This was originally thought of by Andres in > 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd. > > Not a fix, per se, but it does remove a comment. Perhaps the discussion will > just lead to someone deleting the comment, and keeping the code as is. > Either way, a win :). I think it's a good idea. I'd really like to allow extensions to register new types of stats eventually. Stuff like pg_stat_statements having its own, fairly ... mediocre, stats storage shouldn't be necessary. Do we need to increase the stats version, I didn't check if the order we currently store things in and the numerical order of the stats IDs are the same. Greetings, Andres Freund
On Tue May 7, 2024 at 1:29 PM CDT, Andres Freund wrote: > Hi, > > On 2024-05-06 14:07:53 -0500, Tristan Partin wrote: > > Instead of needing to be explicit, we can just iterate the > > pgstat_kind_infos array to find the memory locations to read into. > > > This was originally thought of by Andres in > > 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd. > > > > Not a fix, per se, but it does remove a comment. Perhaps the discussion will > > just lead to someone deleting the comment, and keeping the code as is. > > Either way, a win :). > > I think it's a good idea. I'd really like to allow extensions to register new > types of stats eventually. Stuff like pg_stat_statements having its own, > fairly ... mediocre, stats storage shouldn't be necessary. Could be useful for Neon in the future too. > Do we need to increase the stats version, I didn't check if the order we > currently store things in and the numerical order of the stats IDs are the > same. I checked the orders, and they looked the same. 1. Archiver 2. BgWriter 3. Checkpointer 4. IO 5. SLRU 6. WAL -- Tristan Partin Neon (https://neon.tech)
On Tue, May 07, 2024 at 02:07:42PM -0500, Tristan Partin wrote: > On Tue May 7, 2024 at 1:29 PM CDT, Andres Freund wrote: >> I think it's a good idea. I'd really like to allow extensions to register new >> types of stats eventually. Stuff like pg_stat_statements having its own, >> fairly ... mediocre, stats storage shouldn't be necessary. > > Could be useful for Neon in the future too. Interesting thing to consider. If you do that, I'm wondering if you could, actually, lift the restriction on pg_stat_statements.max and make it a SIGHUP so as it could be increased on-the-fly.. Hmm, just a thought in passing. >> Do we need to increase the stats version, I didn't check if the order we >> currently store things in and the numerical order of the stats IDs are the >> same. > > I checked the orders, and they looked the same. > > 1. Archiver > 2. BgWriter > 3. Checkpointer > 4. IO > 5. SLRU > 6. WAL Yup, I've looked at that yesterday and the read order remains the same so I don't see a need for a version bump. -- Michael