| From: | Sami Imseih <samimseih(at)gmail(dot)com> |
|---|---|
| To: | Michael Paquier <michael(at)paquier(dot)xyz> |
| Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: add validations for required callbacks during pgstat_register_kind() |
| Date: | 2026-07-03 16:04:56 |
| Message-ID: | CAA5RZ0tr9yLrx2RSXJ=bKB48CF2yV4nOGD-j3FEk1U+-2M5aTw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0003-Add-Asserts-for-required-callbacks-before-invocat.patch | application/octet-stream | 1.7 KB |
| v2-0001-Add-validations-for-required-callbacks-during-pgs.patch | application/octet-stream | 2.9 KB |
| v2-0002-Make-error-messages-in-pgstat_register_kind-consi.patch | application/octet-stream | 2.4 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Florin Irion | 2026-07-03 16:26:03 | Re: [PATCH] pg_get_domain_ddl: DDL reconstruction function for CREATE DOMAIN statement |
| Previous Message | Fujii Masao | 2026-07-03 15:46:46 | Re: Truncate logs by max_log_size |