Re: add validations for required callbacks during pgstat_register_kind()

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

In response to

Browse pgsql-hackers by date

  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