Horton

Thread

Commits

  1. Add callback for backend initialization in pgstats

  1. Add callback in pgstats for backend initialization

    Michael Paquier <michael@paquier.xyz> — 2024-09-03T01:52:20Z

    Hi all,

Currently, the backend-level initialization of pgstats happens in
pgstat_initialize(), where we are using a shortcut for the WAL stats,
with pgstat_init_wal().

I'd like to propose to add a callback for that, so as in-core or
custom pgstats kinds can assign custom actions when initializing a
backend.  The use-case I had for this one are pretty close to what we
do for WAL, where we would rely on a difference of state depending on
what may have existed before reaching the initialization path.  So
this can offer more precision.  Another case, perhaps less
interesting, is to be able to initialize some static backend state.

I wanted to get that sent for the current commit fest, but did not get
back in time to it.  Anyway, here it is.  This gives the simple patch
attached.

Thanks,
--
Michael

  2. Re: Add callback in pgstats for backend initialization

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-03T05:00:54Z

    Hi,

On Tue, Sep 03, 2024 at 10:52:20AM +0900, Michael Paquier wrote:
> Hi all,
> 
> Currently, the backend-level initialization of pgstats happens in
> pgstat_initialize(), where we are using a shortcut for the WAL stats,
> with pgstat_init_wal().
> 
> I'd like to propose to add a callback for that, so as in-core or
> custom pgstats kinds can assign custom actions when initializing a
> backend.  The use-case I had for this one are pretty close to what we
> do for WAL, where we would rely on a difference of state depending on
> what may have existed before reaching the initialization path.  So
> this can offer more precision.  Another case, perhaps less
> interesting, is to be able to initialize some static backend state.

I think the proposal makes sense and I can see the use cases too, so +1.

> This gives the simple patch
> attached.

Should we add a few words about this new callback (and the others in
PgStat_KindInfo while at it) in the "Custom Cumulative Statistics" section of
xfunc.sgml?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  3. Re: Add callback in pgstats for backend initialization

    Michael Paquier <michael@paquier.xyz> — 2024-09-03T05:58:49Z

    On Tue, Sep 03, 2024 at 05:00:54AM +0000, Bertrand Drouvot wrote:
> Should we add a few words about this new callback (and the others in
> PgStat_KindInfo while at it) in the "Custom Cumulative Statistics" section of
> xfunc.sgml?

Not sure if it is worth having.  This adds extra maintenance and
there's already a mention of src/include/utils/pgstat_internal.h
telling where the callbacks are described.
--
Michael

  4. Re: Add callback in pgstats for backend initialization

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-09-04T05:15:43Z

    At Tue, 3 Sep 2024 05:00:54 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in 
> Hi,
> 
> On Tue, Sep 03, 2024 at 10:52:20AM +0900, Michael Paquier wrote:
> > Hi all,
> > 
> > Currently, the backend-level initialization of pgstats happens in
> > pgstat_initialize(), where we are using a shortcut for the WAL stats,
> > with pgstat_init_wal().
> > 
> > I'd like to propose to add a callback for that, so as in-core or
> > custom pgstats kinds can assign custom actions when initializing a
> > backend.  The use-case I had for this one are pretty close to what we
> > do for WAL, where we would rely on a difference of state depending on
> > what may have existed before reaching the initialization path.  So
> > this can offer more precision.  Another case, perhaps less
> > interesting, is to be able to initialize some static backend state.
> 
> I think the proposal makes sense and I can see the use cases too, so +1.

+1, too.

The name "init_backend" makes it sound like the function initializes
the backend. backend_init might be a better choice, but I'm not sure.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: Add callback in pgstats for backend initialization

    Michael Paquier <michael@paquier.xyz> — 2024-09-04T06:04:09Z

    On Wed, Sep 04, 2024 at 02:15:43PM +0900, Kyotaro Horiguchi wrote:
> The name "init_backend" makes it sound like the function initializes
> the backend. backend_init might be a better choice, but I'm not sure.

We (kind of) tend to prefer $verb_$thing-or-action_cb for the name of
the callbacks, which is why I chose this naming.  If you feel strongly
about "backend_init_cb", that's also fine by me; you are the original
author of this code area with Andres.
--
Michael

  6. Re: Add callback in pgstats for backend initialization

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-09-04T09:42:33Z

    At Wed, 4 Sep 2024 15:04:09 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Sep 04, 2024 at 02:15:43PM +0900, Kyotaro Horiguchi wrote:
> > The name "init_backend" makes it sound like the function initializes
> > the backend. backend_init might be a better choice, but I'm not sure.
> 
> We (kind of) tend to prefer $verb_$thing-or-action_cb for the name of
> the callbacks, which is why I chose this naming.  If you feel strongly
> about "backend_init_cb", that's also fine by me; you are the original
> author of this code area with Andres.

More accurately, I believe this applies when the sentence follows a
verb-object structure. In this case, the function’s meaning is “pgstat
initialization on backend,” which seems somewhat different from the
policy you mentioned. However, it could also be interpreted as
“initialize backend status related to pgstat.” Either way, I’m okay
with the current name if you are, based on the discussion above.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. Re: Add callback in pgstats for backend initialization

    Michael Paquier <michael@paquier.xyz> — 2024-09-05T03:41:09Z

    On Wed, Sep 04, 2024 at 06:42:33PM +0900, Kyotaro Horiguchi wrote:
> More accurately, I believe this applies when the sentence follows a
> verb-object structure. In this case, the function’s meaning is “pgstat
> initialization on backend,” which seems somewhat different from the
> policy you mentioned. However, it could also be interpreted as
> “initialize backend status related to pgstat.” Either way, I’m okay
> with the current name if you are, based on the discussion above.

Not sure which one is better than the other, TBH.  Either way, we
still have a full release cycle to finalize that, and I am OK to
switch the name to something else if there are more folks in favor of
one, the other or even something entirely different but descriptive
enough.
--
Michael

  8. Re: Add callback in pgstats for backend initialization

    Michael Paquier <michael@paquier.xyz> — 2024-09-05T07:08:05Z

    On Thu, Sep 05, 2024 at 12:41:09PM +0900, Michael Paquier wrote:
> Not sure which one is better than the other, TBH.  Either way, we
> still have a full release cycle to finalize that, and I am OK to
> switch the name to something else if there are more folks in favor of
> one, the other or even something entirely different but descriptive
> enough.

I've used init_backend_cb at the end in 1b373aed20e6.  A change would
be OK by me, if needed.
--
Michael