Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add write_to_file to PgStat_KindInfo for pgstats kinds

  1. Add a write_to_file member to PgStat_KindInfo

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-20T14:05:04Z

    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.

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).

[1]: https://www.postgresql.org/message-id/Zy4bmvgHqGjcK1pI%40ip-10-97-1-34.eu-west-3.compute.internal

Looking forward to your feedback,

Regards,

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

  2. Re: Add a write_to_file member to PgStat_KindInfo

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-11-20T14:45:55Z

    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

  3. Re: Add a write_to_file member to PgStat_KindInfo

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-20T17:13:18Z

    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

  4. Re: Add a write_to_file member to PgStat_KindInfo

    Michael Paquier <michael@paquier.xyz> — 2024-11-21T01:01:07Z

    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

  5. Re: Add a write_to_file member to PgStat_KindInfo

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-21T06:32:03Z

    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

  6. Re: Add a write_to_file member to PgStat_KindInfo

    Michael Paquier <michael@paquier.xyz> — 2024-11-21T07:26:47Z

    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

  7. Re: Add a write_to_file member to PgStat_KindInfo

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-11-21T07:38:28Z

    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

  8. Re: Add a write_to_file member to PgStat_KindInfo

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-21T08:49:13Z

    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

  9. Re: Add a write_to_file member to PgStat_KindInfo

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-21T08:49:31Z

    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

  10. Re: Add a write_to_file member to PgStat_KindInfo

    Yogesh Sharma <yogesh.sharma@catprosystems.com> — 2024-11-21T17:57:18Z

    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 11/20/24 12:13, Bertrand Drouvot wrote:
> 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,
>

  11. Re: Add a write_to_file member to PgStat_KindInfo

    Michael Paquier <michael@paquier.xyz> — 2024-11-22T01:15:26Z

    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

  12. Re: Add a write_to_file member to PgStat_KindInfo

    Michael Paquier <michael@paquier.xyz> — 2024-11-22T02:31:04Z

    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