On 08/15/2014 10:26 AM, Heikki Linnakangas wrote:
On 08/15/2014 02:02 AM, Alvaro Herrera wrote:
Alvaro Herrera wrote:
Heikki Linnakangas wrote:
I'm sure this still needs some cleanup, but here's the patch, based
on your v14. Now that I know what this approach looks like, I still
like it much better. The insert and update code is somewhat more
complicated, because you have to be careful to lock the old page,
new page, and revmap page in the right order. But it's not too bad,
and it gets rid of all the complexity in vacuum.
It seems there is some issue here, because pageinspect tells me the
index is not growing properly for some reason. minmax_revmap_data gives
me this array of TIDs after a bunch of insert/vacuum/delete/ etc:
I fixed this issue, and did a lot more rework and bugfixing. Here's
v15, based on v14-heikki2.
Thanks!
I think remaining issues are mostly minimal (pageinspect should output
block number alongside each tuple, now that we have it, for example.)
There's this one issue I left in my patch version that I think we should
do something about:
+ /*
+ * No luck. Assume that the revmap was updated concurrently.
+ *
+ * XXX: it would be nice to add some kind of a sanity check
here to
+ * avoid looping infinitely, if the revmap points to wrong
tuple for
+ * some reason.
+ */
This happens when we follow the revmap to a tuple, but find that the
tuple points to a different block than what the revmap claimed.
Currently, we just assume that it's because the tuple was updated
concurrently, but while hacking, I frequently had a broken index where
the revmap pointed to bogus tuples or the tuples had a missing/wrong
block number on them, and ran into infinite loop here. It's clearly a
case of a corrupt index and shouldn't happen, but I would imagine that
it's a fairly typical way this would fail in production too because of
hardware issues or bugs. So I think we need to work a bit harder to stop
the looping and throw an error instead.
Perhaps something as simple as keeping a loop counter and giving up
after 1000 attempts would be good enough. The window between releasing
the lock on the revmap, and acquiring the lock on the page containing
the MMTuple is very narrow, so the chances of losing that race to a
concurrent update more than 1-2 times in a row is vanishingly small.
Reading the patch more closely, I see that you added a check that when
we loop, we throw an error if the new item pointer in the revmap is the
same as before. In theory, it's possible that two concurrent updates
happen: one that moves the tuple we're looking for elsewhere, and
another that moves it back again. The probability of that is also
vanishingly small, so maybe that's OK. Or we could check the LSN; if the
revmap has been updated, its LSN must've changed.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
