public inbox for [email protected]  
help / color / mirror / Atom feed
From: Michael Paquier <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: wenhui qiu <[email protected]>
Cc: Robert Treat <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: PoC: history of recent vacuum/checkpoint runs (using new hooks)
Date: Fri, 27 Dec 2024 13:00:37 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAGjGUAJSg3tJGxjeyU6T98ZYCXcAiGpq0eitAUQVaOa+nULDuA@mail.gmail.com>
	<[email protected]>
	<CABV9wwPcnoS+=27o=pHinCy7fegTTfB=yECyK5XyGecCFfrFHg@mail.gmail.com>
	<CAGjGUAKXjFw+zq+9GZ+y5p360pkO7UuTojkKuB6k3MtdDCEf6w@mail.gmail.com>
	<[email protected]>

On Thu, Dec 26, 2024 at 06:58:11PM +0100, Tomas Vondra wrote:
> If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not
> make a fundamental difference ...
> 
> Anyway, the 128MB value is rather arbitrary. I don't mind increasing the
> limit, or possibly removing it entirely (and accepting anything the
> system can handle).

+    DefineCustomIntVariable("stats_history.size",
+                            "Sets the amount of memory available for past events.",

How about some time-based retention?  Data size can be hard to think
about for the end user, while there would be a set of users that would
want to retain data for the past week, month, etc?  If both size and
time upper-bound are define, then entries that match one condition or
the other are removed.

+        checkpoint_log_hook(
+          CheckpointStats.ckpt_start_t,            /* start_time */
+          CheckpointStats.ckpt_end_t,                /* end_time */
+          (flags & CHECKPOINT_IS_SHUTDOWN),        /* is_shutdown */
+          (flags & CHECKPOINT_END_OF_RECOVERY),    /* is_end_of_recovery */
+          (flags & CHECKPOINT_IMMEDIATE),            /* is_immediate */
+          (flags & CHECKPOINT_FORCE),                /* is_force */
+          (flags & CHECKPOINT_WAIT),                /* is_wait */
+          (flags & CHECKPOINT_CAUSE_XLOG),        /* is_wal */
+          (flags & CHECKPOINT_CAUSE_TIME),        /* is_time */
+          (flags & CHECKPOINT_FLUSH_ALL),            /* is_flush_all */
+          CheckpointStats.ckpt_bufs_written,        /* buffers_written */
+          CheckpointStats.ckpt_slru_written,        /* slru_written */
+          CheckpointStats.ckpt_segs_added,        /* segs_added */
+          CheckpointStats.ckpt_segs_removed,        /* segs_removed */
+          CheckpointStats.ckpt_segs_recycled,        /* segs_recycled */

That's a lot of arguments.  CheckpointStatsData and the various
CHECKPOINT_* flags are exposed, why not just send these values to the
hook?

For v1-0001 as well, I'd suggest some grouping with existing
structures, or expose these structures so as they can be reused for
out-of-core code via the proposed hook.  More arguments lead to more
mistakes that could be easily avoided.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
  download

view thread (10+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: PoC: history of recent vacuum/checkpoint runs (using new hooks)
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox