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
Attachment
On Wed, May 08, 2024 at 10:21:56AM +0900, Michael Paquier wrote: > 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. While looking at this stuff again, I got no objections for it except a few edits to make the new code more consistent with the surroundings, as in the loop, the use of pgstat_get_kind_info and an extra assertion. Still, I think that we could be more ambitious to make this area of the code more pluggable in the future (fixed shared stats are not covered by my proposal on the other thread about pluggable cumulative stats, perhaps it should but I could not think about a case for them). Anyway, the first thing is to apply the same pattern as the read part for pgstat_write_statsfile(), based on the content in the local stats snapshot PgStat_Snapshot. So, how about trying to remove the dependency to the fixed shared stats structures in PgStat_ShmemControl and PgStat_Snapshot? I'd like to think that these should be replaced with an area allocated in shared memory and TopMemoryContext respectively, with PgStat_Snapshot and PgStat_ShmemControl pointing to these areas, with an allocated size based on the information aggregated from the KindInfo Array. We could also store the offset of the fixed areas in two extra arrays, one for each of the two structures, indexed by KindInfo and of size PGSTAT_NUM_KINDS. Thoughts? -- Michael
Attachment
On Mon, Jul 01, 2024 at 02:48:19PM +0900, Michael Paquier wrote: > So, how about trying to remove the dependency to the fixed shared > stats structures in PgStat_ShmemControl and PgStat_Snapshot? I'd like > to think that these should be replaced with an area allocated in > shared memory and TopMemoryContext respectively, with PgStat_Snapshot > and PgStat_ShmemControl pointing to these areas, with an allocated > size based on the information aggregated from the KindInfo Array. We > could also store the offset of the fixed areas in two extra arrays, > one for each of the two structures, indexed by KindInfo and of size > PGSTAT_NUM_KINDS. I have been poking at this area, and found a solution that should work. The details will be posted on the pluggable stats thread with a rebased patch and these bits on top of the pluggable APIs: https://www.postgresql.org/message-id/Zmqm9j5EO0I4W8dx%40paquier.xyz So let's move the talking there. -- Michael