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 <x4mmm@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