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
signature.asc
Description: PGP signature
