Horton

Thread

  1. Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-25T12:48:51Z

    In recovery, with full_pages_writes=on, we read in each page only to 
overwrite the contents with a full page image. That's a waste of time, 
and can have a surprisingly large effect on recovery time.

As a quick test on my laptop, I initialized a DBT-2 test with 5 
warehouses, and let it run for 2 minutes without think-times to generate 
some WAL. Then I did a "kill -9 postmaster", and took a copy of the data 
directory to use for testing recovery.

With CVS HEAD, the recovery took ~ 2 minutes. With the attached patch, 
it took 5 seconds. (yes, I used the same not-yet-recovered data 
directory in both tests, and cleared the os cache with "echo 1 > 
/proc/sys/vm/drop_caches").

I was surprised how big a difference it makes, but when you think about 
it it's logical. Without the patch, it's doing roughly the same I/O as 
the test itself, reading in pages, modifying them, and writing them 
back. With the patch, all the reads are done sequentially from the WAL, 
and then written back in a batch at the end of the WAL replay which is a 
lot more efficient.

It's interesting that (with the patch) full_page_writes can *shorten* 
your recovery time. I've always thought it to have a purely negative 
effect on performance.

I'll leave it up to the jury if this tiny little change is appropriate 
after feature freeze...

While working on this, this comment in ReadBuffer caught my eye:

> 	/*
> 	 * During WAL recovery, the first access to any data page should
> 	 * overwrite the whole page from the WAL; so a clobbered page
> 	 * header is not reason to fail.  Hence, when InRecovery we may
> 	 * always act as though zero_damaged_pages is ON.
> 	 */
> 	if (zero_damaged_pages || InRecovery)
> 	{

But that assumption only holds if full_page_writes is enabled, right? I 
changed that in the attached patch as well, but if it isn't accepted 
that part of it should still be applied, I think.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  2. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-25T13:13:26Z

    Heikki Linnakangas wrote:
> While working on this, this comment in ReadBuffer caught my eye:
> 
>>     /*
>>      * During WAL recovery, the first access to any data page should
>>      * overwrite the whole page from the WAL; so a clobbered page
>>      * header is not reason to fail.  Hence, when InRecovery we may
>>      * always act as though zero_damaged_pages is ON.
>>      */
>>     if (zero_damaged_pages || InRecovery)
>>     {
> 
> But that assumption only holds if full_page_writes is enabled, right? I 
> changed that in the attached patch as well, but if it isn't accepted 
> that part of it should still be applied, I think.

On second thought, my fix still isn't 100% right because one could turn 
full_page_writes on before starting replay.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  3. Re: Avoiding unnecessary reads in recovery

    Gregory Stark <stark@enterprisedb.com> — 2007-04-25T13:32:20Z

    "Heikki Linnakangas" <heikki@enterprisedb.com> writes:

> While working on this, this comment in ReadBuffer caught my eye:
>
>> 	/*
>> 	 * During WAL recovery, the first access to any data page should
>> 	 * overwrite the whole page from the WAL; so a clobbered page
>> 	 * header is not reason to fail.  Hence, when InRecovery we may
>> 	 * always act as though zero_damaged_pages is ON.
>> 	 */
>> 	if (zero_damaged_pages || InRecovery)
>> 	{
>
> But that assumption only holds if full_page_writes is enabled, right? I changed
> that in the attached patch as well, but if it isn't accepted that part of it
> should still be applied, I think.

Well it's only true if full_page_writes was on when the WAL was written. Which
isn't necessarily the same as saying it's enabled during recovery...

As long as there's a backup block in the log we can use it to clobber pages in
the heap -- which is what your patch effectively does anyways. If we're
replaying a log entry where there isn't a backup block and we find a damaged
page then we're in trouble. Either the damaged page was in a previous backup
block or it's the recovery itself that's damaging it. 

In the latter case it would be pretty useful to abort the recovery so the user
doesn't lose his backup and has a chance to recovery properly (possibly after
reporting and fixing the bug).

So in short I think with your patch this piece of code no longer has a role.
Either your patch kicks in and we never even look at the damaged page at all,
or we should be treating it as corrupt data and just check zero_damaged_pages
alone and not do anything special in recovery.

-- 
  Gregory Stark
  EnterpriseDB          http://www.enterprisedb.com

  4. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-25T15:02:53Z

    Gregory Stark wrote:
> So in short I think with your patch this piece of code no longer has a role.
> Either your patch kicks in and we never even look at the damaged page at all,
> or we should be treating it as corrupt data and just check zero_damaged_pages
> alone and not do anything special in recovery.

Good point. Adjusted patch attached. I added some comments as well.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  5. Re: Avoiding unnecessary reads in recovery

    Simon Riggs <simon@2ndquadrant.com> — 2007-04-25T15:16:20Z

    On Wed, 2007-04-25 at 13:48 +0100, Heikki Linnakangas wrote:

> I was surprised how big a difference it makes, but when you think about 
> it it's logical. Without the patch, it's doing roughly the same I/O as 
> the test itself, reading in pages, modifying them, and writing them 
> back. With the patch, all the reads are done sequentially from the WAL, 
> and then written back in a batch at the end of the WAL replay which is a 
> lot more efficient.

Interesting patch.

It would be good to see a longer term test. I'd expect things to fall
back to about 50% of the time on a longer recovery where the writes need
to be written out of cache.

I was thinking of getting the bgwriter to help out to improve matters
there.


Patch-wise, I love the name ZapBuffer() but would think that
OverwriteBuffer() would be more descriptive.

As regards the zero_damaged_pages question, I raised that some time ago
but we didn't arrive at an explicit answer. All I would say is we can't
allow invalid pages in the buffer manager at any time, whatever options
we have requested, otherwise other code will fail almost immediately.
I'm not sure there's another option?

-- 
  Simon Riggs             
  EnterpriseDB   http://www.enterprisedb.com

  6. Re: Avoiding unnecessary reads in recovery

    Decibel! <decibel@decibel.org> — 2007-04-25T17:13:57Z

    On Apr 25, 2007, at 2:48 PM, Heikki Linnakangas wrote:
> In recovery, with full_pages_writes=on, we read in each page only  
> to overwrite the contents with a full page image. That's a waste of  
> time, and can have a surprisingly large effect on recovery time.
>
> As a quick test on my laptop, I initialized a DBT-2 test with 5  
> warehouses, and let it run for 2 minutes without think-times to  
> generate some WAL. Then I did a "kill -9 postmaster", and took a  
> copy of the data directory to use for testing recovery.
>
> With CVS HEAD, the recovery took ~ 2 minutes. With the attached  
> patch, it took 5 seconds. (yes, I used the same not-yet-recovered  
> data directory in both tests, and cleared the os cache with "echo 1  
> > /proc/sys/vm/drop_caches").
>
> I was surprised how big a difference it makes, but when you think  
> about it it's logical. Without the patch, it's doing roughly the  
> same I/O as the test itself, reading in pages, modifying them, and  
> writing them back. With the patch, all the reads are done  
> sequentially from the WAL, and then written back in a batch at the  
> end of the WAL replay which is a lot more efficient.
>
> It's interesting that (with the patch) full_page_writes can  
> *shorten* your recovery time. I've always thought it to have a  
> purely negative effect on performance.
>
> I'll leave it up to the jury if this tiny little change is  
> appropriate after feature freeze...
>
> While working on this, this comment in ReadBuffer caught my eye:
>
>> 	/*
>> 	 * During WAL recovery, the first access to any data page should
>> 	 * overwrite the whole page from the WAL; so a clobbered page
>> 	 * header is not reason to fail.  Hence, when InRecovery we may
>> 	 * always act as though zero_damaged_pages is ON.
>> 	 */
>> 	if (zero_damaged_pages || InRecovery)
>> 	{
>
> But that assumption only holds if full_page_writes is enabled,  
> right? I changed that in the attached patch as well, but if it  
> isn't accepted that part of it should still be applied, I think.

So what happens if a backend is running with full_page_writes = off,  
someone edits postgresql.conf to turns it on and forgets to reload/ 
restart, and then we crash? You'll come up in recovery mode thinking  
that f_p_w was turned on, when in fact it wasn't.

ISTM that we need to somehow log what the status of full_page_writes  
is, if it's going to affect how recovery works.
--
Jim Nasby                                            jim@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

  7. Re: Avoiding unnecessary reads in recovery

    Tom Lane <tgl@sss.pgh.pa.us> — 2007-04-25T17:20:13Z

    "Simon Riggs" <simon@2ndquadrant.com> writes:
> As regards the zero_damaged_pages question, I raised that some time ago
> but we didn't arrive at an explicit answer. All I would say is we can't
> allow invalid pages in the buffer manager at any time, whatever options
> we have requested, otherwise other code will fail almost immediately.

Yeah --- the proposed new bufmgr routine should probably explicitly zero
the content of the buffer.  It doesn't really matter in the context of
WAL recovery, since there can't be any concurrent access to the buffer,
but it'd make it safe to use in non-WAL contexts (I think there are
other places where we know we are going to init the page and so a
physical read is a waste of time).  Also, this would let the patch be

+	if (alloc_only)
+		MemSet...
+	else
		smgrread...

and you don't need to hack out the PageHeaderIsValid test, since it will
allow zeroed pages.

Possibly ReadZeroedBuffer would be a better name?

			regards, tom lane

  8. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-25T21:44:25Z

    Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
>> As regards the zero_damaged_pages question, I raised that some time ago
>> but we didn't arrive at an explicit answer. All I would say is we can't
>> allow invalid pages in the buffer manager at any time, whatever options
>> we have requested, otherwise other code will fail almost immediately.
> 
> Yeah --- the proposed new bufmgr routine should probably explicitly zero
> the content of the buffer.  It doesn't really matter in the context of
> WAL recovery, since there can't be any concurrent access to the buffer,
> but it'd make it safe to use in non-WAL contexts (I think there are
> other places where we know we are going to init the page and so a
> physical read is a waste of time).

Is there? I can't think of any. Extending a relation doesn't count.

Zeroing the buffer explicitly might be a good idea anyway. I agree it 
feels a bit dangerous to have a moment when there's a garbled page in 
buffer cache, even if only in recovery.

> Also, this would let the patch be
> 
> +	if (alloc_only)
> +		MemSet...
> +	else
> 		smgrread...
> 
> and you don't need to hack out the PageHeaderIsValid test, since it will
> allow zeroed pages.

Well, I'd still put the PageHeaderIsValid test in the else-branch. Not 
like the few cycles spent on the test matter, but I don't see a reason 
not to.

> Possibly ReadZeroedBuffer would be a better name?

Yeah, sounds good. "Read" is a bit misleading, since it doesn't actually 
read in the buffer, but it's good that the name is closely related to 
ReadBuffer.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  9. Re: Avoiding unnecessary reads in recovery

    Tom Lane <tgl@sss.pgh.pa.us> — 2007-04-25T21:49:42Z

    Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> but it'd make it safe to use in non-WAL contexts (I think there are
>> other places where we know we are going to init the page and so a
>> physical read is a waste of time).

> Is there? I can't think of any. Extending a relation doesn't count.

No, but re-using a free page in an index does.  I'm not sure which index
AMs know for sure the page is free, and which have to read it and check,
but I think there's at least some scope for that.

			regards, tom lane

  10. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-26T08:29:20Z

    Tom Lane wrote:
> Heikki Linnakangas <heikki@enterprisedb.com> writes:
>> Tom Lane wrote:
>>> but it'd make it safe to use in non-WAL contexts (I think there are
>>> other places where we know we are going to init the page and so a
>>> physical read is a waste of time).
> 
>> Is there? I can't think of any. Extending a relation doesn't count.
> 
> No, but re-using a free page in an index does.  I'm not sure which index
> AMs know for sure the page is free, and which have to read it and check,
> but I think there's at least some scope for that.

B-tree, GIN ans GiST read and check. I'm not sure how hash works. I 
think the latest bitmap index patch doesn't support reusing empty pages 
at all.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  11. Re: Avoiding unnecessary reads in recovery

    Zeugswetter Andreas DCP SD <zeugswettera@spardat.at> — 2007-04-26T14:10:56Z

    > So what happens if a backend is running with full_page_writes 
> = off, someone edits postgresql.conf to turns it on and 
> forgets to reload/ restart, and then we crash? You'll come up 
> in recovery mode thinking that f_p_w was turned on, when in 
> fact it wasn't.
> 
> ISTM that we need to somehow log what the status of 
> full_page_writes is, if it's going to affect how recovery works.

Optimally recovery should do this when confronted with a full page image
only. The full page is in the same WAL record that first touches a page,
so this should not need to depend on a setting.

Andreas

  12. Re: Avoiding unnecessary reads in recovery

    Tom Lane <tgl@sss.pgh.pa.us> — 2007-04-26T14:39:28Z

    Jim Nasby <decibel@decibel.org> writes:
> So what happens if a backend is running with full_page_writes = off,  
> someone edits postgresql.conf to turns it on and forgets to reload/ 
> restart, and then we crash? You'll come up in recovery mode thinking  
> that f_p_w was turned on, when in fact it wasn't.

One of the advantages of the proposed patch is that it avoids having to
make any assumptions like that.

			regards, tom lane

  13. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-27T11:22:22Z

    Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
>> As regards the zero_damaged_pages question, I raised that some time ago
>> but we didn't arrive at an explicit answer. All I would say is we can't
>> allow invalid pages in the buffer manager at any time, whatever options
>> we have requested, otherwise other code will fail almost immediately.
> 
> Yeah --- the proposed new bufmgr routine should probably explicitly zero
> the content of the buffer.  It doesn't really matter in the context of
> WAL recovery, since there can't be any concurrent access to the buffer,
> but it'd make it safe to use in non-WAL contexts (I think there are
> other places where we know we are going to init the page and so a
> physical read is a waste of time).  

To implement that correctly, I think we'd need to take the content lock 
to clear the buffer if it's already found in the cache. It doesn't seem 
right to me for the buffer manager to do that, in the worst case it 
could lead to deadlocks if that function was ever used while holding 
another buffer locked.

What we could have is the semantics of "Return a buffer, with either 
correct contents or completely zeroed out". It would act just like 
ReadBuffer if the buffer was already in memory, and zero out the page 
otherwise. That's a bit strange semantics to have, but is simple to 
implement and works for the use-cases we've been talking about.

Patch implementing that attached. I named the function "ReadOrZeroBuffer".

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  14. Re: Avoiding unnecessary reads in recovery

    Alvaro Herrera <alvherre@commandprompt.com> — 2007-04-27T13:16:42Z

    Heikki Linnakangas wrote:

> What we could have is the semantics of "Return a buffer, with either 
> correct contents or completely zeroed out". It would act just like 
> ReadBuffer if the buffer was already in memory, and zero out the page 
> otherwise. That's a bit strange semantics to have, but is simple to 
> implement and works for the use-cases we've been talking about.

Huh, why does that work in the case where the recovery code reads a
page, then evicts it because of memory pressure, and later needs to read
it again?

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

  15. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-27T13:21:04Z

    Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
> 
>> What we could have is the semantics of "Return a buffer, with either 
>> correct contents or completely zeroed out". It would act just like 
>> ReadBuffer if the buffer was already in memory, and zero out the page 
>> otherwise. That's a bit strange semantics to have, but is simple to 
>> implement and works for the use-cases we've been talking about.
> 
> Huh, why does that work in the case where the recovery code reads a
> page, then evicts it because of memory pressure, and later needs to read
> it again?

I don't understand the problem. You only use ReadOrZeroBuffer when 
you're going to replace the contents entirely, and don't care about the 
old contents. If you want to read something in, you use ReadBuffer.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  16. Re: Avoiding unnecessary reads in recovery

    Alvaro Herrera <alvherre@commandprompt.com> — 2007-04-27T14:05:43Z

    Heikki Linnakangas wrote:
> Alvaro Herrera wrote:
> >Heikki Linnakangas wrote:
> >
> >>What we could have is the semantics of "Return a buffer, with either 
> >>correct contents or completely zeroed out". It would act just like 
> >>ReadBuffer if the buffer was already in memory, and zero out the page 
> >>otherwise. That's a bit strange semantics to have, but is simple to 
> >>implement and works for the use-cases we've been talking about.
> >
> >Huh, why does that work in the case where the recovery code reads a
> >page, then evicts it because of memory pressure, and later needs to read
> >it again?
> 
> I don't understand the problem. You only use ReadOrZeroBuffer when 
> you're going to replace the contents entirely, and don't care about the 
> old contents. If you want to read something in, you use ReadBuffer.

Oh, the recovery code selects which one to call based on the "init"
param, which is on the first hunk of the diff :-)  I forgot that, I was
just thinking in your description above.

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

  17. Re: Avoiding unnecessary reads in recovery

    Bruce Momjian <bruce@momjian.us> — 2007-04-27T14:37:29Z

    I assume this is 8.4 material.

---------------------------------------------------------------------------

Heikki Linnakangas wrote:
> Tom Lane wrote:
> > "Simon Riggs" <simon@2ndquadrant.com> writes:
> >> As regards the zero_damaged_pages question, I raised that some time ago
> >> but we didn't arrive at an explicit answer. All I would say is we can't
> >> allow invalid pages in the buffer manager at any time, whatever options
> >> we have requested, otherwise other code will fail almost immediately.
> > 
> > Yeah --- the proposed new bufmgr routine should probably explicitly zero
> > the content of the buffer.  It doesn't really matter in the context of
> > WAL recovery, since there can't be any concurrent access to the buffer,
> > but it'd make it safe to use in non-WAL contexts (I think there are
> > other places where we know we are going to init the page and so a
> > physical read is a waste of time).  
> 
> To implement that correctly, I think we'd need to take the content lock 
> to clear the buffer if it's already found in the cache. It doesn't seem 
> right to me for the buffer manager to do that, in the worst case it 
> could lead to deadlocks if that function was ever used while holding 
> another buffer locked.
> 
> What we could have is the semantics of "Return a buffer, with either 
> correct contents or completely zeroed out". It would act just like 
> ReadBuffer if the buffer was already in memory, and zero out the page 
> otherwise. That's a bit strange semantics to have, but is simple to 
> implement and works for the use-cases we've been talking about.
> 
> Patch implementing that attached. I named the function "ReadOrZeroBuffer".
> 
> -- 
>    Heikki Linnakangas
>    EnterpriseDB   http://www.enterprisedb.com


> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

-- 
  Bruce Momjian  <bruce@momjian.us>          http://momjian.us
  EnterpriseDB                               http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

  18. Re: Avoiding unnecessary reads in recovery

    Simon Riggs <simon@2ndquadrant.com> — 2007-04-27T17:45:38Z

    On Fri, 2007-04-27 at 10:37 -0400, Bruce Momjian wrote:
> I assume this is 8.4 material.

I think its a small enough, performance-only change to allow it at this
time. It will provide considerable additional benefit for Warm Standby
servers.

-- 
  Simon Riggs             
  EnterpriseDB   http://www.enterprisedb.com

  19. Re: Avoiding unnecessary reads in recovery

    Decibel! <decibel@decibel.org> — 2007-04-27T17:59:24Z

    On Apr 26, 2007, at 3:39 PM, Tom Lane wrote:
> Jim Nasby <decibel@decibel.org> writes:
>> So what happens if a backend is running with full_page_writes = off,
>> someone edits postgresql.conf to turns it on and forgets to reload/
>> restart, and then we crash? You'll come up in recovery mode thinking
>> that f_p_w was turned on, when in fact it wasn't.
>
> One of the advantages of the proposed patch is that it avoids  
> having to
> make any assumptions like that.

Yeah, the only email in the thread when I wrote that was Heikki's  
original email (I wrote the email on a plane, but did note it had  
been some time after the original email with no replies). Sorry for  
the noise.
--
Jim Nasby                                            jim@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

  20. Re: Avoiding unnecessary reads in recovery

    Simon Riggs <simon@2ndquadrant.com> — 2007-04-28T10:13:19Z

    On Fri, 2007-04-27 at 12:22 +0100, Heikki Linnakangas wrote:
> Tom Lane wrote:
> > "Simon Riggs" <simon@2ndquadrant.com> writes:
> >> As regards the zero_damaged_pages question, I raised that some time ago
> >> but we didn't arrive at an explicit answer. All I would say is we can't
> >> allow invalid pages in the buffer manager at any time, whatever options
> >> we have requested, otherwise other code will fail almost immediately.
> > 
> > Yeah --- the proposed new bufmgr routine should probably explicitly zero
> > the content of the buffer.  It doesn't really matter in the context of
> > WAL recovery, since there can't be any concurrent access to the buffer,
> > but it'd make it safe to use in non-WAL contexts (I think there are
> > other places where we know we are going to init the page and so a
> > physical read is a waste of time).  
> 
> To implement that correctly, I think we'd need to take the content lock 
> to clear the buffer if it's already found in the cache. It doesn't seem 
> right to me for the buffer manager to do that, in the worst case it 
> could lead to deadlocks if that function was ever used while holding 
> another buffer locked.
> 
> What we could have is the semantics of "Return a buffer, with either 
> correct contents or completely zeroed out". It would act just like 
> ReadBuffer if the buffer was already in memory, and zero out the page 
> otherwise. That's a bit strange semantics to have, but is simple to 
> implement and works for the use-cases we've been talking about.

Sounds good.

> Patch implementing that attached. I named the function "ReadOrZeroBuffer".

We already have an API quirk similar to this: relation extension. It
seems strange to have two different kinds of special case API that are
used alongside each other in XLogReadBuffer()

Currently if we extend by a block we say
	buffer = ReadBuffer(reln, P_NEW);

Why not just add another option, so where you use ReadOrZeroBuffer we
just say
	buffer = ReadBuffer(reln, P_INIT);

which we then check for on entry by saying
	isInit = (blockNum == P_INIT);
just as we already do for P_NEW

That way you can do the code like this
	if (isExtend || isInit)
	{
		/* new or initialised buffers are zero-filled */
		MemSet((char *) bufBlock, 0, BLCKSZ);
		if (isExtend)
			smgrextend(reln->rd_smgr, blockNum, 
				   (char *) bufBlock,
				   reln->rd_istemp);
	}

That way we don't have to have ReadBuffer_common etc..

-- 
  Simon Riggs             
  EnterpriseDB   http://www.enterprisedb.com

  21. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-28T10:20:52Z

    Simon Riggs wrote:
> On Fri, 2007-04-27 at 12:22 +0100, Heikki Linnakangas wrote:
>> Patch implementing that attached. I named the function "ReadOrZeroBuffer".
> 
> We already have an API quirk similar to this: relation extension. It
> seems strange to have two different kinds of special case API that are
> used alongside each other in XLogReadBuffer()
> 
> Currently if we extend by a block we say
> 	buffer = ReadBuffer(reln, P_NEW);
> 
> Why not just add another option, so where you use ReadOrZeroBuffer we
> just say
> 	buffer = ReadBuffer(reln, P_INIT);

Because ReadOrZeroBuffer needs the block number as an argument.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  22. Re: Avoiding unnecessary reads in recovery

    Heikki Linnakangas <heikki@enterprisedb.com> — 2007-04-28T10:22:59Z

    I was actually thinking that we could slip this in 8.3. It's a simple, 
well-understood patch, which fixes a little data integrity quirk as well 
as gives a nice recovery speed up.

Bruce Momjian wrote:
> I assume this is 8.4 material.
> 
> ---------------------------------------------------------------------------
> 
> Heikki Linnakangas wrote:
>> Tom Lane wrote:
>>> "Simon Riggs" <simon@2ndquadrant.com> writes:
>>>> As regards the zero_damaged_pages question, I raised that some time ago
>>>> but we didn't arrive at an explicit answer. All I would say is we can't
>>>> allow invalid pages in the buffer manager at any time, whatever options
>>>> we have requested, otherwise other code will fail almost immediately.
>>> Yeah --- the proposed new bufmgr routine should probably explicitly zero
>>> the content of the buffer.  It doesn't really matter in the context of
>>> WAL recovery, since there can't be any concurrent access to the buffer,
>>> but it'd make it safe to use in non-WAL contexts (I think there are
>>> other places where we know we are going to init the page and so a
>>> physical read is a waste of time).  
>> To implement that correctly, I think we'd need to take the content lock 
>> to clear the buffer if it's already found in the cache. It doesn't seem 
>> right to me for the buffer manager to do that, in the worst case it 
>> could lead to deadlocks if that function was ever used while holding 
>> another buffer locked.
>>
>> What we could have is the semantics of "Return a buffer, with either 
>> correct contents or completely zeroed out". It would act just like 
>> ReadBuffer if the buffer was already in memory, and zero out the page 
>> otherwise. That's a bit strange semantics to have, but is simple to 
>> implement and works for the use-cases we've been talking about.
>>
>> Patch implementing that attached. I named the function "ReadOrZeroBuffer".
>>
>> -- 
>>    Heikki Linnakangas
>>    EnterpriseDB   http://www.enterprisedb.com
> 
> 
>> ---------------------------(end of broadcast)---------------------------
>> TIP 6: explain analyze is your friend
> 


-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  23. Re: Avoiding unnecessary reads in recovery

    Tom Lane <tgl@sss.pgh.pa.us> — 2007-04-28T16:56:06Z

    Heikki Linnakangas <heikki@enterprisedb.com> writes:
> I was actually thinking that we could slip this in 8.3. It's a simple, 
> well-understood patch, which fixes a little data integrity quirk as well 
> as gives a nice recovery speed up.

Yeah.  It's arguably a bug fix, in fact, since it eliminates the issue
that the recovery behavior is wrong if full-page-writes had been off
when the WAL log was made.

			regards, tom lane

  24. Re: Avoiding unnecessary reads in recovery

    Tom Lane <tgl@sss.pgh.pa.us> — 2007-05-02T23:26:27Z

    Heikki Linnakangas <heikki@enterprisedb.com> writes:
> What we could have is the semantics of "Return a buffer, with either 
> correct contents or completely zeroed out". It would act just like 
> ReadBuffer if the buffer was already in memory, and zero out the page 
> otherwise. That's a bit strange semantics to have, but is simple to 
> implement and works for the use-cases we've been talking about.

> Patch implementing that attached. I named the function "ReadOrZeroBuffer".

Applied.  BTW, I realized that there is a potential issue created by
this, which is that the smgr level might see a write for a page that it
never saw a read for.  I don't think there are any bad consequences of
this ATM, but it is skating around the edges of some bugs we've had
previously with relation extension.  In particular ReadOrZeroBuffer
avoids the error that would normally occur if one tries to read a page
that's beyond the logical EOF; and if the page is subsequently modified
and written, md.c is likely to get confused/unhappy, particularly if the
page is beyond the next segment boundary.  This isn't a problem in
XLogReadBuffer's usage because it carefully checks the EOF position
before trying to use ReadOrZeroBuffer, but it's a limitation other
callers will need to think about.

			regards, tom lane