Обсуждение: REINDEX vs broken HOT chains, redux
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
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
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?
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
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
On Tue, Apr 19, 2011 at 4:29 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Greg Stark <gsstark@mit.edu> writes:
> On Tue, Apr 19, 2011 at 4:29 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Wed, Apr 20, 2011 at 3:02 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Greg Stark <gsstark@mit.edu> writes:
> On Wed, Apr 20, 2011 at 3:02 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Wed, Apr 20, 2011 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Greg Stark <gsstark@mit.edu> writes:
> On Wed, Apr 20, 2011 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> 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