| From: | Ayoub Kazar <kazarayoub2004(at)gmail(dot)com> |
|---|---|
| To: | solai v <solai(dot)cdac(at)gmail(dot)com> |
| Cc: | David Geier <geidav(dot)pg(at)gmail(dot)com>, KAZAR Ayoub <ma_kazar(at)esi(dot)dz>, Tomas Vondra <tomas(at)vondra(dot)me>, Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Add pg_stat_vfdcache view for VFD cache statistics |
| Date: | 2026-08-26 13:03:00 |
| Message-ID: | CADu+CpTuf-EQkU=_fCx_mjfyhndBe=4JT9UxX7ExQCDegmk3yA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello Solai,
On Mon, Aug 24, 2026 at 8:31 AM solai v <solai(dot)cdac(at)gmail(dot)com> wrote:
>
> Thank you for the updated patches. I have completed the review and
> testing of the v5 patch series for introducing the pg_stat_vfdcache
> view and VFD cache footprint metrics. I tested the patch on PostgreSQL
> 20devel with the debug and assertion-enabled build and verified the
> following:
>
> 1. Successfully built and ran the patched PostgreSQL tree.
> 2. Verified the newly introduced pg_stat_vfdcache view and its
> reported statistics.
> 3. Tested VFD hit/miss accounting through repeated table accesses.
> 4. Tested VFD cache behavior using a partitioned table with 1500
> partitions and 1.5 million rows.
> 5. Tested VFD pressure with a reduced max_files_per_process setting.
> 6. Verified the behavior of: hits, misses, open_entries,
> allocated_entries, cache_bytes, max_open_fds, and hit_ratio.
> 7. Tested repeated scans and observed the corresponding changes in VFD
> cache statistics.
> 8. Tested statistics reset using pg_stat_reset_shared('vfdcache').
> 9. Verified the behavior across multiple backend sessions.
> 10. Tested the statistics behavior across server restart.
> 11. Inspected the implementation of VFD cache footprint accounting in
> fd.c, including VFD allocation, reuse, filename allocation/freeing,
> and error handling.
>
Thanks for the thorough review.
>
> And also reviewed the statistics flush mechanism and confirmed that
> VFD hit/miss counters are maintained locally and flushed through the
> existing fixed-statistics infrastructure rather than taking a
> shared-memory lock on every VFD access.
> The main point I noticed is related to regression test coverage. The
> current regression test verifies that pg_stat_reset_shared('vfdcache')
> updates the stats_reset timestamp, but it does not directly exercise
> and verify the newly introduced VFD statistics such as hits, misses,
> open_entries, allocated_entries, and cache_bytes. I suggest adding a
> focused regression test covering the actual VFD activity and the newly
> exposed statistics, while avoiding assertions on fragile exact counter
> values where appropriate.
> Apart from these, I did not observe any functional issues during my
> testing and the patch looks good to me.
I rebased the patch (attached).
I agree that the patch needs some activity tests.
For hits & missed, i find a before/after test following the `pg_stat_wal`
pattern is good:
SELECT hits + misses AS vfd_accesses_before FROM pg_stat_vfdcache \gset
CREATE TEMP TABLE vfd_activity_test (i int);
INSERT INTO vfd_activity_test SELECT generate_series(1, 100);
SELECT count(*) FROM vfd_activity_test;
SELECT pg_stat_force_next_flush();
SELECT hits + misses > :vfd_accesses_before FROM pg_stat_vfdcache;
DROP TABLE vfd_activity_test;
For open_entries, allocated_entries, cache_bytes delta tests are not
reliable, open_entries can decrease between activity and measurement as
files are closed. `allocated_entries` is non-decreasing within a single
backend's lifetime, but the view sums across all backends ; any other
activity's effects (like vacuum) might mislead the test a bit.
For the latter counters, we can do some asserts like:
SELECT open_entries <= allocated_entries AS footprint_invariant
FROM pg_stat_vfdcache;
This for example means we never report more open FDs than allocated slots,
which should always be true (and perhaps other tests similar to this).
Do you agree with this, or do you have other recommendations? I will
proceed with v6 once we have alignment on this.
Regards,
Ayoub
| Attachment | Content-Type | Size |
|---|---|---|
| v5-0001-Add-pg_stat_vfdcache-view-for-VFD-cache-statistics.patch | text/x-patch | 24.8 KB |
| v5-0002-Add-VFD-cache-footprint-metrics-to-pg_stat_vfdcache.patch | text/x-patch | 17.6 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ayush Tiwari | 2026-08-26 13:15:18 | Re: More partition pruning bugs with multi-column RANGE partitions |
| Previous Message | Vaibhav Dalvi | 2026-08-26 13:01:54 | Re: [PATCH] Add support for INSERT ... SET syntax |