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 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!
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