Review: compact fsync request queue on overflow

Is there value in moving test_fsync to /contrib?

--
Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> writes:
> On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> writes:
> > On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> wrote:
> Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> > On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/1/17 Bruce Momjian <bruce(at)momjian(dot)us>:
> Robert Haas wrote:
>> On Mon, Jan 17, 2011 at 11:02 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us>        http://momjian.us
>  EnterpriseDB                             http://enterprisedb.com
>
>  + It's impossible for everything to be true. +
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)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(at)gmail(dot)com> writes:
> On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> writes:
> > On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> On Mon, Jan 17, 2011 at 11:47 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jan 17, 2011 at 12:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)acm(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)gmail(dot)com> writes:
> On Thu, Jan 20, 2011 at 9:13 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)2ndquadrant(dot)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'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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

robertmhaas(at)gmail(dot)com (Robert Haas) writes:
> On Mon, Jan 17, 2011 at 8:23 PM, Greg Smith <greg(at)2ndquadrant(dot)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

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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +