Обсуждение: Moving test_fsync to /contrib?
Is there value in moving test_fsync to /contrib? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: > Is there value in moving test_fsync to /contrib? Why would we want to do that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: > > Is there value in moving test_fsync to /contrib? > > Why would we want to do that? If we expect users to run the tool to best choose the best wal_sync_method. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Mon, Jan 17, 2011 at 11:16 AM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: >> > Is there value in moving test_fsync to /contrib? >> >> Why would we want to do that? > > If we expect users to run the tool to best choose the best > wal_sync_method. I don't see how moving it from src/tools to contrib is going to make that happen more often. src/tools to src/bin might have that effect. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: >> Is there value in moving test_fsync to /contrib? > Why would we want to do that? So it would be built by default, installed under reasonable conditions, and there would be a place to document it. Where it is, it's not a user-facing thing at all. regards, tom lane
Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> Is there value in moving test_fsync to /contrib? > > > Why would we want to do that? > > So it would be built by default, installed under reasonable conditions, > and there would be a place to document it. Where it is, it's not a > user-facing thing at all. I have cleaned up the code so it is reasonable to ship and use by end-users. It is documented already where we mention setting wal_sync_method, but having it in src/tools really is a hinderance. It seems like /contrib would be more natural, no? /bin seems like overkill because most people will not want to run it. Most of /contrib is installed already by installers, I think. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce@momjian.us> wrote: > Tom Lane wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >> > On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: >> >> Is there value in moving test_fsync to /contrib? >> >> > Why would we want to do that? >> >> So it would be built by default, installed under reasonable conditions, >> and there would be a place to document it. Where it is, it's not a >> user-facing thing at all. > > I have cleaned up the code so it is reasonable to ship and use by > end-users. It is documented already where we mention setting > wal_sync_method, but having it in src/tools really is a hinderance. > > It seems like /contrib would be more natural, no? /bin seems like > overkill because most people will not want to run it. Most of /contrib > is installed already by installers, I think. At least on Red Hat, it is packaged separately. So if you install postgresql-server and postgresql-client you will not get things that are only in contrib. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Bruce Momjian's message of lun ene 17 13:47:40 -0300 2011: > It seems like /contrib would be more natural, no? /bin seems like > overkill because most people will not want to run it. Most of /contrib > is installed already by installers, I think. I don't understand why it would be "overkill". Are you saying people would complain because you installed a 25 kB executable that they might not want to use? Just for fun I checked /usr/bin and noticed that I have a "pandoc" executable, weighing 17 MB, that I have never used and I have no idea what is it for. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
2011/1/17 Bruce Momjian <bruce@momjian.us>: > Robert Haas wrote: >> On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce@momjian.us> wrote: >> > Is there value in moving test_fsync to /contrib? >> >> Why would we want to do that? > > If we expect users to run the tool to best choose the best > wal_sync_method. +1 to move it to contrib/ > > -- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://enterprisedb.com > > + It's impossible for everything to be true. + > > -- > 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
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce@momjian.us> wrote: >> It seems like /contrib would be more natural, no? �/bin seems like >> overkill because most people will not want to run it. �Most of /contrib >> is installed already by installers, I think. > At least on Red Hat, it is packaged separately. On Red Hat, it is not packaged at all (at least not by me), and won't be unless it goes into contrib. I don't believe it belongs in the base package. Also, it's not going to get packaged at all unless it gets renamed to something less generic, maybe pg_test_fsync; I'm not going to risk the oppobrium of sticking something named "test_fsync" into /usr/bin. Moving to contrib would be a good opportunity to fix the name. regards, tom lane
On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce@momjian.us> wrote: >>> It seems like /contrib would be more natural, no? /bin seems like >>> overkill because most people will not want to run it. Most of /contrib >>> is installed already by installers, I think. > >> At least on Red Hat, it is packaged separately. > > On Red Hat, it is not packaged at all (at least not by me), and won't > be unless it goes into contrib. I don't believe it belongs in the > base package. I confess to some confusion about what things "belong" where. Is contrib the right place for this because we think it's half-baked, or because we think most people won't use it, or just because we're violently allergic to adding stuff to src/bin, or what? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> It seems like /contrib would be more natural, no? �/bin seems like > >> overkill because most people will not want to run it. �Most of /contrib > >> is installed already by installers, I think. > > > At least on Red Hat, it is packaged separately. > > On Red Hat, it is not packaged at all (at least not by me), and won't > be unless it goes into contrib. I don't believe it belongs in the > base package. > > Also, it's not going to get packaged at all unless it gets renamed to > something less generic, maybe pg_test_fsync; I'm not going to risk the > oppobrium of sticking something named "test_fsync" into /usr/bin. > Moving to contrib would be a good opportunity to fix the name. Agreed on the need for a name change. /contrib or /bin are fine with me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Robert Haas wrote: > On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce@momjian.us> wrote: > >>> It seems like /contrib would be more natural, no? ?/bin seems like > >>> overkill because most people will not want to run it. ?Most of /contrib > >>> is installed already by installers, I think. > > > >> At least on Red Hat, it is packaged separately. > > > > On Red Hat, it is not packaged at all (at least not by me), and won't > > be unless it goes into contrib. ?I don't believe it belongs in the > > base package. > > I confess to some confusion about what things "belong" where. Is > contrib the right place for this because we think it's half-baked, or > because we think most people won't use it, or just because we're > violently allergic to adding stuff to src/bin, or what? I was suggesting /contrib because it seems to be of limited usefulness. I assume people want pg_upgrade to stay in /contrib for the same reason. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> On Red Hat, it is not packaged at all (at least not by me), and won't >> be unless it goes into contrib. I don't believe it belongs in the >> base package. > I confess to some confusion about what things "belong" where. Is > contrib the right place for this because we think it's half-baked, or > because we think most people won't use it, or just because we're > violently allergic to adding stuff to src/bin, or what? The first two, if you ask me. And there's another point: I disagree with the assumption that platform-specific packagings will or should include test_fsync by default. It'd be better for the packager to make a platform-specific choice of default for the users. I don't mind too much putting it into a secondary subpackage such as postgresql-contrib, but you won't be seeing it in postgresql-server. regards, tom lane
Bruce Momjian <bruce@momjian.us> writes: > Robert Haas wrote: >> I confess to some confusion about what things "belong" where. > I was suggesting /contrib because it seems to be of limited usefulness. > I assume people want pg_upgrade to stay in /contrib for the same reason. pg_upgrade is a different issue, really. I think it's in contrib because we don't trust it fully and don't want to promise that it will work in every single future release anyway. But even if we moved it to core, it will always be a special case for packagers: not only is it not appropriate to put it in the base package, it's not useful to package it at all unless you also provide a copy of a back-rev postmaster. So at least for my money it will always be part of its own special subpackage postgresql-upgrade. (BTW, I just recently got round to making that work for Fedora.) regards, tom lane
On Mon, Jan 17, 2011 at 12:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> On Red Hat, it is not packaged at all (at least not by me), and won't >>> be unless it goes into contrib. I don't believe it belongs in the >>> base package. > >> I confess to some confusion about what things "belong" where. Is >> contrib the right place for this because we think it's half-baked, or >> because we think most people won't use it, or just because we're >> violently allergic to adding stuff to src/bin, or what? > > The first two, if you ask me. And there's another point: I disagree > with the assumption that platform-specific packagings will or should > include test_fsync by default. It'd be better for the packager to make > a platform-specific choice of default for the users. I don't mind too > much putting it into a secondary subpackage such as postgresql-contrib, > but you won't be seeing it in postgresql-server. I see. Well, that seems reasonable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I have been taking a peek at the following commitfest item: https://commitfest.postgresql.org/action/patch_view?id=497 Submission: - I had to trim a little off the end of the patch to apply it, but that's likely the fault of how I cut'n'pasted it. It appliedcleanly against HEAD. - I observe that it doesn't include any alterations to documentation or to regression tests. Both aspects seem apropos, as the behaviour is entirely internal to the backend. I wouldn't expect a GUC variable for this, or SQL commands to control it. Usability Review: Does the patch actually implement that? - Establishes a hash table - Establishes skip slot array - Walks through all BGWriter requests - Adds to hash table. (I observe that it wasn't all that obvious that "hash_search()" *adds* elements that are missing. I got confusedand went looking for "hash_add() or similar. It's permissible to say "dumb Chris".) - If it's a collision, then mark collision in skip slot array, and add to count - After the walk - Clean up hash table - If nothing found, clean up skip slot array, and return - If collisions found, then clear them out. Question: Is there any further cleanup needed for the entries that got "dropped" out of BGWriterShmem->requests? It seems not, but a leak seems conceivable. Do we want that? Eliminating a bunch of fsync() calls that are already being induced by other backends seems like a good thing, yep. Do we already have it? Evidently not! Does it follow SQL spec, or the community-agreed behavior? That doesn't seem relevant; this is well outside the scope of what SQL spec should have to say. Does it include pg_dump support (if applicable)? Definitely not applicable. Are there dangers? Possibilities... - Mentioned in the patch is the possibility of processing the set of requests in reverse order, which might inprinciple reduce work. But there is some danger of this changing semantics, so that reversal is not done. - Concurrent access... Is there anything that can write extra elements to BGWriterShmem->requests while this is running? I wonder if we need to have any sort of lock surrounding this? Have all the bases been covered? It is a comparatively simple change, so I wouldn't think things are missing. Feature test: - Compiled and ran regression test; no problems found. Need to do... - Validate it works as advertised - Hook up pgbench - Turn on DEBUG1 level - Watch that "compactedfsync request queue from %d entries to %d entries" come up It was a little troublesome inducing it. I did so by cutting shared memory to minimum (128kB). I'd regularly get entries like the following: (Note that I changed the error level to WARNING to induce loggingthis without getting all sorts of other stuff). CONTEXT: writing block 1735 of relation base/11933/16396 WARNING: compacted fsync request queue from 16 entries to 3 entries - lost [13] entries CONTEXT: writing block 14 of relation base/11933/16387 WARNING: compacted fsync request queue from 16 entries to 3 entries - lost [13] entries CONTEXT: writing block 4 of relation base/11933/16387 WARNING: compacted fsync request queue from 16 entries to 3 entries - lost [13] entries CONTEXT: writing block 6 of relation base/11933/16387 WARNING: compacted fsync request queue from 16 entries to 3 entries - lost [13] entries CONTEXT: writing block 1625 of relation base/11933/16396 WARNING: compacted fsync request queue from 16 entries to 4 entries - lost [12] entries CONTEXT: writing block 880 of relation base/11933/16396 WARNING: compacted fsync request queue from 16 entries to 4 entries - lost [12] entries CONTEXT: writing block 133 of relation base/11933/16396 With higher shared memory, I couldn't readily induce compaction, which is probably a concurrency matter of not havingenough volume of concurrent work going on. - Corner cases? It's already a corner case ;-). - Assertion failures? None seen thus far. Performance test - Does it slow down simple cases? It surely shouldn't; compaction is only considered if the fsync queue is larger than the number of shared buffers. That doesn't seem like a "simple case" to me! - Does it improve performance? I haven't been able to induce it at a level that would make the improvement visible. But a database that is busyenough to have a 'full' fsync queue should surely be helped by reducing the number of fsync requests. - Does it slow down other things? In principle, the only case where it should worsen performance is if the amount of time required to: - Setup a hash table - Insert an entry for each buffer - Walk the skip_slot array, shortening the request queue for each duplicate found exceeded the amount of time required to do the duplicate fsync() requests. That cost should be mighty low. It would be interesting to instrument CompactBgwriterRequestQueue() to see howlong it runs. But note that this cost is also spread into a direction where it likely wouldn't matter, as it is typically invokedby the background writer process, so this would frequently not be paid by "on-line" active processes. Coding review- guidelines- portability, Windows/BSD I can't speak to Windows' handling of fsync(), but observe that the existing code works there, so it seems unlikelythat a shortening of data that presently works would cease to work. - Sufficient comments? Seems so.- Does it do what it says, accurately? I think so.- Compiler warnings? bgwriter.c: In function 'CompactBgwriterRequestQueue': bgwriter.c:1142: warning: 'num_skipped' may be used uninitialized in this function - Can I induce a crash? By making changes to the code that corrupt things ;-). Architecture- coherent with other things? Generally a minor change, which is colocated with other background writer request code. Seems fine in that regard. - problematic interdependencies? Surely seems unlikely. Tentative Conclusions:- Seems pretty good so far.- I'd like to see the compiler warning fixed; should be as simple as assigningnum_skipped before it is used. -- let name="cbbrowne" and tld="gmail.com" in name ^ "@" ^ tld;; http://linuxfinances.info/info/spreadsheets.html I am not a number! I am a free man!
On Mon, Jan 17, 2011 at 1:43 PM, Chris Browne <cbbrowne@acm.org> wrote: > (I observe that it wasn't all that obvious that "hash_search()" > *adds* elements that are missing. I got confused and went > looking for "hash_add() or similar. It's permissible to say "dumb > Chris".) I didn't invent that API. It is a little strange, but you get the hang of it. > Question: Is there any further cleanup needed for the entries > that got "dropped" out of BGWriterShmem->requests? It seems > not, but a leak seems conceivable. They're just holding integers, so what's to clean up? > - Concurrent access... > > Is there anything that can write extra elements to > BGWriterShmem->requests while this is running? > > I wonder if we need to have any sort of lock surrounding this? It's called with the BgWriterCommLock already held, and there's an assertion to this effect as well. > With higher shared memory, I couldn't readily induce compaction, > which is probably a concurrency matter of not having enough volume > of concurrent work going on. You can do it artificially by attaching gdb to the bgwriter. > In principle, the only case where it should worsen performance > is if the amount of time required to: > - Set up a hash table > - Insert an entry for each buffer > - Walk the skip_slot array, shortening the request queue > for each duplicate found > exceeded the amount of time required to do the duplicate fsync() > requests. > > That cost should be mighty low. It would be interesting to > instrument CompactBgwriterRequestQueue() to see how long it runs. > > But note that this cost is also spread into a direction where it > likely wouldn't matter, as it is typically invoked by the > background writer process, so this would frequently not be paid > by "on-line" active processes. This is not correct. The background writer only ever does AbsorbFsyncRequests(); this would substitute (to some degree) in cases where the background writer fell down on the job. > bgwriter.c: In function 'CompactBgwriterRequestQueue': > bgwriter.c:1142: warning: 'num_skipped' may be used uninitialized in this function OK, I can fix that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> Also, it's not going to get packaged at all unless it gets renamed to > something less generic, maybe pg_test_fsync; I'm not going to risk the > oppobrium of sticking something named "test_fsync" into /usr/bin. > Moving to contrib would be a good opportunity to fix the name. +1. It would be a lot easier to tell people on -performance to use this if it was in contrib, for that matter. "Go to *what* directory?" -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com
Chris Browne wrote: > It was a little troublesome inducing it. I did so by cutting > shared memory to minimum (128kB)... > With higher shared memory, I couldn't readily induce compaction, > which is probably a concurrency matter of not having enough volume > of concurrent work going on. > Quite. It's taken me 12 days of machine time running pgbench to find the spots where this problem occurs on a system with a reasonably sized shared_buffers (I'm testing against 256MB). It's one of those things it's hard to reproduce with test data. Thanks for the thorough code review. I've got a clear test plan I'm progressing through this week to beat on the performance measurement aspects of the patch. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books
Alvaro Herrera wrote: > I don't understand why it would be "overkill". Are you saying people > would complain because you installed a 25 kB executable that they might > not want to use? Just for fun I checked /usr/bin and noticed that I > have a "pandoc" executable, weighing 17 MB, that I have never used and I > have no idea what is it for. > It's for converting between the various types of text-like markup, i.e. reST, LaTex, Markdown, etc. Anyway, just because the rest of the world has no standards anymore doesn't mean we shouldn't. The changes Bruce has made recently have gotten this tool to where its output is starting to be readable and reliable. The sort of people who want to run this will certainly be fine with installing contrib to do it, because they may want to have things like pgbench too. There's really not enough demand for this to pollute the default server install footprint with any overhead from this tool, either in bytes or increased tool name squatting. And the fact that it's still a little rough around the edges nudges away from the standard server package too. Install in contrib as pg_test_fsync and I think you'll achieve the optimal subset of people who can be made happy here. Not having it packaged at all before wasn't a big deal, because it was so hard to collect good data from only developer-level people were doing it anyway. Now that it is starting to be more useful in that role for less experienced users, we need to make it easier for more people to run it, to collect feedback toward further improving its quality. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books
Greg Smith wrote: > Alvaro Herrera wrote: > > I don't understand why it would be "overkill". Are you saying people > > would complain because you installed a 25 kB executable that they might > > not want to use? Just for fun I checked /usr/bin and noticed that I > > have a "pandoc" executable, weighing 17 MB, that I have never used and I > > have no idea what is it for. > > > > It's for converting between the various types of text-like markup, i.e. > reST, LaTex, Markdown, etc. > > Anyway, just because the rest of the world has no standards anymore > doesn't mean we shouldn't. The changes Bruce has made recently have > gotten this tool to where its output is starting to be readable and > reliable. The sort of people who want to run this will certainly be > fine with installing contrib to do it, because they may want to have > things like pgbench too. There's really not enough demand for this to > pollute the default server install footprint with any overhead from this > tool, either in bytes or increased tool name squatting. And the fact > that it's still a little rough around the edges nudges away from the > standard server package too. > > Install in contrib as pg_test_fsync and I think you'll achieve the > optimal subset of people who can be made happy here. Not having it > packaged at all before wasn't a big deal, because it was so hard to > collect good data from only developer-level people were doing it > anyway. Now that it is starting to be more useful in that role for less > experienced users, we need to make it easier for more people to run it, > to collect feedback toward further improving its quality. OK, I am ready to move test_fsync to /contrib. Is pg_test_fsync the best name? pg_check_fsync? pg_fsync_performance? pg_verify_fsync? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Jan 20, 2011 at 9:13 AM, Bruce Momjian <bruce@momjian.us> wrote: > OK, I am ready to move test_fsync to /contrib. Is pg_test_fsync the > best name? pg_check_fsync? pg_fsync_performance? pg_verify_fsync? I don't see too much reason to rename it more than necessary, so how about pg_test_fsync? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Jan 20, 2011 at 9:13 AM, Bruce Momjian <bruce@momjian.us> wrote: >> OK, I am ready to move test_fsync to /contrib. �Is pg_test_fsync the >> best name? �pg_check_fsync? �pg_fsync_performance? �pg_verify_fsync? > I don't see too much reason to rename it more than necessary, so how > about pg_test_fsync? Yeah, there's no reason to try to confuse people about whether it's the same program or not. regards, tom lane
On 1/20/11 6:15 AM, Robert Haas wrote: > On Thu, Jan 20, 2011 at 9:13 AM, Bruce Momjian <bruce@momjian.us> wrote: >> OK, I am ready to move test_fsync to /contrib. Is pg_test_fsync the >> best name? pg_check_fsync? pg_fsync_performance? pg_verify_fsync? > > I don't see too much reason to rename it more than necessary, so how > about pg_test_fsync? +1. -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com
On Mon, Jan 17, 2011 at 8:23 PM, Greg Smith <greg@2ndquadrant.com> wrote: > Quite. It's taken me 12 days of machine time running pgbench to find the > spots where this problem occurs on a system with a reasonably sized > shared_buffers (I'm testing against 256MB). It's one of those things it's > hard to reproduce with test data. > > Thanks for the thorough code review. I've got a clear test plan I'm > progressing through this week to beat on the performance measurement aspects > of the patch. Any update on this? I think the test results you've posted previously - particularly, the fact that when the queue fills up, there are always many duplicates - is pretty much sufficient for us to convince ourselves that this will provide a benefit in cases where that occurs.And, in cases where the queue doesn't fill up, we'llnever hit the test that triggers this code, so it seems pretty clear there won't be a negative impact there either. I don't want to rush your testing process, but if it's already fairly clear that this will have some benefit, I think it would be good to get it committed and move on to working on the parts we're less sure about, like sorting writes and spreading fsyncs, where we will probably need a lot more testing than here to be sure that we have the right behavior. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Josh Berkus wrote: > On 1/20/11 6:15 AM, Robert Haas wrote: > > On Thu, Jan 20, 2011 at 9:13 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> OK, I am ready to move test_fsync to /contrib. Is pg_test_fsync the > >> best name? pg_check_fsync? pg_fsync_performance? pg_verify_fsync? > > > > I don't see too much reason to rename it more than necessary, so how > > about pg_test_fsync? > > +1. OK, src/tools/test_fsync moved to contrib/pg_test_fsync. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
robertmhaas@gmail.com (Robert Haas) writes: > On Mon, Jan 17, 2011 at 8:23 PM, Greg Smith <greg@2ndquadrant.com> wrote: >> Quite. It's taken me 12 days of machine time running pgbench to find the >> spots where this problem occurs on a system with a reasonably sized >> shared_buffers (I'm testing against 256MB). It's one of those things it's >> hard to reproduce with test data. >> >> Thanks for the thorough code review. I've got a clear test plan I'm >> progressing through this week to beat on the performance measurement aspects >> of the patch. > > Any update on this? I think the test results you've posted previously > - particularly, the fact that when the queue fills up, there are > always many duplicates - is pretty much sufficient for us to convince > ourselves that this will provide a benefit in cases where that occurs. Agreed. This showed up eminently nicely when beating up the database using pgbench. I imagine it would be interesting to run it against a different test than pgbench, particularly one which involves a larger number of tables. From the behavior I have seen thus far, I'm expecting that the queue essentially gets compressed to the size indicating the number of active tables. With pgbench, there are 4 tables, and the queue kept getting compressed to 3 or 4 entries that nicely corresponds with that. > And, in cases where the queue doesn't fill up, we'll never hit the > test that triggers this code, so it seems pretty clear there won't be > a negative impact there either. I don't want to rush your testing > process, but if it's already fairly clear that this will have some > benefit, I think it would be good to get it committed and move on to > working on the parts we're less sure about, like sorting writes and > spreading fsyncs, where we will probably need a lot more testing than > here to be sure that we have the right behavior. I'm pretty happy with what I've seen thus far; I don't want to be over-antsy about getting it all dealt with Right Quick Instantly, but it seems like a change that doesn't have a terribly bad risk of a big downside. -- (reverse (concatenate 'string "ofni.secnanifxunil" "@" "enworbbc")) The statistics on sanity are that one out of every four Americans is suffering from some form of mental illness. Think of your three best friends. If they're okay, then it's you. -- Rita Mae Brown