On 04/14/2014 09:48 PM, Heikki Linnakangas wrote:
> On 04/14/2014 07:51 PM, Tom Lane wrote:
>> I'd prefer to leave the prepare sequence alone and instead find a way
>> to reject COMMIT PREPARED until after the source transaction is safely
>> clear of the race conditions. The upthread idea of looking at vxid
>> instead of xid might help, except that I see we clear both of them
>> in ProcArrayClearTransaction. We'd need some state in PGPROC that
>> isn't cleared till later than that.
>
> Hmm. What if one of the post-cleanup action fails? We can't bail out of
> the prepare sequence until we have transfered the locks to the new
> PGPROC. Otherwise the locks are lost. In essence, there should be a
> critical section from the EndPrepare call until all the critical cleanup
> actions like PostPrepare_Locks have been done, and I don't think we want
> that. We might be able to guarantee that the built-in post-cleanup
> operations are safe enough for that, but there's also CallXactCallbacks
> in there.
>
> Given the lack of reports of that happening, though, perhaps that's not
> an issue.
I came up with the attached fix for this. Currently, the entry is
implicitly considered dead or unlocked if the locking_xid transaction is
no longer active, but this patch essentially turns locking_xid into a
simple boolean, and makes it the backend's responsibility to clear it on
abort. (it's not actually a boolean, it's a BackendId, but that's just
for debugging purposes to track who's keeping the entry locked). This
requires a process exit hook, and an abort hook, to make sure the entry
is always released, but that's not too difficult. It allows the backend
to release the entry at exactly the right time, instead of having it
implicitly released by ProcArrayClearTransaction.
If we error during prepare, after having written the prepare WAL record
but before the locks have been transfered to the dummy PGPROC, the locks
are simply released. This is wrong, but it's always been like that and
we haven't heard any complaints of that from the field, so I'm inclined
to leave it as it is. We could use a critical section to force a panic,
but that cure could be a worse than the disease.
I considered Andres' idea of using a new heavy-weight lock, but didn't
like it much. It would be a larger patch, which is not nice for
back-patching. One issue would be that if you run out of lock memory,
you could not roll back any prepared transactions, which is not nice
because it could be a prepared transaction that's hoarding the lock memory.
- Heikki