Обсуждение: pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.
Allow Pin/UnpinBuffer to operate in a lockfree manner. Pinning/Unpinning a buffer is a very frequent operation; especially in read-mostly cache resident workloads. Benchmarking shows that in various scenarios the spinlock protecting a buffer header's state becomes a significant bottleneck. The problem can be reproduced with pgbench -S on larger machines, but can be considerably worse for queries which touch the same buffers over and over at a high frequency (e.g. nested loops over a small inner table). To allow atomic operations to be used, cram BufferDesc's flags, usage_count, buf_hdr_lock, refcount into a single 32bit atomic variable; that allows to manipulate them together using 32bit compare-and-swap operations. This requires reducing MAX_BACKENDS to 2^18-1 (which could be lifted by using a 64bit field, but it's not a realistic configuration atm). As not all operations can easily implemented in a lockfree manner, implement the previous buf_hdr_lock via a flag bit in the atomic variable. That way we can continue to lock the header in places where it's needed, but can get away without acquiring it in the more frequent hot-paths. There's some additional operations which can be done without the lock, but aren't in this patch; but the most important places are covered. As bufmgr.c now essentially re-implements spinlocks, abstract the delay logic from s_lock.c into something more generic. It now has already two users, and more are coming up; there's a follupw patch for lwlock.c at least. This patch is based on a proof-of-concept written by me, which Alexander Korotkov made into a fully working patch; the committed version is again revised by me. Benchmarking and testing has, amongst others, been provided by Dilip Kumar, Alexander Korotkov, Robert Haas. On a large x86 system improvements for readonly pgbench, with a high client count, of a factor of 8 have been observed. Author: Alexander Korotkov and Andres Freund Discussion: 2400449.GjM57CE0Yg@dinodell Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/48354581a49c30f5757c203415aa8412d85b0f70 Modified Files -------------- contrib/pg_buffercache/pg_buffercache_pages.c | 15 +- src/backend/storage/buffer/buf_init.c | 7 +- src/backend/storage/buffer/bufmgr.c | 508 +++++++++++++++++--------- src/backend/storage/buffer/freelist.c | 44 ++- src/backend/storage/buffer/localbuf.c | 64 ++-- src/backend/storage/lmgr/s_lock.c | 206 ++++++----- src/include/postmaster/postmaster.h | 15 +- src/include/storage/buf_internals.h | 101 +++-- src/include/storage/s_lock.h | 18 + src/tools/pgindent/typedefs.list | 1 + 10 files changed, 622 insertions(+), 357 deletions(-)
Andres Freund <andres@anarazel.de> writes: > Allow Pin/UnpinBuffer to operate in a lockfree manner. Now that I've had some occasion to look around in bufmgr.c, I am very unhappy that there are still boatloads of comments talking about a buffer header's spinlock, when there is in fact no spinlock anymore. Please expend some effort on making this less confusing for the next hacker. Maybe make those comments talk about a "lock bit" instead? regards, tom lane
Hi, On 2016-04-17 12:01:48 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Allow Pin/UnpinBuffer to operate in a lockfree manner. > > Now that I've had some occasion to look around in bufmgr.c, I am very > unhappy that there are still boatloads of comments talking about a buffer > header's spinlock, when there is in fact no spinlock anymore. That's actually intentional. Alexander had changed those, and it made the patch a good bit harder to read because there's so many references. As the new thing is still a spinlock, just not a s_lock.[ch] style on, I don't see changing all that to be a significant benefit. - Andres
Andres Freund <andres@anarazel.de> writes: > On 2016-04-17 12:01:48 -0400, Tom Lane wrote: >> Now that I've had some occasion to look around in bufmgr.c, I am very >> unhappy that there are still boatloads of comments talking about a buffer >> header's spinlock, when there is in fact no spinlock anymore. > That's actually intentional. Alexander had changed those, and it made > the patch a good bit harder to read because there's so many references. > As the new thing is still a spinlock, just not a s_lock.[ch] style on, I > don't see changing all that to be a significant benefit. Well, I disagree: we consistently use "spinlock" to mean the s_lock.h mechanism. Lock mechanisms are important, so using the same terminology to refer to different implementations is not helpful. Especially when it means you have no way to talk about one implementation as opposed to the other. Imagine writing a comment that says "back when we used to use spinlocks for this, we had to do FOO. Now that we use spinlocks, we can do BAR instead." regards, tom lane
On Sun, Apr 17, 2016 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andres Freund <andres@anarazel.de> writes: >> Allow Pin/UnpinBuffer to operate in a lockfree manner. > > Now that I've had some occasion to look around in bufmgr.c, I am very > unhappy that there are still boatloads of comments talking about a buffer > header's spinlock, when there is in fact no spinlock anymore. Please > expend some effort on making this less confusing for the next hacker. > Maybe make those comments talk about a "lock bit" instead? I was actually going to complain about this, too. I noticed it over the weekend when noodling around with another patch. I'm not sure exactly how it should be revised, but I find the current state of things confusing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Apr 18, 2016 at 2:34 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Apr 17, 2016 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Allow Pin/UnpinBuffer to operate in a lockfree manner.
>
> Now that I've had some occasion to look around in bufmgr.c, I am very
> unhappy that there are still boatloads of comments talking about a buffer
> header's spinlock, when there is in fact no spinlock anymore. Please
> expend some effort on making this less confusing for the next hacker.
> Maybe make those comments talk about a "lock bit" instead?
I was actually going to complain about this, too. I noticed it over
the weekend when noodling around with another patch. I'm not sure
exactly how it should be revised, but I find the current state of
things confusing.
+1
Do we have consensus on renaming "buffer header spinlock" to "buffer header lock bit"?If so, I can provide a patch for it.
The Russian Postgres Company
On 2016-04-18 17:12:32 +0300, Alexander Korotkov wrote: > On Mon, Apr 18, 2016 at 2:34 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > > On Sun, Apr 17, 2016 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Andres Freund <andres@anarazel.de> writes: > > >> Allow Pin/UnpinBuffer to operate in a lockfree manner. > > > > > > Now that I've had some occasion to look around in bufmgr.c, I am very > > > unhappy that there are still boatloads of comments talking about a buffer > > > header's spinlock, when there is in fact no spinlock anymore. Please > > > expend some effort on making this less confusing for the next hacker. > > > Maybe make those comments talk about a "lock bit" instead? > > > > I was actually going to complain about this, too. I noticed it over > > the weekend when noodling around with another patch. I'm not sure > > exactly how it should be revised, but I find the current state of > > things confusing. > > > > +1 > Do we have consensus on renaming "buffer header spinlock" to "buffer header > lock bit"? Personally I think the "spin" part is actually quite relevant, and I think we shouldn't loose it. It describes concurrency and blocking behaviour, and how errors need to be handled (i.e. there may not be any). Greetings, Andres Freund
On 2016-04-17 13:20:29 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2016-04-17 12:01:48 -0400, Tom Lane wrote: > >> Now that I've had some occasion to look around in bufmgr.c, I am very > >> unhappy that there are still boatloads of comments talking about a buffer > >> header's spinlock, when there is in fact no spinlock anymore. > > > That's actually intentional. Alexander had changed those, and it made > > the patch a good bit harder to read because there's so many references. > > As the new thing is still a spinlock, just not a s_lock.[ch] style on, I > > don't see changing all that to be a significant benefit. > > Well, I disagree: we consistently use "spinlock" to mean the s_lock.h > mechanism. Lock mechanisms are important, so using the same terminology > to refer to different implementations is not helpful. Especially when > it means you have no way to talk about one implementation as opposed > to the other. "s_lock.h" style spinlock vs. "BufferDesc type spinlock" etc. seems to work. Given where vertical scalability is going (more cores, more numa partitions (two in one socket now!), ...), it seems quite likely that we'll end up with further special case implementations. I don't think we'll be served well naming them differently if they have very similar rules to s_lock.h style spinlocks. > Imagine writing a comment that says "back when we used > to use spinlocks for this, we had to do FOO. Now that we use spinlocks, > we can do BAR instead." "Back when buffer locking used a s_lock.h style spinlock, ..., but now that it's just a flag in the BufferDesc's state, ...".. Doesn't seem like a problem. Greetings, Andres Freund
On Mon, Apr 18, 2016 at 10:27 AM, Andres Freund <andres@anarazel.de> wrote: > On 2016-04-18 17:12:32 +0300, Alexander Korotkov wrote: >> On Mon, Apr 18, 2016 at 2:34 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> > On Sun, Apr 17, 2016 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > > Andres Freund <andres@anarazel.de> writes: >> > >> Allow Pin/UnpinBuffer to operate in a lockfree manner. >> > > >> > > Now that I've had some occasion to look around in bufmgr.c, I am very >> > > unhappy that there are still boatloads of comments talking about a buffer >> > > header's spinlock, when there is in fact no spinlock anymore. Please >> > > expend some effort on making this less confusing for the next hacker. >> > > Maybe make those comments talk about a "lock bit" instead? >> > >> > I was actually going to complain about this, too. I noticed it over >> > the weekend when noodling around with another patch. I'm not sure >> > exactly how it should be revised, but I find the current state of >> > things confusing. >> > >> >> +1 >> Do we have consensus on renaming "buffer header spinlock" to "buffer header >> lock bit"? > > Personally I think the "spin" part is actually quite relevant, and I > think we shouldn't loose it. It describes concurrency and blocking > behaviour, and how errors need to be handled (i.e. there may not be > any). IMHO, "buffer header lock bit" is plenty clear enough. We could say "buffer header spin lock bit" but I think that's too many words and not actually more clear. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Apr 18, 2016 at 10:27 AM, Andres Freund <andres@anarazel.de> wrote: >> Personally I think the "spin" part is actually quite relevant, and I >> think we shouldn't loose it. It describes concurrency and blocking >> behaviour, and how errors need to be handled (i.e. there may not be >> any). > IMHO, "buffer header lock bit" is plenty clear enough. We could say > "buffer header spin lock bit" but I think that's too many words and > not actually more clear. I agree. If it's just a bit, it pretty much has to be a spin lock, because there's no way for it to contain any infrastructure that would support anything else. In any case, you could and should document the exact semantics in a comment associated with the declaration of that bit field. It's not really necessary to repeat them in every reference, so long as the references use a unique name that won't be confused with other possible locking mechanisms. (From that standpoint, "header lock bit" might be actively better than anything including the phrase "spin lock", since it reduces the chance of confusion with s_lock.h.) regards, tom lane