Re: Add a write_to_file member to PgStat_KindInfo

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