Re: [COMMITTERS] pgsql: Avoid marking buffer dirty when VACUUM has no work to do.

Avoid marking buffer dirty when VACUUM has no work to do.
When wal_level = 'hot_standby' we touched the last page of the
relation during a VACUUM, even if nothing else had happened.
That would alter the LSN of the last block and set the mtime
of the relation file unnecessarily. Noted by Thom Brown.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/c1458cc495ff800cd176a1c2e56d8b62680d9b71

Modified Files
--------------
src/backend/access/nbtree/nbtpage.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> Avoid marking buffer dirty when VACUUM has no work to do.
> When wal_level = 'hot_standby' we touched the last page of the
> relation during a VACUUM, even if nothing else had happened.
> That would alter the LSN of the last block and set the mtime
> of the relation file unnecessarily. Noted by Thom Brown.

This doesn't look right to me --- you have not accounted for the
possibility that btpo_cycleid or BTP_HAS_GARBAGE is changed.

Also, I'm confused about the business of not setting the LSN. Thom
claimed that he was seeing the page not change at all (or at least
md5sum of the file didn't change) despite mtime changing. If we'd
been plastering a new LSN on the page each time, then that should
certainly not have been possible. So I now think maybe we've
mis-analyzed what was happening in his example.

I think this requires more careful analysis.

regards, tom lane

I wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> Avoid marking buffer dirty when VACUUM has no work to do.
>> When wal_level = 'hot_standby' we touched the last page of the
>> relation during a VACUUM, even if nothing else had happened.
>> That would alter the LSN of the last block and set the mtime
>> of the relation file unnecessarily. Noted by Thom Brown.

> This doesn't look right to me --- you have not accounted for the
> possibility that btpo_cycleid or BTP_HAS_GARBAGE is changed.

> Also, I'm confused about the business of not setting the LSN. Thom
> claimed that he was seeing the page not change at all (or at least
> md5sum of the file didn't change) despite mtime changing. If we'd
> been plastering a new LSN on the page each time, then that should
> certainly not have been possible. So I now think maybe we've
> mis-analyzed what was happening in his example.

> I think this requires more careful analysis.

Ping? If you don't respond, I'm going to take it on my own authority to
revert this patch, because it's definitely broken as-is, and I don't
think the consequences of not updating the page LSN have been thought
through either.

regards, tom lane

On Tue, Nov 22, 2011 at 2:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>>> Avoid marking buffer dirty when VACUUM has no work to do.
>>> When wal_level = 'hot_standby' we touched the last page of the
>>> relation during a VACUUM, even if nothing else had happened.
>>> That would alter the LSN of the last block and set the mtime
>>> of the relation file unnecessarily. Noted by Thom Brown.
>
>> This doesn't look right to me --- you have not accounted for the
>> possibility that btpo_cycleid or BTP_HAS_GARBAGE is changed.
>
>> Also, I'm confused about the business of not setting the LSN.  Thom
>> claimed that he was seeing the page not change at all (or at least
>> md5sum of the file didn't change) despite mtime changing.  If we'd
>> been plastering a new LSN on the page each time, then that should
>> certainly not have been possible.  So I now think maybe we've
>> mis-analyzed what was happening in his example.
>
>> I think this requires more careful analysis.
>
> Ping?  If you don't respond, I'm going to take it on my own authority to
> revert this patch, because it's definitely broken as-is, and I don't
> think the consequences of not updating the page LSN have been thought
> through either.

Tom, waiting across a weekend isn't a cause for concern.

I made that change for you, so am happy to revoke for you also.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services