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 applied cleanly 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 confused and 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 in principle 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 "compacted fsync 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 logging this 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 having enough 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 busy enough 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: - 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. 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 unlikely that 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 assigning num_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! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers