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(dot)pg(at)gmail(dot)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