Message-ID: <02cfc5e7-e152-4d2d-8b4b-e899d9901ed5@gmail.com>
Date: Thu, 23 Apr 2026 09:34:20 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add pg_stat_vfdcache view for VFD cache statistics
To: KAZAR Ayoub <ma_kazar@esi.dz>, Tomas Vondra <tomas@vondra.me>
Cc: Jakub Wartak <jakub.wartak@enterprisedb.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>
References: 
 <CA+K2RumP33Cpj--88E+rNADa8fzSBBiav=rvzyaMM=sYNaOkfA@mail.gmail.com>
 <CAKZiRmzs99+q55pgBJ0-RHxL-z=ZRD-22cGR2w9jscOyxY-vfA@mail.gmail.com>
 <c85907b2-1e91-47e3-82dc-dafda295ded4@gmail.com>
 <CA+K2Ru=WXpG1Sfx=_DQVn7ENZz6C_j-HxVJ1TmkEKowLVQcYwg@mail.gmail.com>
 <d305fd37-346b-4512-808e-5dd7968eb569@gmail.com>
 <CA+K2RuncGCW55b-XUe8gJGwdMSOgTFKG-uSCgbVDZ+HewiT5EQ@mail.gmail.com>
 <CA+K2RumDZ05pru3-YSZxe3Y==vO0hsahiweyvJw6QsjuGR5WsA@mail.gmail.com>
 <42776281-3603-4161-b47d-d4ffd2029e8c@vondra.me>
 <CA+K2RumSp-kTw_YHXs_qN_RLt6cWfFR=LMq9coLgu8eyGydpHQ@mail.gmail.com>
Content-Language: en-US
From: David Geier <geidav.pg@gmail.com>
In-Reply-To: 
 <CA+K2RumSp-kTw_YHXs_qN_RLt6cWfFR=LMq9coLgu8eyGydpHQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/02cfc5e7-e152-4d2d-8b4b-e899d9901ed5%40gmail.com>
Precedence: bulk

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?

>> 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.

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.

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?

	/*
	 * 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.

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.

--
David Geier