Обсуждение: Re: [COMMITTERS] pgsql: Make the visibility map crash-safe.
On 22.06.2011 06:05, Robert Haas wrote: > Second, when inserting, updating, or deleting > a tuple, we can no longer get away with clearing the visibility map > bit after releasing the lock on the corresponding heap page, because > an intervening crash might leave the visibility map bit set and the > page-level bit clear. heap_update seems to still do that. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, Jun 22, 2011 at 6:55 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 22.06.2011 06:05, Robert Haas wrote: >> >> Second, when inserting, updating, or deleting >> a tuple, we can no longer get away with clearing the visibility map >> bit after releasing the lock on the corresponding heap page, because >> an intervening crash might leave the visibility map bit set and the >> page-level bit clear. > > heap_update seems to still do that. Aw, crap. I'll take another look. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jun 22, 2011 at 9:12 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jun 22, 2011 at 6:55 AM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> On 22.06.2011 06:05, Robert Haas wrote: >>> >>> Second, when inserting, updating, or deleting >>> a tuple, we can no longer get away with clearing the visibility map >>> bit after releasing the lock on the corresponding heap page, because >>> an intervening crash might leave the visibility map bit set and the >>> page-level bit clear. >> >> heap_update seems to still do that. > > Aw, crap. I'll take another look. Well, it seems I didn't put nearly enough thought into heap_update(). The fix for the immediate problem looks simple enough - all the code has been refactored to use the new API, so the calls can be easily be moved into the critical section (see attached). But looking at this a little more, I see that heap_update() is many bricks short of a load, because there are several places where the buffer can be unlocked and relocked, and we don't recheck whether the page is all-visible after reacquiring the lock. So I've got some more work to do here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On Wed, Jun 22, 2011 at 10:23 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Well, it seems I didn't put nearly enough thought into heap_update(). > The fix for the immediate problem looks simple enough - all the code > has been refactored to use the new API, so the calls can be easily be > moved into the critical section (see attached). But looking at this a > little more, I see that heap_update() is many bricks short of a load, > because there are several places where the buffer can be unlocked and > relocked, and we don't recheck whether the page is all-visible after > reacquiring the lock. So I've got some more work to do here. See what you think of the attached. I *think* this covers all bases. It's a little more complicated than I would like, but I don't think fatally so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On Thu, Jun 23, 2011 at 9:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jun 22, 2011 at 10:23 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Well, it seems I didn't put nearly enough thought into heap_update(). >> The fix for the immediate problem looks simple enough - all the code >> has been refactored to use the new API, so the calls can be easily be >> moved into the critical section (see attached). But looking at this a >> little more, I see that heap_update() is many bricks short of a load, >> because there are several places where the buffer can be unlocked and >> relocked, and we don't recheck whether the page is all-visible after >> reacquiring the lock. So I've got some more work to do here. > > See what you think of the attached. I *think* this covers all bases. > It's a little more complicated than I would like, but I don't think > fatally so. For lack of comment, committed. It's hopefully at least better than what was there before, which was clearly several bricks short of a load. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/6/27 Robert Haas <robertmhaas@gmail.com>: > On Thu, Jun 23, 2011 at 9:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Jun 22, 2011 at 10:23 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> Well, it seems I didn't put nearly enough thought into heap_update(). >>> The fix for the immediate problem looks simple enough - all the code >>> has been refactored to use the new API, so the calls can be easily be >>> moved into the critical section (see attached). But looking at this a >>> little more, I see that heap_update() is many bricks short of a load, >>> because there are several places where the buffer can be unlocked and >>> relocked, and we don't recheck whether the page is all-visible after >>> reacquiring the lock. So I've got some more work to do here. >> >> See what you think of the attached. I *think* this covers all bases. >> It's a little more complicated than I would like, but I don't think >> fatally so. > > For lack of comment, committed. It's hopefully at least better than > what was there before, which was clearly several bricks short of a > load. out of curiosity, does it affect the previous benchmarks of the feature ? > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support
On Tue, Jun 28, 2011 at 11:44 AM, Cédric Villemain <cedric.villemain.debian@gmail.com> wrote: > out of curiosity, does it affect the previous benchmarks of the feature ? I don't think there's much performance impact, because the only case where we actually have to do any real work is when vacuum comes through and marks a buffer we're about to update all-visible. It would probably be good to run some tests to verify that, though. I'll try to set something up. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Jun 28, 2011 at 12:29 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jun 28, 2011 at 11:44 AM, Cédric Villemain > <cedric.villemain.debian@gmail.com> wrote: >> out of curiosity, does it affect the previous benchmarks of the feature ? > > I don't think there's much performance impact, because the only case > where we actually have to do any real work is when vacuum comes > through and marks a buffer we're about to update all-visible. It > would probably be good to run some tests to verify that, though. I'll > try to set something up. I did some pgbench runs on the 32-core box Nate Boley gave me access to. I'm not sure that pgbench is the best workload to test this with, but I gave it a shot. I used these settings: checkpoint_segments=64 shared_buffers=8GB synchronous_commit=off checkpoint_completion_target=0.9 I compare the performance of commit 431ab0e82819b31fcd1e33ecb52c2cd3b4b41da7 (post-patch) with commit 431ab0e82819b31fcd1e33ecb52c2cd3b4b41da7 (pre-patch). I tried 3 15-minute pgbench runs each with one client, eight clients, and thirty-two clients, on each branch. I used scale factor 100, reinitializing the entire cluster after each run. one client (prepatch) tps = 616.412009 (including connections establishing) tps = 619.189040 (including connections establishing) tps = 605.357710 (including connections establishing) one client (postpatch) tps = 583.295139 (including connections establishing) tps = 609.236975 (including connections establishing) tps = 597.674354 (including connections establishing) eight clients (prepatch) tps = 2635.459096 (including connections establishing) tps = 2468.973962 (including connections establishing) tps = 2592.889454 (including connections establishing) eight clients (postpatch) tps = 2602.226439 (including connections establishing) tps = 2644.201228 (including connections establishing) tps = 2725.760364 (including connections establishing) thirty-two clients (prepatch) tps = 3889.761627 (including connections establishing) tps = 4365.501093 (including connections establishing) tps = 3816.119328 (including connections establishing) thirty-two clients (postpatch) tps = 3888.620044 (including connections establishing) tps = 3614.252463 (including connections establishing) tps = 4090.430296 (including connections establishing) I think it's pretty difficult to draw any firm conclusions from this data. The one and thirty-two core results came out a bit slower; the eight core results came out a bit faster. But the variation between branches is less than the inter-run variation on the same branch, so if there is an effect, this test doesn't clearly capture it. Having thought about it a bit, I think that if we ran this test for long enough, we probably could measure a small effect. pgbench is a very write-intensive test, so anything that writes additional WAL records is going cause checkpoints to happen more frequently, and there's no denying that this change increases WAL volume slightly. On another note, these results are also interesting from a scalability perspective. The eight-core results are about 4.4x the one-core results, and the 32-client results are about 6.5x the one-core results. Needless to say, there's a lot of room for improvement there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, 2011-06-30 at 07:50 -0400, Robert Haas wrote: > I compare the performance of commit > 431ab0e82819b31fcd1e33ecb52c2cd3b4b41da7 (post-patch) with commit > 431ab0e82819b31fcd1e33ecb52c2cd3b4b41da7 (pre-patch). I believe that is a copy/paste error. Regards,Jeff Davis
On Thu, Jun 30, 2011 at 6:19 PM, Jeff Davis <pgsql@j-davis.com> wrote: > On Thu, 2011-06-30 at 07:50 -0400, Robert Haas wrote: >> I compare the performance of commit >> 431ab0e82819b31fcd1e33ecb52c2cd3b4b41da7 (post-patch) with commit >> 431ab0e82819b31fcd1e33ecb52c2cd3b4b41da7 (pre-patch). > > I believe that is a copy/paste error. Yeah, it sure is. I think (but am not 100% certain) that the post-patch version was actually 21f1e15aafb13ab2430e831a3da7d4d4f525d1ce. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company