Free WAL caches on switching segments
Hi,
Here is a small patch to prevent undesired WAL file caching by kernel.
posix_fadvise(POSIX_FADV_DONTNEED) attempts to free cached pages and
the kernel will discard them in preference to other data caches.
I think it works just like as O_DIRECT in terms of cache control.
O_DIRECT may be a better solution than posix_fadvise, but
posix_fadvise may be used on platforms where O_DIRECT is not supported.
pgbench results are as follows:
wal_sync_method
- open_sync : 156.0 tps
- fdatasync : 126.3 tps
- fdatasync+fadvise : 161.2 tps
(8.1beta1 on Linux 2.6.8-24)
I'll appreciate any comments and advices,
Takahiro
*** xlog.c Mon Aug 29 11:51:19 2005
--- xlog-with-fadvise.c Tue Aug 30 15:08:24 2005
***************
*** 1352,1357 ****
--- 1352,1358 ----
Assert(npages == 0);
if (openLogFile >= 0)
{
+ posix_fadvise(openLogFile, 0, 0, POSIX_FADV_DONTNEED);
if (close(openLogFile))
ereport(PANIC,
(errcode_for_file_access(),
***************
*** 1535,1540 ****
--- 1536,1542 ----
if (openLogFile >= 0 &&
!XLByteInPrevSeg(LogwrtResult.Write, openLogId, openLogSeg))
{
+ posix_fadvise(openLogFile, 0, 0, POSIX_FADV_DONTNEED);
if (close(openLogFile))
ereport(PANIC,
(errcode_for_file_access(),
ITAGAKI Takahiro <itagaki.takahiro@lab.ntt.co.jp> writes:
Here is a small patch to prevent undesired WAL file caching by kernel.
posix_fadvise(POSIX_FADV_DONTNEED) attempts to free cached pages and
the kernel will discard them in preference to other data caches.
On plenty of platforms, this won't even compile ...
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Here is a small patch to prevent undesired WAL file caching by kernel.
posix_fadvise(POSIX_FADV_DONTNEED) attempts to free cached pages and
the kernel will discard them in preference to other data caches.On plenty of platforms, this won't even compile ...
Do you mean simply following code? or more pretty way?
#ifdef POSIX_FADV_DONTNEED
posix_fadvise(openLogFile, 0, 0, POSIX_FADV_DONTNEED);
#endif
---
ITAGAKI Takahiro
NTT Cyber Space Laboratories
ITAGAKI Takahiro wrote:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Here is a small patch to prevent undesired WAL file caching by kernel.
posix_fadvise(POSIX_FADV_DONTNEED) attempts to free cached pages and
the kernel will discard them in preference to other data caches.On plenty of platforms, this won't even compile ...
Do you mean simply following code? or more pretty way?
#ifdef POSIX_FADV_DONTNEED
posix_fadvise(openLogFile, 0, 0, POSIX_FADV_DONTNEED);
#endif
Yes, but we probably have to have a configure test to see if
posix_fadvise exists too. I will keep this for 8.2.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> wrote:
Here is a small patch to prevent undesired WAL file caching by kernel.
posix_fadvise(POSIX_FADV_DONTNEED) attempts to free cached pages and
the kernel will discard them in preference to other data caches.On plenty of platforms, this won't even compile ...
Yes, but we probably have to have a configure test to see if
posix_fadvise exists too. I will keep this for 8.2.
I think we can use _POSIX_ADVISORY_INFO to test if posix_fadvise exists.
Also, I added the check on whether WAL archiving is enabled, because
archivers might use the caches to read the WAL segment.
By the way, should we put posix_fadvise on the separated place with renaming
pg_fadvise? If we use posix_fadvise in other purposes, for example,
read-ahead control, the separation would be good to keep codes clean.
---
ITAGAKI Takahiro
NTT Cyber Space Laboratories
Attachments:
xlog-with-fadvise.patchapplication/octet-stream; name=xlog-with-fadvise.patchDownload+48-21
I looked this over and I am unsure what this does for us that isn't
already accomplished using the wal_sync_method settings. See xlog.c for
a description of O_DIRECT and when it is used.
---------------------------------------------------------------------------
ITAGAKI Takahiro wrote:
Bruce Momjian <pgman@candle.pha.pa.us> wrote:
Here is a small patch to prevent undesired WAL file caching by kernel.
posix_fadvise(POSIX_FADV_DONTNEED) attempts to free cached pages and
the kernel will discard them in preference to other data caches.On plenty of platforms, this won't even compile ...
Yes, but we probably have to have a configure test to see if
posix_fadvise exists too. I will keep this for 8.2.I think we can use _POSIX_ADVISORY_INFO to test if posix_fadvise exists.
Also, I added the check on whether WAL archiving is enabled, because
archivers might use the caches to read the WAL segment.By the way, should we put posix_fadvise on the separated place with renaming
pg_fadvise? If we use posix_fadvise in other purposes, for example,
read-ahead control, the separation would be good to keep codes clean.---
ITAGAKI Takahiro
NTT Cyber Space Laboratories
[ Attachment, skipping... ]
---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> wrote:
I looked this over and I am unsure what this does for us that isn't
already accomplished using the wal_sync_method settings. See xlog.c for
a description of O_DIRECT and when it is used.
I proposed it to supplement the cache control. There are some OSes that
supports posix_fadvise but not O_DIRECT, for example, NetBSD 4.0
(http://www.netbsd.org/Changes/changes-4.0.html).
---
ITAGAKI Takahiro
NTT Cyber Space Laboratories
ITAGAKI Takahiro wrote:
Bruce Momjian <pgman@candle.pha.pa.us> wrote:
I looked this over and I am unsure what this does for us that isn't
already accomplished using the wal_sync_method settings. See xlog.c for
a description of O_DIRECT and when it is used.I proposed it to supplement the cache control. There are some OSes that
supports posix_fadvise but not O_DIRECT, for example, NetBSD 4.0
(http://www.netbsd.org/Changes/changes-4.0.html).
Oh, that makes sense then. Let me re-add it to the queue.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
ITAGAKI Takahiro wrote:
Bruce Momjian <pgman@candle.pha.pa.us> wrote:
I looked this over and I am unsure what this does for us that isn't
already accomplished using the wal_sync_method settings. See xlog.c for
a description of O_DIRECT and when it is used.I proposed it to supplement the cache control. There are some OSes that
supports posix_fadvise but not O_DIRECT, for example, NetBSD 4.0
(http://www.netbsd.org/Changes/changes-4.0.html).
Oh, that makes sense then. Let me re-add it to the queue.
Could we see some performance measurements *from those OSes*? The given
test on Linux certainly does not justify adding another operating
system dependency to the WAL code. For that matter, even if it is a big
win on some versions of NetBSD, I'm not sure I'd want to accept it ...
how many NetBSD users do we have who would care?
Depending on OS features we have never depended on before is a *huge*
ongoing maintenance cost, and I have not seen an argument that I think
justifies this one.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
ITAGAKI Takahiro wrote:
Bruce Momjian <pgman@candle.pha.pa.us> wrote:
I looked this over and I am unsure what this does for us that isn't
already accomplished using the wal_sync_method settings. See xlog.c for
a description of O_DIRECT and when it is used.I proposed it to supplement the cache control. There are some OSes that
supports posix_fadvise but not O_DIRECT, for example, NetBSD 4.0
(http://www.netbsd.org/Changes/changes-4.0.html).Oh, that makes sense then. Let me re-add it to the queue.
Could we see some performance measurements *from those OSes*? The given
test on Linux certainly does not justify adding another operating
system dependency to the WAL code. For that matter, even if it is a big
win on some versions of NetBSD, I'm not sure I'd want to accept it ...
how many NetBSD users do we have who would care?Depending on OS features we have never depended on before is a *huge*
ongoing maintenance cost, and I have not seen an argument that I think
justifies this one.
I disagree. It is a localized change and seems like a win, and it uses
a standard POSIX feature, rather than an OS-specific one.
I fact the patch cleans up our code by centralizing the WAL close() code
and error message.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
Depending on OS features we have never depended on before is a *huge*
ongoing maintenance cost, and I have not seen an argument that I think
justifies this one.
I disagree. It is a localized change and seems like a win, and it uses
a standard POSIX feature, rather than an OS-specific one.
It's still gonna need a configure test and so on. "POSIX" does not mean
"exists everywhere". Moreover, the submitter has not even proven that
the code works (or even builds, much less does anything useful) on the
platforms it's supposedly for.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
Depending on OS features we have never depended on before is a *huge*
ongoing maintenance cost, and I have not seen an argument that I think
justifies this one.I disagree. It is a localized change and seems like a win, and it uses
a standard POSIX feature, rather than an OS-specific one.It's still gonna need a configure test and so on. "POSIX" does not mean
"exists everywhere". Moreover, the submitter has not even proven that
the code works (or even builds, much less does anything useful) on the
platforms it's supposedly for.
The submitter believes the C macro test is sufficient:
I think we can use _POSIX_ADVISORY_INFO to test if posix_fadvise exists.
Also, I added the check on whether WAL archiving is enabled, because
archivers might use the caches to read the WAL segment.
I assume if it follows the POSIX spec it will work on all platforms that
support this feature.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
It's still gonna need a configure test and so on.
The submitter believes the C macro test is sufficient:
Do I get to revert the patch the moment that fails in buildfarm?
More to the point, the utility of the patch remains unproven.
We are not in the habit of adding OS dependencies on speculation.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
It's still gonna need a configure test and so on.
The submitter believes the C macro test is sufficient:
Do I get to revert the patch the moment that fails in buildfarm?
You bet. In fact I liked his centralizing the WAL close() irrespective
of the feature itself.
More to the point, the utility of the patch remains unproven.
We are not in the habit of adding OS dependencies on speculation.
He ran tests, though it is speculation because non-caching is a pretty
hard thing to find a benefit from except under low memory situations.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
It's still gonna need a configure test and so on.
The submitter believes the C macro test is sufficient:
Do I get to revert the patch the moment that fails in buildfarm?
In fact, I will revert it when it fails in the build farm. :-)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
More to the point, the utility of the patch remains unproven.
We are not in the habit of adding OS dependencies on speculation.
He ran tests, though it is speculation because non-caching is a pretty
hard thing to find a benefit from except under low memory situations.
Well, the tests (a) didn't show any particularly good speedup, and
(b) were not on the platforms that this is speculated to be useful on
(ie, those without O_DIRECT).
I really don't think that an adequate case has been made for adding
a new OS dependency.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
More to the point, the utility of the patch remains unproven.
We are not in the habit of adding OS dependencies on speculation.He ran tests, though it is speculation because non-caching is a pretty
hard thing to find a benefit from except under low memory situations.Well, the tests (a) didn't show any particularly good speedup, and
(b) were not on the platforms that this is speculated to be useful on
(ie, those without O_DIRECT).I really don't think that an adequate case has been made for adding
a new OS dependency.
Well, I think the patch should be applied, and the submitter does too,
so unless I hear other votes, it is going in.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Mon, Feb 13, 2006 at 02:59:43PM -0500, Bruce Momjian wrote:
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
More to the point, the utility of the patch remains unproven.
We are not in the habit of adding OS dependencies on speculation.He ran tests, though it is speculation because non-caching is a pretty
hard thing to find a benefit from except under low memory situations.Well, the tests (a) didn't show any particularly good speedup, and
(b) were not on the platforms that this is speculated to be useful on
(ie, those without O_DIRECT).I really don't think that an adequate case has been made for adding
a new OS dependency.Well, I think the patch should be applied, and the submitter does too,
so unless I hear other votes, it is going in.
I vote no for whatever that is worth. A "performance" change needs to
actually demonstrate improved performance. If the change is really
desireable to clean up some messy code, then add it as a cleanup change
without the extra system calls. Otherwise it just adds one more bit of
mystery for future maintainers who may be decieved into thinking that posix
advise calls are important voodoo.
-dg
--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with insects.
daveg wrote:
On Mon, Feb 13, 2006 at 02:59:43PM -0500, Bruce Momjian wrote:
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
More to the point, the utility of the patch remains unproven.
We are not in the habit of adding OS dependencies on speculation.He ran tests, though it is speculation because non-caching is a pretty
hard thing to find a benefit from except under low memory situations.Well, the tests (a) didn't show any particularly good speedup, and
(b) were not on the platforms that this is speculated to be useful on
(ie, those without O_DIRECT).I really don't think that an adequate case has been made for adding
a new OS dependency.Well, I think the patch should be applied, and the submitter does too,
so unless I hear other votes, it is going in.I vote no for whatever that is worth. A "performance" change needs to
actually demonstrate improved performance. If the change is really
desireable to clean up some messy code, then add it as a cleanup change
without the extra system calls. Otherwise it just adds one more bit of
mystery for future maintainers who may be decieved into thinking that posix
advise calls are important voodoo.
Yes, your vote counts very much. What if I apply the patch, but mark
the posix_advise() call in a NOT_USED macro block, so it will be ready
for people to test, but will not be used until we are sure.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Yes, your vote counts very much. What if I apply the patch, but mark
the posix_advise() call in a NOT_USED macro block, so it will be ready
for people to test, but will not be used until we are sure.
Sounds like a recipe for ensuring it never will be tested. What's
needed here is some actual tests, not preparation...
regards, tom lane
