Re: Add LSN <-> time conversion functionality

Thanks for the review!

Attached v4 implements the new algorithm/compression described in [1].

We had discussed off-list possibly using error in some way. So, I'm
interested to know what you think about this method I suggested which
calculates error. It doesn't save the error so that we could use it
when interpolating for reasons I describe in that mail. If you have
any ideas on how to use the calculated error or just how to combine
error when dropping a point, that would be super helpful.

Note that in this version, I've changed the name from LSNTimeline to
LSNTimeStream to address some feedback from another reviewer about
Timeline being already in use in Postgres as a concept.

On Mon, Mar 18, 2024 at 10:02 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 22 Feb 2024, at 03:45, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
> > On Fri, Feb 16, 2024 at 3:41 PM Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> >> - I wonder what happens if we lose the data - we know that if people
> >> reset statistics for whatever reason (or just lose them because of a
> >> crash, or because they're on a replica), bad things happen to
> >> autovacuum. What's the (expected) impact on pruning?
> >
> > This is an important question. Because stats aren't crashsafe, we
> > could return very inaccurate numbers for some time/LSN values if we
> > crash. I don't actually know what we could do about that. When I use
> > the LSNTimeline for the freeze heuristic it is less of an issue
> > because the freeze heuristic has a fallback strategy when there aren't
> > enough stats to do its calculations. But other users of this
> > LSNTimeline will simply get highly inaccurate results (I think?). Is
> > there anything we could do about this? It seems bad.
>
> A complication with this over stats is that we can't recompute this in case of
> a crash/corruption issue. The simple solution is to consider this unlogged
> data and start fresh at every unclean shutdown, but I have a feeling that won't
> be good enough for basing heuristics on?

Yes, I still haven't dealt with this yet. Tomas had a list of
suggestions in an upthread email, so I will spend some time thinking
about those next.

It seems like we might be able to come up with some way of calculating
a valid "default" value or "best guess" which could be used whenever
there isn't enough data. Though, if we crash and lose some time stream
data, we won't know that that data was lost due to a crash so we
wouldn't know to use our "best guess" to make up for it. So, maybe I
should try and rebuild the stream using some combination of WAL, clog,
and commit timestamps? Or perhaps I should do some basic WAL logging
just for this data structure.

> > Andres had brought up something at some point about, what if the
> > database is simply turned off for awhile and then turned back on. Even
> > if you cleanly shut down, will there be "gaps" in the timeline? I
> > think that could be okay, but it is something to think about.
>
> The gaps would represent reality, so there is nothing wrong per se with gaps,
> but if they inflate the interval of a bucket which in turns impact the
> precision of the results then that can be a problem.

Yes, actually I added some hacky code to the quick and dirty python
simulator I wrote [2] to test out having a big gap with no updates (if
there is no db activity so nothing for bgwriter to do or the db is off
for a while). And it seemed to basically work fine.

> While the bucketing algorithm is a clever algorithm for degrading precision for
> older entries without discarding them, I do fear that we'll risk ending up with
> answers like "somewhere between in the past and even further in the past".
> I've been playing around with various compression algorithms for packing the
> buckets such that we can retain precision for longer. Since you were aiming to
> work on other patches leading up to the freeze, let's pick this up again once
> things calm down.

Let me know what you think about the new algorithm. I wonder if for
points older than the second to oldest point, we have the function
return something like "older than a year ago" instead of guessing. The
new method doesn't focus on compressing old data though.

> When compiling I got this warning for lsntime_merge_target:
>
> pgstat_wal.c:242:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
> }
> ^
> 1 warning generated.
>
> The issue seems to be that the can't-really-happen path is protected with an
> Assertion, which is a no-op for production builds. I think we should handle
> the error rather than rely on testing catching it (since if it does happen even
> though it can't, it's going to be when it's for sure not tested). Returning an
> invalid array subscript like -1 and testing for it in lsntime_insert, with an
> elog(WARNING..), seems enough.
>
>
> - last_snapshot_lsn <= GetLastImportantRecPtr())
> + last_snapshot_lsn <= current_lsn)
> I think we should delay extracting the LSN with GetLastImportantRecPtr until we
> know that we need it, to avoid taking locks in this codepath unless needed.
>
> I've attached a diff with the above suggestions which applies on op of your
> patchset.

I've implemented these review points in the attached v4.

- Melanie

[1] https://www.postgresql.org/message-id/CAAKRu_YbbZGz-X_pm2zXJA%2B6A22YYpaWhOjmytqFL1yF_FCv6w%40mail.gmail.com
[2] https://gist.github.com/melanieplageman/7400e81bbbd518fe08b4af55a9b632d4