Last week we fixed a problem in which REINDEX could corrupt pg_index's
own indexes by forbidding it from setting indcheckxmin on a system
catalog's index. While thinking about bug #5985 I realized that there's
a better, more general solution. Namely, that when reindexing an
existing index, there cannot be any need to advance the index's
indcheckxmin horizon. The existing code just blindly pushes the horizon
forward to current time if it finds any possibly-broken HOT chains ---
but if the index existed before, then any HOT chains that are actually
broken with respect to it must predate its existing horizon.

Therefore, when reindexing an existing index, we should never set
indcheckxmin if it wasn't set before. In particular, this rule fixes
the previous issue for system catalogs, which are certain to not have
had indcheckxmin set when initdb made them.

The existing code in index_build also forcibly updates the pg_index
row even if indcheckxmin was already set, thus pushing the row'x xmin
forward and moving the index's usability horizon. This effect isn't
desirable either.

In short, the entire update of pg_index in index_build is unwanted when
reindexing an existing index. index_build doesn't currently know
whether it's being called for a new index or a reindex operation,
but it wouldn't be hard to pass down a flag for that.

I'm intending to revert last week's patch in favor of this approach,
at least in HEAD. It'll be slightly more invasive than the previous
patch because of the API change for index_build, so I'm not sure whether
to back-patch or not --- comments?

regards, tom lane

Search Discussions

  • Alvaro Herrera at Apr 19, 2011 at 4:58 pm

    Excerpts from Tom Lane's message of mar abr 19 12:29:04 -0300 2011:
    Last week we fixed a problem in which REINDEX could corrupt pg_index's
    own indexes by forbidding it from setting indcheckxmin on a system
    catalog's index. While thinking about bug #5985 I realized that there's
    a better, more general solution. Namely, that when reindexing an
    existing index, there cannot be any need to advance the index's
    indcheckxmin horizon. The existing code just blindly pushes the horizon
    forward to current time if it finds any possibly-broken HOT chains ---
    but if the index existed before, then any HOT chains that are actually
    broken with respect to it must predate its existing horizon.

    Therefore, when reindexing an existing index, we should never set
    indcheckxmin if it wasn't set before. In particular, this rule fixes
    the previous issue for system catalogs, which are certain to not have
    had indcheckxmin set when initdb made them.
    Interesting.
    In short, the entire update of pg_index in index_build is unwanted when
    reindexing an existing index. index_build doesn't currently know
    whether it's being called for a new index or a reindex operation,
    but it wouldn't be hard to pass down a flag for that.

    I'm intending to revert last week's patch in favor of this approach,
    at least in HEAD. It'll be slightly more invasive than the previous
    patch because of the API change for index_build, so I'm not sure whether
    to back-patch or not --- comments?
    Maybe add a new function index_build_ext that has the API change, and
    keep the existing index_build as a wrapper that keeps the current
    behavior. In HEAD just change the API of index_build and make
    index_build_ext a macro on top of the function (or just make it
    disappear.)

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Apr 19, 2011 at 5:12 pm

    Alvaro Herrera writes:
    Excerpts from Tom Lane's message of mar abr 19 12:29:04 -0300 2011:
    I'm intending to revert last week's patch in favor of this approach,
    at least in HEAD. It'll be slightly more invasive than the previous
    patch because of the API change for index_build, so I'm not sure whether
    to back-patch or not --- comments?
    Maybe add a new function index_build_ext that has the API change, and
    keep the existing index_build as a wrapper that keeps the current
    behavior. In HEAD just change the API of index_build and make
    index_build_ext a macro on top of the function (or just make it
    disappear.)
    Not sure it's worth that amount of trouble. index_build is pretty far
    down in the nest of code that manages index (re)building --- is it at
    all likely that third-party code is calling it directly?

    And even more to the point, if there is such third-party code, we don't
    want the fix to fail to operate when a reindex is invoked through that
    code path rather than the core paths. So if you think there's a
    realistic risk of this, we probably shouldn't back-patch.

    regards, tom lane
  • Alvaro Herrera at Apr 19, 2011 at 5:28 pm

    Excerpts from Tom Lane's message of mar abr 19 14:12:46 -0300 2011:
    Alvaro Herrera <alvherre@commandprompt.com> writes:
    Excerpts from Tom Lane's message of mar abr 19 12:29:04 -0300 2011:
    I'm intending to revert last week's patch in favor of this approach,
    at least in HEAD. It'll be slightly more invasive than the previous
    patch because of the API change for index_build, so I'm not sure whether
    to back-patch or not --- comments?
    Maybe add a new function index_build_ext that has the API change, and
    keep the existing index_build as a wrapper that keeps the current
    behavior. In HEAD just change the API of index_build and make
    index_build_ext a macro on top of the function (or just make it
    disappear.)
    Not sure it's worth that amount of trouble. index_build is pretty far
    down in the nest of code that manages index (re)building --- is it at
    all likely that third-party code is calling it directly?
    Then why bother keeping the API unchanged? If you're correct, it would
    be pointless.
    And even more to the point, if there is such third-party code, we don't
    want the fix to fail to operate when a reindex is invoked through that
    code path rather than the core paths. So if you think there's a
    realistic risk of this, we probably shouldn't back-patch.
    After actually having a look at the API, I don't.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Greg Stark at Apr 20, 2011 at 5:17 am

    On Tue, Apr 19, 2011 at 4:29 PM, Tom Lane wrote:
    Namely, that when reindexing an
    existing index, there cannot be any need to advance the index's
    indcheckxmin horizon.
    Note that if isvalid is not set then we don't know anything about the
    hot chains since the concurrent index build never finished.

    I'm also a bit concerned since the part of the use case of REINDEX is
    for handling precisely the situations where the index is corrupt. If I
    change the code for my user-defined data type and knowingly break the
    semantics of the btree op, I might reasonably expect a REINDEX to fix
    it up. ((I don't recall if we went with binary equality or btree
    equality for determining of updates are eligible for hot updates or
    not though.)

    --
    greg
  • Tom Lane at Apr 20, 2011 at 2:02 pm

    Greg Stark writes:
    On Tue, Apr 19, 2011 at 4:29 PM, Tom Lane wrote:
    Namely, that when reindexing an
    existing index, there cannot be any need to advance the index's
    indcheckxmin horizon.
    Note that if isvalid is not set then we don't know anything about the
    hot chains since the concurrent index build never finished.
    Hmm, good point. We can probably handle this by tweaking the logic in
    reindex_index that changes indisvalid so that it will force indcheckxmin
    on when indisvalid had been false and there were any possibly-broken
    HOT chains.

    regards, tom lane
  • Greg Stark at Apr 20, 2011 at 2:21 pm

    On Wed, Apr 20, 2011 at 3:02 PM, Tom Lane wrote:
    Hmm, good point.  We can probably handle this by tweaking the logic in
    reindex_index that changes indisvalid so that it will force indcheckxmin
    on when indisvalid had been false and there were any possibly-broken
    HOT chains.
    I'm not following. You mean set indcheckxmin on the first phase when
    we create the row with indisvalid false? Then when we set indisvalid
    to true also clear indcheckxmin (or just leave it since we would have
    waited out that xmin anyways)?

    Seems like a reasonable thing to do just to make the invariant on
    indcheckxmin simpler. But equally you could just make the check for
    the optimization just check indisvalid && xmin > indcheckxmin. It's
    always safe to bump indcheckxmin except for the pg_index case at hand,
    just unnecessary sometimes.

    I kind of wonder why you like this optimization better than the
    bright-line "never update indcheckxmin on system table indexes" rule.
    That seems to depend on a lot less subtle reasoning about system
    tables not being built with create index concurrently etc. With the
    simple rule we could have an elog() any time a broken hot chain is
    detected in a system table when rebuilding an index and then it would
    be easy enough to verify the correctness of the code by local
    inspection instead of depending on understanding how the last index
    built or rebuild might have set indcheckxmin on system indexes.

    I like the optimization since it reduces the occurrency of the
    indcheckxmin weirdness but I dislike counting on it for correctness on
    pg_index.


    --
    greg
  • Tom Lane at Apr 20, 2011 at 2:36 pm

    Greg Stark writes:
    On Wed, Apr 20, 2011 at 3:02 PM, Tom Lane wrote:
    Hmm, good point.  We can probably handle this by tweaking the logic in
    reindex_index that changes indisvalid so that it will force indcheckxmin
    on when indisvalid had been false and there were any possibly-broken
    HOT chains.
    I'm not following. You mean set indcheckxmin on the first phase when
    we create the row with indisvalid false? Then when we set indisvalid
    to true also clear indcheckxmin (or just leave it since we would have
    waited out that xmin anyways)?
    No, I'm talking about the code at the bottom of reindex_index() that
    will set indisvalid true at the end of a regular not-concurrent REINDEX
    operation. If we had REINDEX CONCURRENTLY, it might have to address
    this issue in some other fashion, but we don't and I don't desire to
    design how it would work right now.
    I kind of wonder why you like this optimization better than the
    bright-line "never update indcheckxmin on system table indexes" rule.
    That's not a "bright line" so much as a hack. System indexes really
    shouldn't be that much different from ordinary indexes. The property
    we actually are relying on is that there can't be any HOT chains that
    break the index, because it existed before any updates could have
    happened. I think the new approach is a more direct implementation of
    that concept than the original.

    regards, tom lane
  • Greg Stark at Apr 20, 2011 at 3:46 pm

    On Wed, Apr 20, 2011 at 3:35 PM, Tom Lane wrote:
    System indexes really
    shouldn't be that much different from ordinary indexes.  The property
    we actually are relying on is that there can't be any HOT chains that
    break the index, because it existed before any updates could have
    happened.  I think the new approach is a more direct implementation of
    that concept than the original.
    The problem was caused by a recursive update to pg_index. We need to
    somehow ensure that update doesn't happen. We can either rely on this
    subtle property we've established is true today but depends on lots of
    fiddly bits of behaviour throughout the system or we can insert a line
    saying "just don't do that".

    I suppose it doesn't matter as long as there are the new assertion
    checks (perhaps they should be elog()s. Since if it ever happens at
    least we won't corrupt the database and we'll detect that the logic no
    longer holds.


    --
    greg
  • Tom Lane at Apr 20, 2011 at 4:03 pm

    Greg Stark writes:
    On Wed, Apr 20, 2011 at 3:35 PM, Tom Lane wrote:
    System indexes really
    shouldn't be that much different from ordinary indexes.  The property
    we actually are relying on is that there can't be any HOT chains that
    break the index, because it existed before any updates could have
    happened.  I think the new approach is a more direct implementation of
    that concept than the original.
    The problem was caused by a recursive update to pg_index. We need to
    somehow ensure that update doesn't happen. We can either rely on this
    subtle property we've established is true today but depends on lots of
    fiddly bits of behaviour throughout the system or we can insert a line
    saying "just don't do that".
    The problem with just adding a line saying "don't do that" is that it'll
    fail (in a different way from the current problem) if there's ever a
    situation where you *do* need it to do that. So I don't find that way
    to be any more future-proof. In particular, the previous fix
    essentially broke any attempt to add an index to a system catalog after
    initdb, since it was highly likely to result in falsely labeling the new
    index as not-indcheckxmin. We know that people do try to add new
    indexes to catalogs, so I wasn't pleased at all with that behavior of
    the previous patch --- but I didn't see another solution at the time.

    With the new patch you can still get screwed if you add an index to
    pg_index (and it's indcheckxmin and then you REINDEX it) --- but that's
    a much smaller bug surface, and it doesn't intersect any use cases I've
    heard of.

    regards, tom lane

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedApr 19, '11 at 3:37p
activeApr 20, '11 at 4:03p
posts10
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase