Thanks for taking a look! > Feel free to add me in CC for such changes; this would drop in my > inbox.
Noted.
> + ereport(ERROR,
> + (errmsg("custom cumulative statistics property is
> invalid"),
> + errhint("Custom cumulative statistics require
> init_shmem_cb, reset_all_cb, and snapshot_cb callbacks for fixed-numbered
> objects.")));
>
> Function names in error messages ought to be replaced with %s
> placeholders, perhaps? The two messages may be better if matching the
> surroundings, with "failed to register custom .. with ID" being the
> primary message, and the hint telling what's missing.
Done in v2-0001.
v2-0002 also makes the errmsg consistent and includes kind names
(except for when a name is not provided)
and kind IDs across all the messages.
> if (kind_info->fixed_amount)
> + {
> + Assert(kind_info->reset_all_cb != NULL);
> kind_info->reset_all_cb(ts);
> + }
> [...]
> + Assert(kind_info->reset_all_cb != NULL);
> kind_info->reset_all_cb(ts);
> [...]
> + Assert(kind_info->init_shmem_cb != NULL);
> kind_info->init_shmem_cb(ptr);
>
> These three don't really bring extra value. If the pointer is not
> set, we just crash the line after on a NULL pointer dereference,
> leading to the same result at the end.
hmm, I think it's better to have a clear assertion log ( when built
with asserts ),
than a segfault. Also, it's self-documenting. It is also consistent with
snapshot_cb. I think we should keep it, but I separated that into a v2-0003,
Perhaps you will change your mind.
--
Sami
v2-0003-Add-Asserts-for-required-callbacks-before-invocat.patch
Description: Binary data
v2-0001-Add-validations-for-required-callbacks-during-pgs.patch
Description: Binary data
v2-0002-Make-error-messages-in-pgstat_register_kind-consi.patch
Description: Binary data
