Re: posix advises ...

hello everybody,

recently we had a bit of a nightmare with some kernels and concurrent
seq scans.
the thing we encountered was the following: a single "SELECT COUNT(*)
FROM table" on a big table (50 gb) gave us constant 350 mb / sec I/O. as
soon as a second scan dropped in speed dropped to 2 mb / sec. first i
thought that some random I/O dropped in but synchronous scans worked
fine. we found out that there is some madness in some linux kernel /
controller combinations causing this issue.
it did some tests on my local boxes which was clearly not affected by
this problem and I have seen a single SATA disks dropping from 65 mb /
sec to around 45. this is not good.
i found a patch by grep stark implementing posix_fadvise for bitmap
scans. i quickly hacked in suggestions to issue the same advises when a
seq scan is done.
the impact was surprisingly high. single scans went up from 65 mb / sec
to something around 70. concurrent scans are basically at steady, high
speed - no dropping I/O speed anymore until something like 16 scans or so.
even the broken controller when up from "350mb -> 2mb" to "350 -> 50mb".
by replacing the kernel and the driver we see steady behavior here as
well now.

maybe it is worth to discuss posix_fadvise.
we hacked up a simple patch based on greg's work which nicely fixed the
problem for us (brute force).
we also made some simple autoconf hack to check for broken posix_fadvise.
maybe people want to test if they see similar performance differences.
if a patch like that is likely to be accepted we would hack up some more
clean implementation.

many thanks,

hans

--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql-support.de, www.postgresql-support.com

On Sun, 11 May 2008, Hans-Juergen Schoenig wrote:

> we also made some simple autoconf hack to check for broken posix_fadvise.

Because of how you did that, your patch is extremely difficult to even
test. You really should at least scan the output from diff you're about
to send before submitting a patch to make sure it makes sense--yours is
over 30,000 lines long just to implement a small improvement. The reason
for that is that you regenerated "configure" using a later version of
Autoconf than the official distribution, and the diff for the result is
gigantic. You even turned off the check in configure.in that specifically
prevents you from making that mistake so it's not like you weren't warned.

You should just show the necessary modifications to configure.in in your
patch, certainly shouldn't submit a patch that subverts the checks there,
and leave out the resulting configure file if you didn't use the same
version of Autoconf.

I find the concept behind this patch very useful and I'd like to see a
useful one re-submitted. I'm in the middle of setting up some new
hardware this month and was planning to test the existing fadvise patches
Greg Stark sent out as part of that. Having another one to test for
accelerating multiple sequential scans would be extremely helpful to add
to that, because then I think I can reuse some of the test cases Jeff
Davis put together for the 8.3 improvements in that area to see how well
it works. It wasn't as clear to me how to test the existing bitmap scan
patch, yours seems a more straightforward patch to use as a testing ground
for fadvise effectiveness.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith írta:
> On Sun, 11 May 2008, Hans-Juergen Schoenig wrote:
>
>> we also made some simple autoconf hack to check for broken
>> posix_fadvise.
>
> Because of how you did that, your patch is extremely difficult to even
> test. You really should at least scan the output from diff you're
> about to send before submitting a patch to make sure it makes
> sense--yours is over 30,000 lines long just to implement a small
> improvement. The reason for that is that you regenerated "configure"
> using a later version of Autoconf than the official distribution, and
> the diff for the result is gigantic. You even turned off the check in
> configure.in that specifically prevents you from making that mistake
> so it's not like you weren't warned.

Sorry, that old autoconf version was nowhere to be found for Fedora 8.
Fortunately PostgreSQL 8.4 switched already to autoconf 2.61... :-)

> You should just show the necessary modifications to configure.in in
> your patch, certainly shouldn't submit a patch that subverts the
> checks there, and leave out the resulting configure file if you didn't
> use the same version of Autoconf.
>
> I find the concept behind this patch very useful and I'd like to see a
> useful one re-submitted. I'm in the middle of setting up some new
> hardware this month and was planning to test the existing fadvise
> patches Greg Stark sent out as part of that.

This patch (revisited and ported to current CVS HEAD) is indeed using
Greg's original patch and also added another patch written by Mark Wong
that helps evicting closed XLOGs from memory faster. Our additions are:
- advise POSIX_FADV_SEQUENTIAL for seqscans
- configure check
- small documentation for Greg's patch and its GUC
- adapt ginget.c that started using tbm_iterate() recently

The configure check implicitely assumes segfaults (which are
returned as exit code 139 here) can be handled. IIRC Tom Lane
talked about unmatched glibc and Linux kernel versions may
segfault when posix_fadvise() was called. (kernel lacked the feature,
glibc erroneously assumed it can use it)

> Having another one to test for accelerating multiple sequential
> scans would be extremely helpful to add to that, because then I think
> I can reuse some of the test cases Jeff Davis put together for the 8.3
> improvements in that area to see how well it works. It wasn't as
> clear to me how to test the existing bitmap scan patch, yours seems a
> more straightforward patch to use as a testing ground for fadvise
> effectiveness.
>
> --
> * Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD
>

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Thu, 19 Jun 2008, Zoltan Boszormenyi wrote:

> This patch (revisited and ported to current CVS HEAD) is indeed using
> Greg's original patch and also added another patch written by Mark Wong
> that helps evicting closed XLOGs from memory faster.

Great, that will save me some trouble. I've got a stack of Linux
performance testing queued up (got stuck behind a kernel bug impacting
pgbench) for the next couple of weeks and I'll include this in that
testing. I think I've got a similar class of hardware as you tested on
for initial evaluation--I'm getting around 200MB/s sequential I/O right
now out of my small RAID setup,.

I added your patch to the queue for next month's CommitFest and listed
myself as the initial reviewer, but a commit that soon is unlikely.
Performance tests like this usually take a while to converge, and since
this is using a less popular API I expect a round of portability concerns,
too.

Where did Marc's patch come from? I'd like to be able to separate out
that change from the rest if necessary.

Also, if you have any specific test cases you ran that I could start by
trying to replicate a speedup on, those would be handy as well.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

good morning,

this is wonderful news.
this is pretty much what we observed as well. the kernel has acted as
showstopper for many setups recently. this patch fixed most cases
related to kernel read ahead and so on for us.
in fact, posix_fadvise was the only way to prevent a big germany
company from replacing postgres with oracle.
the problem was that synchronized scans led to a significant decrease
of I/O throughput as the kernel was simply confused by processes
concurrently reading the same file.

I hope zoltan's autoconf magic fixes the portability issues.

hans

On Jun 20, 2008, at 1:19 AM, Greg Smith wrote:

> On Thu, 19 Jun 2008, Zoltan Boszormenyi wrote:
>
>> This patch (revisited and ported to current CVS HEAD) is indeed using
>> Greg's original patch and also added another patch written by Mark
>> Wong
>> that helps evicting closed XLOGs from memory faster.
>
> Great, that will save me some trouble. I've got a stack of Linux
> performance testing queued up (got stuck behind a kernel bug
> impacting pgbench) for the next couple of weeks and I'll include
> this in that testing. I think I've got a similar class of hardware
> as you tested on for initial evaluation--I'm getting around 200MB/s
> sequential I/O right now out of my small RAID setup,.
>
> I added your patch to the queue for next month's CommitFest and
> listed myself as the initial reviewer, but a commit that soon is
> unlikely. Performance tests like this usually take a while to
> converge, and since this is using a less popular API I expect a
> round of portability concerns, too.
>
> Where did Marc's patch come from? I'd like to be able to separate
> out that change from the rest if necessary.
>
> Also, if you have any specific test cases you ran that I could
> start by trying to replicate a speedup on, those would be handy as
> well.
>
> --
> * Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com
> Baltimore, MD
>
> --
> Sent via pgsql-patches mailing list (pgsql-patches(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-patches

--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, 2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql-support.de, www.postgresql-support.com

Greg Smith írta:
> On Thu, 19 Jun 2008, Zoltan Boszormenyi wrote:
>
>> This patch (revisited and ported to current CVS HEAD) is indeed using
>> Greg's original patch and also added another patch written by Mark Wong
>> that helps evicting closed XLOGs from memory faster.
>
> Great, that will save me some trouble. I've got a stack of Linux
> performance testing queued up (got stuck behind a kernel bug impacting
> pgbench) for the next couple of weeks and I'll include this in that
> testing. I think I've got a similar class of hardware as you tested
> on for initial evaluation--I'm getting around 200MB/s sequential I/O
> right now out of my small RAID setup,.
>
> I added your patch to the queue for next month's CommitFest and listed
> myself as the initial reviewer, but a commit that soon is unlikely.
> Performance tests like this usually take a while to converge, and
> since this is using a less popular API I expect a round of portability
> concerns, too.
>
> Where did Marc's patch come from? I'd like to be able to separate out
> that change from the rest if necessary.

That patch was posted here:
http://archives.postgresql.org/pgsql-patches/2008-03/msg00000.php

> Also, if you have any specific test cases you ran that I could start
> by trying to replicate a speedup on, those would be handy as well.

We experienced synchronized seqscans slowing down after some (10+) clients
which seems to be strange as it should be a strong selling point of 8.3.
With the posix_fadvise() patchs, the dropoff was pushed further.
The query involved multiple tables, it was not a trivial one table
seqscan case.
Without the patch, both a simpler SATA system (each disk at ~63MB/sec)
and a hw RAID with 400+ MB/sec showed slowdown.
The initial 60-63MB/sec with 1-3 clients on the single SATA disk system
quickly dropped to 11-17MB/sec with more clients.
With the patch, it only dropped to 40-47MB/sec.
I cannot recall the RAID figures.

> --
> * Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Hi Zoltán.

I was reading through your posix_fadvise patch, and I wanted to ask
about this change in particular:

> --- a/src/backend/executor/nodeIndexscan.c
> +++ b/src/backend/executor/nodeIndexscan.c
> @@ -290,7 +290,7 @@ ExecIndexEvalArrayKeys(ExprContext *econtext,
> /* We want to keep the arrays in per-tuple memory */
> oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>
> - for (j = 0; j < numArrayKeys; j++)
> + for (j = numArrayKeys-1; j >= 0; j--)
> {
> ScanKey scan_key = arrayKeys[j].scan_key;
> ExprState *array_expr = arrayKeys[j].array_expr;

Why is this loop reversed? (I could have missed some discussion about
this, I just wanted to make sure it was intentional.)

I can confirm that the patch applies to HEAD, that the configure test
correctly #defines USE_POSIX_FADVISE, that it compiles cleanly, and it
looks basically sensible to me. The nodeBitmapHeapscan.c and iterator
changes need a second look by someone who understands the code better
than I do.

The documentation patch could use a little tweaking:

> + <productname>PostgreSQL</productname> can give hints to POSIX compatible
> + operating systems using posix_fadvise(2) to pre-read pages that will
> + be needed in the near future. Reading the pages into the OS kernel's
> + disk buffer will be done asynchronically while <productname>PostgreSQL</productname>
> + works on pages which are already in memory. This may speed up bitmap scans
> + considerably. This setting only applies to bitmap scans.
> + Hinting for sequential scans is also used but no GUC is needed in this case.

I would suggest something like this instead:

<productname>PostgreSQL</productname> can use posix_fadvise(2) to
tell the operating system about pages that it knows will be needed
in the near future. The performance of bitmap scans is considerably
improved when the kernel pre-reads such pages into its disk buffers.
This variable controls how many pages are marked for pre-reading at
a time during a bitmap scan.

But I'm not convinced that this GUC is well-advised; at least, it needs
some advice about how to determine a sensible size for the parameter
(and maybe a better name). But is it really necessary at all?

Thanks.

-- ams

"Abhijit Menon-Sen" <ams(at)oryx(dot)com> writes:

> Hi Zoltán.
>
> I was reading through your posix_fadvise patch,

Actually Zoltan's patch was based on an earlier patch from me. The sections
you're highlighting here are from my original patch.

> and I wanted to ask about this change in particular:
>
>> --- a/src/backend/executor/nodeIndexscan.c
>> +++ b/src/backend/executor/nodeIndexscan.c
>> @@ -290,7 +290,7 @@ ExecIndexEvalArrayKeys(ExprContext *econtext,
>> /* We want to keep the arrays in per-tuple memory */
>> oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>>
>> - for (j = 0; j < numArrayKeys; j++)
>> + for (j = numArrayKeys-1; j >= 0; j--)

> Why is this loop reversed? (I could have missed some discussion about
> this, I just wanted to make sure it was intentional.)

There was some discussion about this change and in fact if you look at CVS
HEAD you'll find it already applied. It probably doesn't make a big difference
at least in most cases but it seems more sensible to increment the least
significant key elements first since that maximizes the chances of fetching
index keys from the same index pages or nearby pages. Incrementing the most
significant index keys would maximize the distance we're jumpin around in the
index tree.

> But I'm not convinced that this GUC is well-advised; at least, it needs
> some advice about how to determine a sensible size for the parameter
> (and maybe a better name). But is it really necessary at all?

I'm not sure which version of my patch Zoltan's was based on. The later
versions of mine had a GUC named effective_spindle_count which I think is
nicely abstracted away from the implementation details. We do need something
like that because we have no way to determine how wide a raid stripe Postgres
is sitting on and the optimal read-ahead depends on that.

The alternative would be to measure the bandwith with various amounts of
read-ahead. But I fear that would be quite hard on a production system with
the amount of noise from other i/o. It's hard enough on an idle machine with
synthetic benchmarks.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

At 2008-07-12 00:52:42 +0100, stark(at)enterprisedb(dot)com wrote:
>
> There was some discussion about this change and in fact if you
> look at CVS HEAD you'll find it already applied.

Not as far as I can see.

> Incrementing the most significant index keys would maximize the
> distance we're jumpin around in the index tree.

I see. Thanks.

> The later versions of mine had a GUC named effective_spindle_count
> which I think is nicely abstracted away from the implementation
> details.

Yes, that does sound much better. (The patch I read had a
preread_pages_bitmapscan variable instead.)

-- ams

Abhijit Menon-Sen <ams(at)oryx(dot)com> writes:
> At 2008-07-12 00:52:42 +0100, stark(at)enterprisedb(dot)com wrote:
>> There was some discussion about this change and in fact if you
>> look at CVS HEAD you'll find it already applied.

> Not as far as I can see.

The place where it matters is in ExecIndexAdvanceArrayKeys(),
not initial setup.

regards, tom lane

On Sat, 12 Jul 2008, Abhijit Menon-Sen wrote:

> At 2008-07-12 00:52:42 +0100, stark(at)enterprisedb(dot)com wrote:
>>
>> The later versions of mine had a GUC named effective_spindle_count
>> which I think is nicely abstracted away from the implementation
>> details.
>
> Yes, that does sound much better. (The patch I read had a
> preread_pages_bitmapscan variable instead.)

This patch does need a bit of general care in a couple of areas. The
reviewing game plan I'm working through goes like this:

1) Update the original fadvise test program Greg Stark wrote to be a bit
easier to use for testing general compatibility of this approach. I want
to collect some data from at least two Linux and Solaris systems with
different disk setups.

2) Check out effective_spindle_count and see if it looks like a reasonable
way to tune this feature. If so, will probably need to merge that in to
Zoltan's version of the patch. May need some other cleanup in that patch
set as well--I'm not sure that closed XLOG patch that got pushed into here
as well is really helpful for example.

3) Generate a sequential scan test program aimed to hobble the Linux
kernel in the way Zoltan described as motivation for his work. I'm
working with Jeff Davis this week to try and repurpose some of his
syncronized scan test programs to handle this while we're both in the same
place for a bit.

4) Generate a bitmap scan test program to check the original patch.

5) If the performance results look useful and consistant, then move toward
cleaning up broader compatibility issues like the segfault concerns Zoltan
mentioned.

Going to take a while to work through all that, but performance patches
with platform-specific benefit are always painful like this.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith wrote:

> This patch does need a bit of general care in a couple of areas. The
> reviewing game plan I'm working through goes like this:

Did this review effort go anywhere?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Sun, 31 Aug 2008, Alvaro Herrera wrote:

> Greg Smith wrote:
>
>> This patch does need a bit of general care in a couple of areas. The
>> reviewing game plan I'm working through goes like this:
>
> Did this review effort go anywhere?

Haven't made much progress--all my spare time for work like this lately
has been stuck fighting broken hardware and OS issues on my test systems.
I have a lot more time set aside for PostgreSQL hacking this month, and
I'll look at that and the checkpoint block sorting patch I've also been
tinkering with soon and kick back an initial review of each.

But if anybody else is keen on working on either patch, let me know. Be
glad to assist instead of lead. It takes fairly serious hardware to work
on either of these usefully though.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD