Re: Add a write_to_file member to PgStat_KindInfo

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