Add last commit LSN to pg_last_committed_xact()
I'd recently been thinking about monitoring how many bytes behind a
logical slot was and realized that's not really possible to compute
currently. That's easy enough with a physical slot because we can get
the current WAL LSN easily enough and the slot exposes the current LSN
positions of the slot. However for logical slots that naive
computation isn't quite right. The logical slot can't flush past the
last commit, so even if there's 100s of megabytes of unflushed WAL on
the slot there may be zero lag (in terms of what's possible to
process).
I've attached a simple patch (sans tests and documentation) to get
feedback early. After poking around this afternoon it seemed to me
that the simplest approach was to hook into the commit timestamps
infrastructure and store the commit's XLogRecPtr in the cache of the
most recent value (but of course don't write it out to disk). That the
downside of making this feature dependent on "track_commit_timestamps
= on", but that seems reasonable:
1. Getting the xid of the last commit is similarly dependent on commit
timestamps infrastructure.
2. It's a simple place to hook into and avoids new shared data and locking.
Thoughts?
Thanks,
James Coleman
Attachments:
v1-0001-Expose-LSN-of-last-commit-via-pg_last_committed_x.patchapplication/octet-stream; name=v1-0001-Expose-LSN-of-last-commit-via-pg_last_committed_x.patchDownload+29-18
On Fri, Jan 14, 2022 at 7:42 PM James Coleman <jtc331@gmail.com> wrote:
I've attached a simple patch (sans tests and documentation) to get
feedback early. After poking around this afternoon it seemed to me
that the simplest approach was to hook into the commit timestamps
infrastructure and store the commit's XLogRecPtr in the cache of the
most recent value (but of course don't write it out to disk). That the
downside of making this feature dependent on "track_commit_timestamps
= on", but that seems reasonable:1. Getting the xid of the last commit is similarly dependent on commit
timestamps infrastructure.
2. It's a simple place to hook into and avoids new shared data and locking.Thoughts?
It doesn't seem great to me. It's making commit_ts do something other
than commit timestamps, which looks kind of ugly.
In general, I'm concerned about the cost of doing something like this.
Extra shared memory updates as part of the process of committing a
transaction are not (and can't be made) free.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2022-Jan-14, James Coleman wrote:
The logical slot can't flush past the
last commit, so even if there's 100s of megabytes of unflushed WAL on
the slot there may be zero lag (in terms of what's possible to
process).I've attached a simple patch (sans tests and documentation) to get
feedback early. After poking around this afternoon it seemed to me
that the simplest approach was to hook into the commit timestamps
infrastructure and store the commit's XLogRecPtr in the cache of the
most recent value (but of course don't write it out to disk).
Maybe it would work to have a single LSN in shared memory, as an atomic
variable, which uses monotonic advance[1]part of a large patch at /messages/by-id/202111222156.xmo2yji5ifi2@alvherre.pgsql to be updated. Whether this is
updated or not would depend on a new GUC, maybe track_latest_commit_lsn.
Causing performance pain during transaction commit is not great, but at
least this way it shouldn't be *too* a large hit.
[1]: part of a large patch at /messages/by-id/202111222156.xmo2yji5ifi2@alvherre.pgsql
/messages/by-id/202111222156.xmo2yji5ifi2@alvherre.pgsql
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)
On Mon, Jan 17, 2022 at 4:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Jan-14, James Coleman wrote:
The logical slot can't flush past the
last commit, so even if there's 100s of megabytes of unflushed WAL on
the slot there may be zero lag (in terms of what's possible to
process).I've attached a simple patch (sans tests and documentation) to get
feedback early. After poking around this afternoon it seemed to me
that the simplest approach was to hook into the commit timestamps
infrastructure and store the commit's XLogRecPtr in the cache of the
most recent value (but of course don't write it out to disk).Maybe it would work to have a single LSN in shared memory, as an atomic
variable, which uses monotonic advance[1] to be updated. Whether this is
updated or not would depend on a new GUC, maybe track_latest_commit_lsn.
Causing performance pain during transaction commit is not great, but at
least this way it shouldn't be *too* a large hit.
I don't know if it would or not, but it's such a hot path that I find
the idea a bit worrisome. Atomics aren't free - especially inside of a
loop.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Mon, Jan 17, 2022 at 4:20 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Jan 14, 2022 at 7:42 PM James Coleman <jtc331@gmail.com> wrote:
I've attached a simple patch (sans tests and documentation) to get
feedback early. After poking around this afternoon it seemed to me
that the simplest approach was to hook into the commit timestamps
infrastructure and store the commit's XLogRecPtr in the cache of the
most recent value (but of course don't write it out to disk). That the
downside of making this feature dependent on "track_commit_timestamps
= on", but that seems reasonable:1. Getting the xid of the last commit is similarly dependent on commit
timestamps infrastructure.
2. It's a simple place to hook into and avoids new shared data and locking.Thoughts?
It doesn't seem great to me. It's making commit_ts do something other
than commit timestamps, which looks kind of ugly.
I wondered about that, but commit_ts already does more than commit
timestamps by recording the xid of the last commit.
For that matter, keeping a cache of last commit metadata in shared
memory is arguably not obviously implied by "track_commit_timestamps",
which leads to the below...
In general, I'm concerned about the cost of doing something like this.
Extra shared memory updates as part of the process of committing a
transaction are not (and can't be made) free.
It seems to me that to the degree there's a hot path concern here we
ought to separate out the last commit metadata caching from the
"track_commit_timestamps" feature (at least in terms of how it's
controlled by GUCs). If that were done we could also, in theory, allow
controlling which items are tracked to reduce hot path cost if only a
subset is needed. For that matter it'd also allow turning on this
metadata caching without enabling the commit timestamp storage.
I'm curious, though: I realize it's in the hot path, and I realize
that there's an accretive cost to even small features, but given we're
already paying the lock cost and updating memory in what is presumably
the same cache line, would you expect this cost to be clearly
measurable?
Thanks,
James Coleman
On Mon, Jan 17, 2022 at 4:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Jan-14, James Coleman wrote:
The logical slot can't flush past the
last commit, so even if there's 100s of megabytes of unflushed WAL on
the slot there may be zero lag (in terms of what's possible to
process).I've attached a simple patch (sans tests and documentation) to get
feedback early. After poking around this afternoon it seemed to me
that the simplest approach was to hook into the commit timestamps
infrastructure and store the commit's XLogRecPtr in the cache of the
most recent value (but of course don't write it out to disk).Maybe it would work to have a single LSN in shared memory, as an atomic
variable, which uses monotonic advance[1] to be updated. Whether this is
updated or not would depend on a new GUC, maybe track_latest_commit_lsn.
Causing performance pain during transaction commit is not great, but at
least this way it shouldn't be *too* a large hit.[1] part of a large patch at
/messages/by-id/202111222156.xmo2yji5ifi2@alvherre.pgsql
I'd be happy to make it a separate GUC, though it seems adding an
additional atomic access is worse (assuming we can convince ourselves
putting this into the commit timestamps infrastructure is acceptable)
given here we're already under a lock.
Thanks,
James Coleman
On 2022-Jan-17, Robert Haas wrote:
On Mon, Jan 17, 2022 at 4:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Maybe it would work to have a single LSN in shared memory, as an atomic
variable, which uses monotonic advance[1] to be updated. Whether this is
updated or not would depend on a new GUC, maybe track_latest_commit_lsn.
Causing performance pain during transaction commit is not great, but at
least this way it shouldn't be *too* a large hit.I don't know if it would or not, but it's such a hot path that I find
the idea a bit worrisome. Atomics aren't free - especially inside of a
loop.
I think the aspect to worry about the most is what happens when the
feature is disabled. The cost for that should be just one comparison,
which I think can be optimized by the compiler fairly well. That should
be cheap enough. People who enable it would have to pay the cost of the
atomics, which is of course much higher.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Mon, Jan 17, 2022 at 8:39 PM James Coleman <jtc331@gmail.com> wrote:
I wondered about that, but commit_ts already does more than commit
timestamps by recording the xid of the last commit.
Well, if you're maintaining an SLRU, you do kind of need to know where
the leading and lagging ends are.
For that matter, keeping a cache of last commit metadata in shared
memory is arguably not obviously implied by "track_commit_timestamps",
which leads to the below...
I suppose that's true in the strictest sense, but tracking information
does seem to imply having a way to look it up.
I'm curious, though: I realize it's in the hot path, and I realize
that there's an accretive cost to even small features, but given we're
already paying the lock cost and updating memory in what is presumably
the same cache line, would you expect this cost to be clearly
measurable?
If you'd asked me ten years ago, I would have said "no, can't matter,"
but Andres has subsequently demonstrated that a lot of things that I
thought were well-optimized were actually able to be optimized a lot
better than I thought possible, and some of them were in this area.
Still, I think it's unlikely that your patch would have a measurable
effect for the reasons that you state. Wouldn't hurt to test, though.
As far as performance goes, I'm more concerned about Alvaro's patch.
My concern with this one is more around whether it's too much of a
kludge.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Jan 18, 2022 at 9:25 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 17, 2022 at 8:39 PM James Coleman <jtc331@gmail.com> wrote:
I wondered about that, but commit_ts already does more than commit
timestamps by recording the xid of the last commit.Well, if you're maintaining an SLRU, you do kind of need to know where
the leading and lagging ends are.
As far as I can tell the data in commitTsShared is used purely as an
optimization for the path looking up the timestamp for an arbitrary
xid when that xid happens to be the most recent one so that we don't
have to look up in the SLRU for that specific case. Maybe I'm missing
something else you're seeing?
For that matter, keeping a cache of last commit metadata in shared
memory is arguably not obviously implied by "track_commit_timestamps",
which leads to the below...I suppose that's true in the strictest sense, but tracking information
does seem to imply having a way to look it up.
Looking up for an arbitrary commit, sure, (that's how I understand the
commit timestamps feature anyway) but it seems to me that the "most
recent' is distinct. Reading the code it seems the only usage (besides
the boolean activation status also stored there) is in
TransactionIdGetCommitTsData, and the only consumers of that in core
appear to be the SQL callable functions to get the latest commit info.
It is in commit_ts.h though, so I'm guessing someone is using this
externally (and maybe that's why the feature has the shape it does).
I'm curious, though: I realize it's in the hot path, and I realize
that there's an accretive cost to even small features, but given we're
already paying the lock cost and updating memory in what is presumably
the same cache line, would you expect this cost to be clearly
measurable?If you'd asked me ten years ago, I would have said "no, can't matter,"
but Andres has subsequently demonstrated that a lot of things that I
thought were well-optimized were actually able to be optimized a lot
better than I thought possible, and some of them were in this area.
Still, I think it's unlikely that your patch would have a measurable
effect for the reasons that you state. Wouldn't hurt to test, though.
If we get past your other main concern I'd be happy to spin something
up to prove that out.
As far as performance goes, I'm more concerned about Alvaro's patch.
My concern with this one is more around whether it's too much of a
kludge.
As far as the kludginess factor: do you think additional GUCs would
help clarify that? And/or are the earlier comments on the right path?
Thanks,
James Coleman
On Tue, Jan 18, 2022 at 9:47 AM James Coleman <jtc331@gmail.com> wrote:
Well, if you're maintaining an SLRU, you do kind of need to know where
the leading and lagging ends are.As far as I can tell the data in commitTsShared is used purely as an
optimization for the path looking up the timestamp for an arbitrary
xid when that xid happens to be the most recent one so that we don't
have to look up in the SLRU for that specific case. Maybe I'm missing
something else you're seeing?
I wasn't looking at the code, but that use also seems closer to the
purpose of committs than your proposal.
As far as performance goes, I'm more concerned about Alvaro's patch.
My concern with this one is more around whether it's too much of a
kludge.As far as the kludginess factor: do you think additional GUCs would
help clarify that? And/or are the earlier comments on the right path?
To be honest, I'm sort of keen to hear what other people think. I'm
shooting from the hip a little bit here...
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2022-Jan-17, James Coleman wrote:
I'd be happy to make it a separate GUC, though it seems adding an
additional atomic access is worse (assuming we can convince ourselves
putting this into the commit timestamps infrastructure is acceptable)
given here we're already under a lock.
I was thinking it'd not be under any locks ... and I don't think it
belongs under commit timestamps either.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)
On Tue, Jan 18, 2022 at 12:50 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Jan-17, James Coleman wrote:
I'd be happy to make it a separate GUC, though it seems adding an
additional atomic access is worse (assuming we can convince ourselves
putting this into the commit timestamps infrastructure is acceptable)
given here we're already under a lock.I was thinking it'd not be under any locks ... and I don't think it
belongs under commit timestamps either.
I'm not sure if you saw the other side of this thread with Robert, but
my argument is basically that the commit_ts infrastructure already
currently does more than just record commit timestamps for future use,
it also includes what looks to me like a more general "last commit
metadata" facility (which is not actually at all necessary to the
storing of commit timestamps). It might make sense to refactor this
somewhat so that that's more obvious, but I'd like to know if it looks
that way to you as well, and, if so, does that make it make more sense
to rely on the existing infrastructure rather than inventing a new
facility?
Thanks,
James Coleman
On 2022-Jan-18, James Coleman wrote:
Reading the code it seems the only usage (besides
the boolean activation status also stored there) is in
TransactionIdGetCommitTsData, and the only consumers of that in core
appear to be the SQL callable functions to get the latest commit info.
It is in commit_ts.h though, so I'm guessing someone is using this
externally (and maybe that's why the feature has the shape it does).
Logical replication is the intended consumer of that info, for the
purposes of conflict handling. I suppose pglogical uses it, but I don't
know that code myself.
[ ... greps ... ]
Yeah, that function is called from pglogical.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
On Tue, Jan 18, 2022 at 1:52 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Jan-18, James Coleman wrote:
Reading the code it seems the only usage (besides
the boolean activation status also stored there) is in
TransactionIdGetCommitTsData, and the only consumers of that in core
appear to be the SQL callable functions to get the latest commit info.
It is in commit_ts.h though, so I'm guessing someone is using this
externally (and maybe that's why the feature has the shape it does).Logical replication is the intended consumer of that info, for the
purposes of conflict handling. I suppose pglogical uses it, but I don't
know that code myself.[ ... greps ... ]
Yeah, that function is called from pglogical.
That's interesting, because my use case for the lsn is also logical
replication (monitoring).
James Coleman
Hi,
On 2022-01-17 18:34:16 -0300, Alvaro Herrera wrote:
Maybe it would work to have a single LSN in shared memory, as an atomic
variable, which uses monotonic advance[1] to be updated.
That could be a reasonable approach.
Whether this is updated or not would depend on a new GUC, maybe
track_latest_commit_lsn. Causing performance pain during transaction commit
is not great, but at least this way it shouldn't be *too* a large hit.
What kind of consistency are we expecting from this new bit of information?
Does it have to be perfectly aligned with visibility? If so, it'd need to
happen in ProcArrayEndTransaction(), with ProcArrayLock held - which I'd
consider a complete no-go, that's way too contended.
If it's "just" another piece of work happening "sometime around" transaction
commit, it'd be a bit less concerning.
I wonder if a very different approach could make sense here. Presumably this
wouldn't need to be queried at a very high frequency, right? If so, what about
storing the latest commit LSN for each backend in PGPROC? That could be
maintained without a lock/atomics, and should be just about free.
pg_last_committed_xact() then would have to iterate over all PGPROCs to
complete the LSN, but that's not too bad for an operation like that. We'd also
need to maintain a value for all disconnected backends, but that's also not a hot
path.
Greetings,
Andres Freund
On Tue, Jan 18, 2022 at 4:32 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2022-01-17 18:34:16 -0300, Alvaro Herrera wrote:
Maybe it would work to have a single LSN in shared memory, as an atomic
variable, which uses monotonic advance[1] to be updated.That could be a reasonable approach.
Whether this is updated or not would depend on a new GUC, maybe
track_latest_commit_lsn. Causing performance pain during transaction commit
is not great, but at least this way it shouldn't be *too* a large hit.What kind of consistency are we expecting from this new bit of information?
Does it have to be perfectly aligned with visibility? If so, it'd need to
happen in ProcArrayEndTransaction(), with ProcArrayLock held - which I'd
consider a complete no-go, that's way too contended.
My use case wouldn't require perfect alignment with visibility (I'm
not sure about the use case Alvaro mentioned in pglogical).
If it's "just" another piece of work happening "sometime around" transaction
commit, it'd be a bit less concerning.
That raises the interesting question of where the existing commit_ts
infrastructure and last commit caching falls into that range.
I wonder if a very different approach could make sense here. Presumably this
wouldn't need to be queried at a very high frequency, right? If so, what about
storing the latest commit LSN for each backend in PGPROC? That could be
maintained without a lock/atomics, and should be just about free.
pg_last_committed_xact() then would have to iterate over all PGPROCs to
complete the LSN, but that's not too bad for an operation like that. We'd also
need to maintain a value for all disconnected backends, but that's also not a hot
path.
I expect most monitoring setups default to around something like
checking anywhere from every single digit seconds to minutes.
If I read between the lines I imagine you'd see even e.g. every 2s as
not that big of a deal here, right?
Thanks,
James Coleman
On 2022-01-18 16:40:25 -0500, James Coleman wrote:
If I read between the lines I imagine you'd see even e.g. every 2s as
not that big of a deal here, right?
Right. Even every 0.2s wouldn't be a problem.
On Tue, Jan 18, 2022 at 4:32 PM Andres Freund <andres@anarazel.de> wrote:
I wonder if a very different approach could make sense here. Presumably this
wouldn't need to be queried at a very high frequency, right? If so, what about
storing the latest commit LSN for each backend in PGPROC? That could be
maintained without a lock/atomics, and should be just about free.
pg_last_committed_xact() then would have to iterate over all PGPROCs to
complete the LSN, but that's not too bad for an operation like that. We'd also
need to maintain a value for all disconnected backends, but that's also not a hot
path.
One other question on this: if we went with this would you expect a
new function to parallel pg_last_committed_xact()? Or allow the xid
and lsn in the return of pg_last_committed_xact() potentially not to
match (of course xid might also not be present if
track_commit_timestamps isn't on)? Or would you expect the current xid
and timestamp use the new infrastructure also?
Thanks,
James Coleman
Hi,
On 2022-01-18 18:31:42 -0500, James Coleman wrote:
One other question on this: if we went with this would you expect a
new function to parallel pg_last_committed_xact()?
I don't think I have an opinion the user interface aspect.
Or allow the xid and lsn in the return of pg_last_committed_xact()
potentially not to match (of course xid might also not be present if
track_commit_timestamps isn't on)? Or would you expect the current xid and
timestamp use the new infrastructure also?
When you say "current xid", what do you mean?
I think it might make sense to use the new approach for all of these.
Greetings,
Andres Freund
On Tue, Jan 18, 2022 at 8:05 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2022-01-18 18:31:42 -0500, James Coleman wrote:
One other question on this: if we went with this would you expect a
new function to parallel pg_last_committed_xact()?I don't think I have an opinion the user interface aspect.
Or allow the xid and lsn in the return of pg_last_committed_xact()
potentially not to match (of course xid might also not be present if
track_commit_timestamps isn't on)? Or would you expect the current xid and
timestamp use the new infrastructure also?When you say "current xid", what do you mean?
I mean the existing commitTsShared->xidLastCommit field which is
returned by pg_last_committed_xact().
I think it might make sense to use the new approach for all of these.
I think that would mean we could potentially remove commitTsShared,
but before doing so I'd like to know if that'd break existing
consumers.
Alvaro: You'd mentioned a use case in pglogical; if we moved the
xidLastCommit (and possibly even the cached last timestamp) out of
commit_ts.c (meaning it'd also no longer be under the commit ts lock)
would that be a problem for the current use (whether in lock safety or
in performance)?
Thanks,
James Coleman
