Re: ALTER TABLE lock strength reduction patch is unsafe

On Sun, Jun 19, 2011 at 5:13 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Fri, Jun 17, 2011 at 11:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Jun 16, 2011 at 6:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> 4. Backend #2 visits the new, about-to-be-committed version of
>>>> pgbench_accounts' pg_class row just before backend #3 commits.
>>>> It sees the row as not good and keeps scanning.  By the time it
>>>> reaches the previous version of the row, however, backend #3
>>>> *has* committed.  So that version isn't good according to SnapshotNow
>>>> either.
>>
>>> <thinks some more>
>>
>>> Why isn't this a danger for every pg_class update?  For example, it
>>> would seem that if VACUUM updates relpages/reltuples, it would be
>>> prone to this same hazard.
>>
>> VACUUM does that with an in-place, nontransactional update.  But yes,
>> this is a risk for every transactional catalog update.
>
> Well, after various efforts to fix the problem, I notice that there
> are two distinct problems brought out by your test case.
>
> One of them is caused by my patch, one of them was already there in
> the code - this latter one is actually the hardest to fix.
>
> It took me about an hour to fix the first bug, but its taken a while
> of thinking about the second before I realised it was a pre-existing
> bug.
>
> The core problem is, as you observed that a pg_class update can cause
> rows to be lost with concurrent scans.
> We scan pg_class in two ways: to rebuild a relcache entry based on a
> relation's oid (easy fix). We also scan pg_class to resolve the name
> to oid mapping. The name to oid mapping is performed *without* a lock
> on the relation, since we don't know which relation to lock. So the
> name lookup can fail if we are in the middle of a pg_class update.
> This is an existing potential bug in Postgres unrelated to my patch.
> Ref: SearchCatCache()
>
> I've been looking at ways to lock the relation name and namespace
> prior to the lookup (or more precisely the hash), but its worth
> discussing whether we want that at all?

If this is a pre-existing bug, then it's not clear to me why we need
to do anything about it at all right now. I mean, it would be nice to
have a fix, but it's hard to imagine that any proposed fix will be
low-risk, and I don't remember user complaints about this. I continue
to think that the root of the problem here is that SnapshotNow is Evil
(TM). If we get rid of that, then this problem goes away, but that
strikes me as a long-term project.

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