Обсуждение: 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
Вложения
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] 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 https://www.postgresql.org/message-id/202111222156.xmo2yji5ifi2%40alvherre.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 > https://www.postgresql.org/message-id/202111222156.xmo2yji5ifi2%40alvherre.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
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. Is something roughly like the attached what you'd envisioned? I wouldn't expect the final implementation to be in commit_ts.c, but I left it there for expediency's sake in demonstrating the idea since pg_last_committed_xact() currently finds its home there. I think we need a shared ProcArrayLock to read the array, correct? We also need to do the global updating under lock, but given it's when a proc is removed, that shouldn't be a performance issue if I'm following what you are saying. Thanks, James Coleman
Вложения
Hi,
On 2022-01-18 20:58:01 -0500, James Coleman wrote:
> Is something roughly like the attached what you'd envisioned?
Roughly, yea.
> I think we need a shared ProcArrayLock to read the array, correct?
You could perhaps get away without it, but it'd come at the price of needing
to look at all procs, rather than the connected procs. And I don't think it's
needed.
> We also need to do the global updating under lock, but given it's when a
> proc is removed, that shouldn't be a performance issue if I'm following what
> you are saying.
Yup.
> + LWLockAcquire(ProcArrayLock, LW_SHARED);
> + lsn = ShmemVariableCache->finishedProcsLastCommitLSN;
> + for (index = 0; index < ProcGlobal->allProcCount; index++)
> + {
> + XLogRecPtr procLSN = ProcGlobal->allProcs[index].lastCommitLSN;
> + if (procLSN > lsn)
> + lsn = procLSN;
> + }
> + LWLockRelease(ProcArrayLock);
I think it'd be better to go through the pgprocnos infrastructure, so that
only connected procs need to be checked.
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (i = 0; i < arrayP->numProcs; i++)
{
int pgprocno = arrayP->pgprocnos[i];
PGPROC *proc = &allProcs[pgprocno];
if (proc->lastCommitLSN > lsn)
lsn =proc->lastCommitLSN;
}
> diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
> index a58888f9e9..2a026b0844 100644
> --- a/src/include/storage/proc.h
> +++ b/src/include/storage/proc.h
> @@ -258,6 +258,11 @@ struct PGPROC
> PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
> dlist_head lockGroupMembers; /* list of members, if I'm a leader */
> dlist_node lockGroupLink; /* my member link, if I'm a member */
> +
> + /*
> + * Last transaction metadata.
> + */
> + XLogRecPtr lastCommitLSN; /* cache of last committed LSN */
> };
We do not rely on 64bit integers to be read/written atomically, just 32bit
ones. To make this work for older platforms you'd have to use a
pg_atomic_uint64. On new-ish platforms pg_atomic_read_u64/pg_atomic_write_u64
end up as plain read/writes, but on older ones they'd do the necessarily
locking to make that safe...
Greetings,
Andres Freund
On Tue, Jan 18, 2022 at 9:19 PM Andres Freund <andres@anarazel.de> wrote:
>
> > + LWLockAcquire(ProcArrayLock, LW_SHARED);
> > + lsn = ShmemVariableCache->finishedProcsLastCommitLSN;
> > + for (index = 0; index < ProcGlobal->allProcCount; index++)
> > + {
> > + XLogRecPtr procLSN = ProcGlobal->allProcs[index].lastCommitLSN;
> > + if (procLSN > lsn)
> > + lsn = procLSN;
> > + }
> > + LWLockRelease(ProcArrayLock);
>
> I think it'd be better to go through the pgprocnos infrastructure, so that
> only connected procs need to be checked.
>
> LWLockAcquire(ProcArrayLock, LW_SHARED);
> for (i = 0; i < arrayP->numProcs; i++)
> {
> int pgprocno = arrayP->pgprocnos[i];
> PGPROC *proc = &allProcs[pgprocno];
>
> if (proc->lastCommitLSN > lsn)
> lsn =proc->lastCommitLSN;
> }
>
>
> > diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
> > index a58888f9e9..2a026b0844 100644
> > --- a/src/include/storage/proc.h
> > +++ b/src/include/storage/proc.h
> > @@ -258,6 +258,11 @@ struct PGPROC
> > PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
> > dlist_head lockGroupMembers; /* list of members, if I'm a leader */
> > dlist_node lockGroupLink; /* my member link, if I'm a member */
> > +
> > + /*
> > + * Last transaction metadata.
> > + */
> > + XLogRecPtr lastCommitLSN; /* cache of last committed LSN */
> > };
>
> We do not rely on 64bit integers to be read/written atomically, just 32bit
> ones. To make this work for older platforms you'd have to use a
> pg_atomic_uint64. On new-ish platforms pg_atomic_read_u64/pg_atomic_write_u64
> end up as plain read/writes, but on older ones they'd do the necessarily
> locking to make that safe...
All right, here's an updated patch.
The final interface (new function or refactor the existing not to rely
on commit_ts) is still TBD (and I'd appreciate input on that from
Alvaro and others).
Thanks,
James Coleman
Вложения
Hi,
On 2022-01-19 21:23:12 -0500, James Coleman wrote:
> { oid => '3537', descr => 'get identification of SQL object',
> diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
> index a58888f9e9..2a026b0844 100644
> --- a/src/include/storage/proc.h
> +++ b/src/include/storage/proc.h
> @@ -258,6 +258,11 @@ struct PGPROC
> PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
> dlist_head lockGroupMembers; /* list of members, if I'm a leader */
> dlist_node lockGroupLink; /* my member link, if I'm a member */
> +
> + /*
> + * Last transaction metadata.
> + */
> + XLogRecPtr lastCommitLSN; /* cache of last committed LSN */
> };
Might be worth forcing this to be on a separate cacheline than stuff more
hotly accessed by other backends, like the lock group stuff.
Greetings,
Andres Freund
On Wed, Jan 19, 2022 at 10:12 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-01-19 21:23:12 -0500, James Coleman wrote:
> > { oid => '3537', descr => 'get identification of SQL object',
> > diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
> > index a58888f9e9..2a026b0844 100644
> > --- a/src/include/storage/proc.h
> > +++ b/src/include/storage/proc.h
> > @@ -258,6 +258,11 @@ struct PGPROC
> > PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
> > dlist_head lockGroupMembers; /* list of members, if I'm a leader */
> > dlist_node lockGroupLink; /* my member link, if I'm a member */
> > +
> > + /*
> > + * Last transaction metadata.
> > + */
> > + XLogRecPtr lastCommitLSN; /* cache of last committed LSN */
> > };
>
> Might be worth forcing this to be on a separate cacheline than stuff more
> hotly accessed by other backends, like the lock group stuff.
What's the best way to do that? I'm poking around and don't see any
obvious cases of doing that in a struct definition. I could add a
char* of size PG_CACHE_LINE_SIZE, but that seems unnecessarily
wasteful, and the other ALIGN macros seem mostly used in situations
where we're allocating memory. Is it possible in C to get the size of
the struct so far to be able to subtract from PG_CACHE_LINE_SIZE?
Maybe there's some other approach I'm missing...
Thanks,
James Coleman
On Thu, Jan 20, 2022 at 8:15 AM James Coleman <jtc331@gmail.com> wrote:
>
> On Wed, Jan 19, 2022 at 10:12 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2022-01-19 21:23:12 -0500, James Coleman wrote:
> > > { oid => '3537', descr => 'get identification of SQL object',
> > > diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
> > > index a58888f9e9..2a026b0844 100644
> > > --- a/src/include/storage/proc.h
> > > +++ b/src/include/storage/proc.h
> > > @@ -258,6 +258,11 @@ struct PGPROC
> > > PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
> > > dlist_head lockGroupMembers; /* list of members, if I'm a leader */
> > > dlist_node lockGroupLink; /* my member link, if I'm a member */
> > > +
> > > + /*
> > > + * Last transaction metadata.
> > > + */
> > > + XLogRecPtr lastCommitLSN; /* cache of last committed LSN */
> > > };
> >
> > Might be worth forcing this to be on a separate cacheline than stuff more
> > hotly accessed by other backends, like the lock group stuff.
>
> What's the best way to do that? I'm poking around and don't see any
> obvious cases of doing that in a struct definition. I could add a
> char* of size PG_CACHE_LINE_SIZE, but that seems unnecessarily
> wasteful, and the other ALIGN macros seem mostly used in situations
> where we're allocating memory. Is it possible in C to get the size of
> the struct so far to be able to subtract from PG_CACHE_LINE_SIZE?
> Maybe there's some other approach I'm missing...
Looking at this again it seems like there are two ways to do this I see so far:
First would be to have a container struct and two structs inside --
something like one struct for local process access and one for shared
process access. But that seems like it'd likely end up pretty messy in
terms of how much it'd affect other parts of the code, so I'm hesitant
to go down that path.
Alternatively I see pg_attribute_aligned, but that's not defined
(AFAICT) on clang, for example, so I'm not sure that'd be acceptable?
It doesn't seem to me that there's anything like CACHELINEALIGN that
would work in this context (in a struct definition) since that appears
to be designed to work with allocated memory.
Is there an approach I'm missing? Or does one of these seem reasonable?
Thanks,
James Coleman
Hi, On 2022-01-28 18:43:57 -0500, James Coleman wrote: > Alternatively I see pg_attribute_aligned, but that's not defined > (AFAICT) on clang, for example, so I'm not sure that'd be acceptable? clang should have it (it defines __GNUC__). The problem would be msvc, I think. Not sure if there's a way to get to a common way of defining it between gcc-like compilers and msvc (the rest is niche enough that we don't need to care about the efficiency I think). > Is there an approach I'm missing? Or does one of these seem reasonable? I'd probably just slap a char *pad[PG_CACHELINE_SIZE] in there if the above can't be made work. Greetings, Andres Freund
On 2022-01-28 16:36:32 -0800, Andres Freund wrote: > On 2022-01-28 18:43:57 -0500, James Coleman wrote: > > Alternatively I see pg_attribute_aligned, but that's not defined > > (AFAICT) on clang, for example, so I'm not sure that'd be acceptable? > > clang should have it (it defines __GNUC__). The problem would be msvc, I > think. Not sure if there's a way to get to a common way of defining it between > gcc-like compilers and msvc (the rest is niche enough that we don't need to > care about the efficiency I think). Seems like it's doable: https://godbolt.org/z/3c5573bTW
On Fri, Jan 28, 2022 at 7:47 PM Andres Freund <andres@anarazel.de> wrote: > > On 2022-01-28 16:36:32 -0800, Andres Freund wrote: > > On 2022-01-28 18:43:57 -0500, James Coleman wrote: > > > Alternatively I see pg_attribute_aligned, but that's not defined > > > (AFAICT) on clang, for example, so I'm not sure that'd be acceptable? > > > > clang should have it (it defines __GNUC__). The problem would be msvc, I > > think. Not sure if there's a way to get to a common way of defining it between > > gcc-like compilers and msvc (the rest is niche enough that we don't need to > > care about the efficiency I think). > > Seems like it's doable: > > https://godbolt.org/z/3c5573bTW Oh, thanks. I'd seen some discussion previously on the list about clang not supporting it, but that seems to have been incorrect. Also I didn't know about that compiler site -- that's really neat. Here's an updated patch series using that approach; the first patch can (and probably should be) committed separately/regardless to update the pg_attribute_aligned to be used in MSVC. Thanks, James Coleman
Вложения
On Sat, Jan 29, 2022 at 02:51:32PM -0500, James Coleman wrote: > Oh, thanks. I'd seen some discussion previously on the list about > clang not supporting it, but that seems to have been incorrect. Also I > didn't know about that compiler site -- that's really neat. > > Here's an updated patch series using that approach; the first patch > can (and probably should be) committed separately/regardless to update > the pg_attribute_aligned to be used in MSVC. I don't have much an opinion on 0002, except that I am worried about more data added to PGPROC that make it larger. Now, 0001 looks like a hidden gem. Based on the upstream docs, it looks that using __declspec(align(a)) is right: https://docs.microsoft.com/en-us/cpp/cpp/align-cpp?view=msvc-170 Is __declspec available in Visual Studio 2013? I can see it in the upstream docs for 2015, but I am not sure about 2013. > /* This must match the corresponding code in c.h: */ > #if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__) > #define pg_attribute_aligned(a) __attribute__((aligned(a))) > +#elif defined(_MSC_VER) > +#define pg_attribute_aligned(a) __declspec(align(a)) > #endif > typedef __int128 int128a This change in ./configure looks incorrect to me. Shouldn't the change happen in c-compiler.m4 instead? > +#if defined(_MSC_VER) > +#define pg_attribute_aligned(a) __declspec(align(a)) > +#endif This way of doing things is inconsistent with the surroundings. I think that you should have an #elif for _MSC_VER to keep all the definitions of pg_attribute_aligned(9 & friends in the same block. This makes me wonder whether we would should introduce noreturn, as of: https://docs.microsoft.com/en-us/cpp/c-language/noreturn?view=msvc-140 -- Michael
Вложения
This entry has been waiting on author input for a while (our current
threshold is roughly two weeks), so I've marked it Returned with
Feedback.
Once you think the patchset is ready for review again, you (or any
interested party) can resurrect the patch entry by visiting
https://commitfest.postgresql.org/38/3515/
and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)
Thanks,
--Jacob