Re: Add pg_stat_vfdcache view for VFD cache statistics

Hello,
On Thu, Apr 23, 2026, 9:34 AM David Geier <geidav(dot)pg(at)gmail(dot)com> wrote:

> Hi!
>
> I finally got around taking a look at this patch.
>
> On 03.04.2026 15:53, KAZAR Ayoub wrote:
> >> - I think it would be useful to report how many file descriptors we
> >> are allowed to open (it's less than max_files_per_process, depending
> >> on the ulimits etc.)
> >>
> > Agree, This should be max_safe_fds calculated by postmaster, I added this
> > but let me know if its acceptable to export max_safe_fds in the way I
> did.
>
> An alternative to including it in the view would be using a GUC of type
> PGC_INTERNAL. That seems more inline with how we expose other PostgreSQL
> internal read-only variables that don't change.
>
> Or is there an advantage to including max_safe_fds in the view?
>
There's no significant advantage other than seeing all info related to
vfdcache together in one view
Also in fd.h i just remembered:
/*
* This is private to fd.c, but exported for
save/restore_backend_variables()
*/
extern PGDLLIMPORT int max_safe_fds;

I don't think it's intended to be exported for reads like i'm doing (which
can be fine?), or maybe update this comment about its export.

>
> >> The global cache stats is going to be virtually free (at least the
> >> hits/misses, I'm not sure about the number of entries and bytes), and
> >> it's obviously useful for tuning the max_files_per_process GUC. I'd even
> >> contemplate getting this into PG19, maybe.
>
> The number of used entries already exists, see nfile in fd.c.
>
Would one want the number of all entries (i.e SizeVfdCache see fd.c) or the
number of used entries (i.e entries with fds in use, which is nfile) ? I
thought of the first, that's what 0002 patch contains for the moment.

>
> Including the total cache size would also be virtually free if we don't
> iterate over all VFDs each time, but update the size as we go. That
> would have to happen when resizing the cache and when populating /
> freeing a cache entry because extra memory is allocated / freed for
> Vfd::fileName.
>
Is it a big deal if we miss some bytes of filename globally ?

>
> I'm happy to code this up if there's agreement that it's sensible to
> include it, in the current version of the patch or a follow-up patch.
>
> Beyond that:
>
> While looking through the code I saw a mistake (repetition of "that") in
> a comment in existing code. Maybe you want to fix that as well right away?

Noted.

>
> /*
> * For variable-numbered stats: flush pending stats. Required if
> pending
> * data is used. See flush_static_cb when dealing with stats data
> that
> * that cannot use PgStat_EntryRef->pending.
> */
> bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool
> nowait);
>
> The indentation of the type at the end of the following two structs is
> inconsistent with the rest of the files.
>
Fixed.
That's pg_indent doing me dirty, although i know it's wrong i didn't
understand why it kept indenting like this, only in those two structs.

>
> typedef struct PgStatShared_VfdCache
> {
> /* lock protects ->stats */
> LWLock lock;
> PgStat_VfdCacheStats stats;
> } PgStatShared_VfdCache;
>
> typedef struct PgStat_VfdCacheStats
> {
> PgStat_Counter vfd_hits; /* fd was open, no open() was
> needed */
> PgStat_Counter vfd_misses; /* fd was VFD_CLOSED, open() was
> required */
> TimestampTz stat_reset_timestamp;
> } PgStat_VfdCacheStats;
>
> Apart from these nit comments the patch looks good to me.
>
Thanks for the review!
Other than the above small changes, i'll be moving forward with 0002 which
is also ready.

Regards,
Ayoub