Re: Add LSN <-> time conversion functionality

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(at)yandex-team(dot)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