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
callbacksfor 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
