On Wed, Nov 20, 2024 at 05:13:18PM +0000, Bertrand Drouvot wrote: > I don't have a strong opinion for this particular case here (I think the code > is harder to read but yeah there is some code reduction): so I'm fine with > v2 too.
Well, I like the enthusiasm of having tests, but injection_points stats being persistent on disk is the only write property I want to keep in the module, because it can be useful across clean restarts. Your patch results in a weird mix where you now need to have a total of four stats IDs rather than two: one more for the fixed-numbered case and one more for the variable-numbered case to be able to control the write/no-write property set in shared memory. The correct design, if we were to control the write/no-write for individual entries, would be to store a state flag in each individual entries and decide if an entry should be flushed or not, which would require an additional callback to check the state of an individual entry at write. Not sure if we really need that, TBH, until someone comes up with a case for it. Your patch adding backend statistics would be a much better fit to add a test for this specific property, so let's keep injection_points out of it, even if it means that we would have only coverage for variable-numbered stats. So let's keep it simple here, and just set .write_to_file to true for both injection_points stats kinds per the scope of this thread. Another point is that injection_points acts as a template for custom pgstats, this makes the code harder to use as a reference. Point taken that the tests added in v2 show that the patch works. -- Michael
signature.asc
Description: PGP signature
