Hi,

I noticed that heap_tuple_needs_freeze might return true in cases where
the Xmax is leftover junk from somebody who set HEAP_XMAX_INVALID in the
far past without resetting the Xmax value itself to Invalid. I think
this is incorrect usage; the rule, I think, is that one shouldn't even
read Xmax at all unless HEAP_XMAX_INVALID is reset.

This might cause unnecessary acquisitions of the cleanup lock, if a
tuple is deemed freezable when in fact it isn't.

Suggested patch attached. I'd backpatch this as far as it applies
cleanly.

--
Álvaro Herrera <alvherre@alvh.no-ip.org>

Search Discussions

  • Tom Lane at Feb 2, 2012 at 4:04 am

    Alvaro Herrera writes:
    ! if (!(tuple->t_infomask & HEAP_XMAX_INVALID))
    {
    ! if (!(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
    How about just one test,

    if (!(tuple->t_infomask & (HEAP_XMAX_INVALID | HEAP_XMAX_IS_MULTI)))

    But other than that quibble, yeah, it's a bug. XMAX_INVALID means just
    that: the xmax is not to be thought valid.

    regards, tom lane
  • Robert Haas at Feb 2, 2012 at 2:31 pm

    On Wed, Feb 1, 2012 at 8:01 PM, Alvaro Herrera wrote:
    Suggested patch attached.  I'd backpatch this as far as it applies
    cleanly.
    This is new code in 9.2, but it's modelled on heap_freeze_tuple(), which is old.

    I'm not convinced that it's a bug. Suppose that xmax is set but is
    hinted as invalid. We process the table and advanced relfrozenxid;
    then, we crash. After recovery, it's possible that the hint bit is
    gone (after all, setting hint bits isn't WAL-logged). Now we're in
    big trouble, because the next CLOG lookup on that xmax value might not
    happen until it's been reused, and we might get a different answer
    than before.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Tom Lane at Feb 2, 2012 at 4:27 pm

    Robert Haas writes:
    I'm not convinced that it's a bug. Suppose that xmax is set but is
    hinted as invalid.
    XMAX_INVALID is not a "hint". When it's set, the contents of the field
    must be presumed to be garbage. Any code failing to adhere to that rule
    is broken.
    We process the table and advanced relfrozenxid;
    then, we crash. After recovery, it's possible that the hint bit is
    gone (after all, setting hint bits isn't WAL-logged). Now we're in
    big trouble, because the next CLOG lookup on that xmax value might not
    happen until it's been reused, and we might get a different answer
    than before.
    I believe we have adequate defenses against that, and even if we did
    not, that doesn't make the code in question less wrong.

    regards, tom lane
  • Robert Haas at Feb 2, 2012 at 5:45 pm

    On Thu, Feb 2, 2012 at 11:27 AM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    I'm not convinced that it's a bug.  Suppose that xmax is set but is
    hinted as invalid.
    XMAX_INVALID is not a "hint".  When it's set, the contents of the field
    must be presumed to be garbage.  Any code failing to adhere to that rule
    is broken.
    We process the table and advanced relfrozenxid;
    then, we crash.  After recovery, it's possible that the hint bit is
    gone (after all, setting hint bits isn't WAL-logged).  Now we're in
    big trouble, because the next CLOG lookup on that xmax value might not
    happen until it's been reused, and we might get a different answer
    than before.
    I believe we have adequate defenses against that, and even if we did
    not, that doesn't make the code in question less wrong.
    I believe the adequate defense that we have is precisely the logic you
    are proposing to change. Regardless of whether you want to call
    XMAX_INVALID a hint or, say, a giant tortoise, I am fairly sure that
    we don't WAL-log setting it. That means that a bit set before a crash
    won't necessarily still be set after a crash. But the corresponding
    relfrozenxid advancement will be WAL-logged, leading to the problem
    scenario I described.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Robert Haas at Feb 2, 2012 at 5:55 pm

    On Thu, Feb 2, 2012 at 12:45 PM, Robert Haas wrote:
    I believe the adequate defense that we have is precisely the logic you
    are proposing to change.  Regardless of whether you want to call
    XMAX_INVALID a hint or, say, a giant tortoise, I am fairly sure that
    we don't WAL-log setting it.  That means that a bit set before a crash
    won't necessarily still be set after a crash.  But the corresponding
    relfrozenxid advancement will be WAL-logged, leading to the problem
    scenario I described.
    To put that another way, the problem isn't that we might have code
    somewhere in the system that ignores HEAP_XMAX_INVALID. The problem
    is that HEAP_XMAX_INVALID might not still be set on that tuple the
    next time somebody looks at it, if a database crash intervenes after
    that bit is set and before it is flushed to disk.

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedFeb 2, '12 at 1:01a
activeFeb 2, '12 at 5:55p
posts6
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase