Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Rearrange lazy_scan_heap to avoid visibility map race conditions.

  1. buffer locking fix for lazy_scan_heap

    Robert Haas <robertmhaas@gmail.com> — 2012-04-18T20:09:45Z

    I discovered when researching the issue of index-only scans and Hot
Standby that there's a bug (for which I'm responsible) in
lazy_scan_heap[1].  Actually, the code has been like this for a long
time, but I needed to change it when I did the crash-safe visibility
map work, and I didn't.  The problem is that lazy_scan_heap() releases
the lock on the buffer it's whacking around before it sets the
visibility map bit.  Thus, it's possible for the page-level bit to be
cleared by some other backend before the visibility map bit gets set.
In previous releases that was arguably OK, since the worst thing that
could happen is a postponement of vacuuming on that page until the
next anti-wraparound cycle; but now that we have index-only scans, it
can cause queries to return wrong answers.

Attached is a patch to fix the problem, which rearranges things so
that we set the bit in the visibility map before releasing the buffer
lock.  Similar work has already been done for inserts, updates, and
deletes, which in previous releases would *clear* the visibility map
bit after releasing the page lock, and no longer done.  But the vacuum
case, which *sets* the bit, was overlooked.  Our convention in those
related cases is that it's acceptable to hold the buffer lock while
setting the visibility map bit and issuing the WAL log record, but
there must be no possibility of an I/O to read in the visibility map
page while the buffer lock is held.  This turned out to be pretty
simple because, as it turns out, lazy_scan_heap() is almost always
holding a pin on the correct page anyway, so I only had to tweak
things slightly to guarantee it.  As a pleasant side effect, the new
code is actually quite a bit simpler and more readable than the old
code, at least IMHO.

While I was at it, I made a couple of minor, related changes which I
believe to be improvements.  First, I arranged things so that, when we
pause the first vacuum pass to do an index vac cycle, we release any
buffer pin we're holding on the visibility map page.  The fact that we
haven't done that in the past is probably harmless, but I think
there's something to be said for not holding onto buffer pins for long
periods of time when it isn't really necessary.  Second, I adjusted
things so that we print a warning if the visibility map bit is set and
the page-level bit is clear, since that should never happen in 9.2+.
It is similar to the existing warning which catches the case where a
page is marked all-visible despite containing dead tuples.

Comments?

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

http://archives.postgresql.org/pgsql-hackers/2012-04/msg00682.php

  2. Re: buffer locking fix for lazy_scan_heap

    高增琦 <pgf00a@gmail.com> — 2015-08-03T04:03:55Z

    Hi,

sorry for asking this really old commit.

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7ab9b2f3b79177e501a1ef90ed004cc68788abaf

I could not understand why "releases the lock on the buffer" is
a problem since vacuum will lock and check the page bit again before
set the vm. Did I missed something?

Thanks for your help.

On Thu, Apr 19, 2012 at 4:09 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> I discovered when researching the issue of index-only scans and Hot
> Standby that there's a bug (for which I'm responsible) in
> lazy_scan_heap[1].  Actually, the code has been like this for a long
> time, but I needed to change it when I did the crash-safe visibility
> map work, and I didn't.  The problem is that lazy_scan_heap() releases
> the lock on the buffer it's whacking around before it sets the
> visibility map bit.  Thus, it's possible for the page-level bit to be
> cleared by some other backend before the visibility map bit gets set.
> In previous releases that was arguably OK, since the worst thing that
> could happen is a postponement of vacuuming on that page until the
> next anti-wraparound cycle; but now that we have index-only scans, it
> can cause queries to return wrong answers.
>
> Attached is a patch to fix the problem, which rearranges things so
> that we set the bit in the visibility map before releasing the buffer
> lock.  Similar work has already been done for inserts, updates, and
> deletes, which in previous releases would *clear* the visibility map
> bit after releasing the page lock, and no longer done.  But the vacuum
> case, which *sets* the bit, was overlooked.  Our convention in those
> related cases is that it's acceptable to hold the buffer lock while
> setting the visibility map bit and issuing the WAL log record, but
> there must be no possibility of an I/O to read in the visibility map
> page while the buffer lock is held.  This turned out to be pretty
> simple because, as it turns out, lazy_scan_heap() is almost always
> holding a pin on the correct page anyway, so I only had to tweak
> things slightly to guarantee it.  As a pleasant side effect, the new
> code is actually quite a bit simpler and more readable than the old
> code, at least IMHO.
>
> While I was at it, I made a couple of minor, related changes which I
> believe to be improvements.  First, I arranged things so that, when we
> pause the first vacuum pass to do an index vac cycle, we release any
> buffer pin we're holding on the visibility map page.  The fact that we
> haven't done that in the past is probably harmless, but I think
> there's something to be said for not holding onto buffer pins for long
> periods of time when it isn't really necessary.  Second, I adjusted
> things so that we print a warning if the visibility map bit is set and
> the page-level bit is clear, since that should never happen in 9.2+.
> It is similar to the existing warning which catches the case where a
> page is marked all-visible despite containing dead tuples.
>
> Comments?
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> http://archives.postgresql.org/pgsql-hackers/2012-04/msg00682.php
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>


-- 
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

  3. Re: buffer locking fix for lazy_scan_heap

    Robert Haas <robertmhaas@gmail.com> — 2015-08-04T01:19:34Z

    On Mon, Aug 3, 2015 at 12:03 AM, 高增琦 <pgf00a@gmail.com> wrote:
> sorry for asking this really old commit.
>
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7ab9b2f3b79177e501a1ef90ed004cc68788abaf
>
> I could not understand why "releases the lock on the buffer" is
> a problem since vacuum will lock and check the page bit again before
> set the vm. Did I missed something?

Hmm, you might be right.  It certainly seems safer to do all the work
without releasing the buffer lock temporarily in the middle, but maybe
the old way wasn't actually broken.

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