CacheInvalidateRelcache in btree is a crummy idea

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: CacheInvalidateRelcache in btree is a crummy idea
Date: 2014-02-02 21:45:20
Message-ID: 28062.1391377520@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

While investigating the assertion failure Noah presented at
http://www.postgresql.org/message-id/20130805170931.GA369289@tornado.leadboat.com
I was confused for a bit as to why we're getting a relcache rebuild
request for t1's index at exit of the subtransaction, when the
subtransaction hasn't made any change to the index's schema.
Investigation showed that the reason is that _bt_getroot() creates
a root page for the index during the subtransaction's INSERT, and
then it does CacheInvalidateRelcache() to notify other backends
that it's updated the index's metapage.

Now, this is a dumb idea, because relcache invalidation is assumed
to be a transactional operation, which means that if the subtransaction
rolls back, we don't send any invalidation event to other backends.
Update of a btree metapage, however, is not a transactional thing.
So if we actually *needed* that notification to be sent, this would be
a bug.

But we don't need that. _bt_getroot() is the primary user of the
cached metapage, and it sufficiently verifies that the page it's
reached is the right one, which is why we've not seen bug reports.
_bt_getrootheight() also uses the cached data, but that's only for a
statistical estimate, so it doesn't matter if it's a bit stale.

Moreover, if we did need reliable transmission of the invalidation
message, that would've gotten broken even more by commit 96675bff1, which
suppressed doing that when InRecovery. (In fairness, it wasn't a problem
at the time since that was pre-Hot Standby. But the lack of notification
would be an issue now for hot standby sessions, if they needed one.)

So I'm thinking my commit d2896a9ed, which introduced this mechanism,
was poorly thought out and we should just remove the relcache invals
as per the attached patch. Letting _bt_getroot() update the cached
metapage at next use should be a lot cheaper than a full relcache
rebuild for the index.

Note: this change will mean that Noah's test case no longer triggers
the problem discussed in that thread ... but I'm sure we can find
another test case for that.

regards, tom lane

Attachment Content-Type Size
no-relcache-flush-for-metapage-update.patch text/x-diff 5.6 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2014-02-02 21:53:12 Re: [COMMITTERS] pgsql: Include planning time in EXPLAIN ANALYZE output.
Previous Message Gavin Flower 2014-02-02 21:11:49 Re: [HACKERS] pgsql: Include planning time in EXPLAIN ANALYZE output.