Re: Add last_commit_lsn to pg_stat_database

Hello,

Thanks for reviewing!

On Tue, Sep 19, 2023 at 8:16 AM Aleksander Alekseev
<aleksander(at)timescale(dot)com> wrote:
>
> Hi,
>
> > [...]
> > As I was thinking about how to improve things, I realized that this
> > information (since it's for monitoring anyway) fits more naturally
> > into the stats system. I'd originally thought of exposing it in
> > pg_stat_wal, but that's per-cluster rather than per-database (indeed,
> > this is a flaw I hadn't considered in the original patch), so I think
> > pg_stat_database is the correct location.
> >
> > I've attached a patch to track the latest commit's LSN in pg_stat_database.
>
> Thanks for the patch. It was marked as "Needs Review" so I decided to
> take a brief look.
>
> All in all the code seems to be fine but I have a couple of nitpicks:
>
> - If you are modifying pg_stat_database the corresponding changes to
> the documentation are needed.

Updated.

> - pg_stat_get_db_last_commit_lsn() has the same description as
> pg_stat_get_db_xact_commit() which is confusing.

I've fixed this.

> - pg_stat_get_db_last_commit_lsn() is marked as STABLE which is
> probably correct. But I would appreciate a second opinion on this.

Sounds good.

> - Wouldn't it be appropriate to add a test or two?

Added.

> - `if (!XLogRecPtrIsInvalid(commit_lsn))` - I suggest adding
> XLogRecPtrIsValid() macro for better readability

We have 36 usages of !XLogRecPtrIsInvalid(...) already, so I think we
should avoid making this change in this patch.

v2 is attached.

Regards,
James Coleman