Add a write_to_file member to PgStat_KindInfo

Hi,

Thank you for working on this!

On Wed, 20 Nov 2024 at 17:05, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi hackers,

Working on [1] produced the need to give to the statistics the ability to
decide whether or not they want to be written to the file on disk.

Indeed, there is no need to write the per backend I/O stats to disk (no point to
see stats for backends that do not exist anymore after a re-start), so $SUBJECT.

I think this is a good idea. +1 for the $SUBJECT.

This new member could also be useful for custom statistics, so creating this
dedicated thread.

The attached patch also adds a test in the fixed injection points statistics.
It does not add one for the variable injection points statistics as I think adding
one test is enough (the code changes are simple enough).

There are duplicated codes in the injection_stats_fixed.c file. Do you
think that 'modifying existing functions to take an argument to
differentiate whether the kind is default or no-write' would be
better?

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On Wed, Nov 20, 2024 at 05:45:55PM +0300, Nazir Bilal Yavuz wrote:

I think this is a good idea. +1 for the $SUBJECT.

Thanks for looking at it!

There are duplicated codes in the injection_stats_fixed.c file. Do you
think that 'modifying existing functions to take an argument to
differentiate whether the kind is default or no-write' would be
better?

Some of the new functions are callbacks so we don't have the control on the
parameters list that the core is expecting.

It remains:

pgstat_register_inj_fixed_no_write()
pgstat_report_inj_fixed_no_write()
injection_points_stats_fixed_no_write()

for which I think we could add an extra "write_to_file" argument on their original
counterparts.

Not sure how many code reduction we would get, so out of curiosity I did the
exercice in v2 attached and that gives us:

v1:
8 files changed, 211 insertions(+), 2 deletions(-)

v2:
8 files changed, 152 insertions(+), 22 deletions(-)

I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Nov 20, 2024 at 05:13:18PM +0000, Bertrand Drouvot wrote:

I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.

Well, I like the enthusiasm of having tests, but injection_points
stats being persistent on disk is the only write property I want to
keep in the module, because it can be useful across clean restarts.
Your patch results in a weird mix where you now need to have a total
of four stats IDs rather than two: one more for the fixed-numbered
case and one more for the variable-numbered case to be able to control
the write/no-write property set in shared memory. The correct design,
if we were to control the write/no-write for individual entries, would
be to store a state flag in each individual entries and decide if an
entry should be flushed or not, which would require an additional
callback to check the state of an individual entry at write. Not sure
if we really need that, TBH, until someone comes up with a case for
it.

Your patch adding backend statistics would be a much better fit to add
a test for this specific property, so let's keep injection_points out
of it, even if it means that we would have only coverage for
variable-numbered stats. So let's keep it simple here, and just set
.write_to_file to true for both injection_points stats kinds per the
scope of this thread. Another point is that injection_points acts as
a template for custom pgstats, this makes the code harder to use as a
reference.

Point taken that the tests added in v2 show that the patch works.
--
Michael

Hi,

On Thu, Nov 21, 2024 at 10:01:07AM +0900, Michael Paquier wrote:

On Wed, Nov 20, 2024 at 05:13:18PM +0000, Bertrand Drouvot wrote:

I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.

Your patch adding backend statistics would be a much better fit to add
a test for this specific property

Right.

so let's keep injection_points out
of it, even if it means that we would have only coverage for
variable-numbered stats.

Right, and only when per-backend I/O stats goes in.

So let's keep it simple here, and just set
.write_to_file to true for both injection_points stats kinds per the
scope of this thread.

That's fine by me (the code is simple enough anyway). Done that way in v3
attached.

Another point is that injection_points acts as
a template for custom pgstats

That was in fact the main reason why I added this test. But well, just
adding the "write_to_file" in the injection test is enough to "show" that this
member does exist. So I'm fine with v3.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Nov 21, 2024 at 06:32:03AM +0000, Bertrand Drouvot wrote:

That was in fact the main reason why I added this test. But well, just
adding the "write_to_file" in the injection test is enough to "show" that this
member does exist. So I'm fine with v3.

Plus/minus some tweaks in the wordings (like a s/stat /stats /), the
logic is sound, and that's a lot simpler.
--
Michael

Hi,

On Thu, 21 Nov 2024 at 09:32, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

That was in fact the main reason why I added this test. But well, just
adding the "write_to_file" in the injection test is enough to "show" that this
member does exist. So I'm fine with v3.

I think that the changes below (write_to_file checks) should come
after the assertion checks. Otherwise, they could mask some problems.

=== 1
-        if (!info || !info->fixed_amount)
+        /* skip if not fixed or this kind does not want to write to the file */
+        if (!info || !info->fixed_amount || !info->write_to_file)
             continue;

if (pgstat_is_kind_builtin(kind))
Assert(info->snapshot_ctl_off != 0);

=== 2
kind_info = pgstat_get_kind_info(ps->key.kind);

+        /* skip if this kind does not want to write to the file */
+        if (!kind_info->write_to_file)
+            continue;
+
         /* if not dropped the valid-entry refcount should exist */
         Assert(pg_atomic_read_u32(&ps->refcount) > 0);
===

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On Thu, Nov 21, 2024 at 10:38:28AM +0300, Nazir Bilal Yavuz wrote:

Hi,

On Thu, 21 Nov 2024 at 09:32, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

That was in fact the main reason why I added this test. But well, just
adding the "write_to_file" in the injection test is enough to "show" that this
member does exist. So I'm fine with v3.

I think that the changes below (write_to_file checks) should come
after the assertion checks. Otherwise, they could mask some problems.

=== 1
-        if (!info || !info->fixed_amount)
+        /* skip if not fixed or this kind does not want to write to the file */
+        if (!info || !info->fixed_amount || !info->write_to_file)
continue;

if (pgstat_is_kind_builtin(kind))
Assert(info->snapshot_ctl_off != 0);

We need "if (!info || !info->fixed_amount)" before the assertion check (as
it makes sense only for fixed stats). Then I'm not sure to create a new branch
for the "write_to_file" check after this assertion is worth it.

=== 2
kind_info = pgstat_get_kind_info(ps->key.kind);

+        /* skip if this kind does not want to write to the file */
+        if (!kind_info->write_to_file)
+            continue;
+
/* if not dropped the valid-entry refcount should exist */
Assert(pg_atomic_read_u32(&ps->refcount) > 0);

Makes sense, done in v4 attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Thu, Nov 21, 2024 at 04:26:47PM +0900, Michael Paquier wrote:

On Thu, Nov 21, 2024 at 06:32:03AM +0000, Bertrand Drouvot wrote:

That was in fact the main reason why I added this test. But well, just
adding the "write_to_file" in the injection test is enough to "show" that this
member does exist. So I'm fine with v3.

Plus/minus some tweaks in the wordings (like a s/stat /stats /),

I think the singular was ok here, but v4 (just shared up-thread) uses your
proposal.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

Thanks for patch. I have tested both patches and they work as per design.

My +1 for v1, it is much cleaner approach and has properly named
functions whereas in v2 it is not.

Injection points is documented, if so, doc patch is missing.

Regards,
Yogesh

On Thu, Nov 21, 2024 at 08:49:13AM +0000, Bertrand Drouvot wrote:

On Thu, Nov 21, 2024 at 10:38:28AM +0300, Nazir Bilal Yavuz wrote:

-        if (!info || !info->fixed_amount)
+        /* skip if not fixed or this kind does not want to write to the file */
+        if (!info || !info->fixed_amount || !info->write_to_file)
continue;

if (pgstat_is_kind_builtin(kind))
Assert(info->snapshot_ctl_off != 0);

We need "if (!info || !info->fixed_amount)" before the assertion check (as
it makes sense only for fixed stats). Then I'm not sure to create a new branch
for the "write_to_file" check after this assertion is worth it.

It could still be possible that a builtin fixed-numbered stats kind is
introduced with !write_to_file. I'd want the sanity check on
snapshot_ctl_off also in this case. As-is, you are right that it does
not really matter as we use this field only to grab the fixed data
from the stats snapshot before writing it to the file, but having this
sanity check may avoid issues in the long run should write_to_file be
flipped from false to true for some reason, and the check is a free
bonus.

=== 2
kind_info = pgstat_get_kind_info(ps->key.kind);

+        /* skip if this kind does not want to write to the file */
+        if (!kind_info->write_to_file)
+            continue;
+
/* if not dropped the valid-entry refcount should exist */
Assert(pg_atomic_read_u32(&ps->refcount) > 0);

Makes sense, done in v4 attached.

Right. Checking the refcount is a good thing even if we don't write
such stats entries.

I've done both of these things, and applied the patch. Let's move on
to the next things.
--
Michael

On Thu, Nov 21, 2024 at 12:57:18PM -0500, Yogesh Sharma wrote:

Thanks for patch. I have tested both patches and they work as per design.

I've missed your message, sorry about that.
--
Michael