Thanks so much Bharath, Andrey, and Ilya for the review! I've posted a new version here [1] which addresses some of your concerns. I'll comment on those it does not address inline.
On Thu, May 30, 2024 at 1:26 PM Andrey M. Borodin <x4...@yandex-team.ru> wrote: > > === Questions === > 1. The patch does not handle server restart. All pages will need freeze after > any crash? I haven't fixed this yet. See my email for some thoughts on what I should do here. > 2. Some benchmarks to proof the patch does not have CPU footprint. This is still a todo. Typically when designing a benchmark like this, I would want to pick a worst-case workload to see how bad it could be. I wonder if just a write heavy workload like pgbench builtin tpcb-like would be sufficient? > === Nits === > "Timeline" term is already taken. I changed it to LSNTimeStream. What do you think? > The patch needs rebase due to some header changes. I did this. > Tests fail on Windows. I think this was because of the compiler warnings, but I need to double-check now. > The patch lacks tests. I thought about this a bit. I wonder what kind of tests make sense. I could 1) Add tests with the existing stats tests (src/test/regress/sql/stats.sql) and just test that bgwriter is in fact adding to the time stream. 2) Or should I add some infrastructure to be able to create an LSNTimeStream and then insert values to it and do some validations of what is added? I did a version of this but it is just much more annoying with C & SQL than with python (where I tried out my algorithms) [2]. > Some docs would be nice, but the feature is for developers. I added some docs. > Mapping is protected for multithreaded access by walstats LWlock and might > have tuplestore_putvalues() under that lock. That might be a little > dangerous, if tuplestore will be on-disk for some reason (should not happen). Ah, great point! I forgot about the *fetch_stat*() functions. I used pgstat_fetch_stat_wal() in the latest version so I have a local copy that I can stuff into the tuplestore without any locking. It won't be as up-to-date, but I think that is 100% okay for this function. - Melanie [1] https://www.postgresql.org/message-id/CAAKRu_a6WSkWPtJCw%3DW%2BP%2Bg-Fw9kfA_t8sMx99dWpMiGHCqJNA%40mail.gmail.com [2] https://gist.github.com/melanieplageman/95126993bcb43d4b4042099e9d0ccc11
