Re: mvcc catalo gsnapshots and TopTransactionContext

On February 2, 2014 5:52:22 PM CET, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> On 2014-01-31 16:41:33 -0500, Bruce Momjian wrote:
>>> Is there any plan to commit this?
>
>> IMO it has to be applied. Tom objected on the grounds that cache
>> invalidation has to be fixed properly but that's a major
>restructuring
>> of code that worked this way for a long time. So changing the
>Assert()
>> to reflect that seems fair to me.
>
>The replacement Assert is wrong no? At least that's what was said
>upthread.

Well, no. Noah's version isn't as strict as desirable, but it doesn't trigger in wrong cases. Thus still better than what's in 9.3 (nothing).

> More to the point, changing the Assert so it doesn't fire
>doesn't do one damn thing to ameliorate the fact that cache reload
>during transaction abort is wrong and unsafe.

And, as upthread, I still don't think that's correct. I don't have sources available right now, but IIRC we already have aborted out of the transaction. Released locks, the xid and everything. We just haven't changed the state yet - and affair we can't naively do so, otherwise we'd do incomplete cleanups if we got interrupted somehow.
So yes, it's not pretty and it's really hard to verify. But that doesn't make it entirely wrong.
I don't see the point of an assert that triggers for code (and coders) that can't do anything about it because their code is correct. All the while the assertion actually triggers for ugly but correct code.

Andres

--
Please excuse brevity and formatting - I am writing this on my mobile phone.

Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services