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

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

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

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