Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: Problem with txid_snapshot_in/out() functionality)

On 2014-05-15 17:21:28 +0300, Heikki Linnakangas wrote:
> >Is it guaranteed that all paths have called LWLockReleaseAll()
> >before calling the proc exit hooks? Otherwise we might end up waiting
> >for ourselves...
>
> Hmm. AbortTransaction() will release locks before we get here, but the
> before_shmem_exit() callpath will not. So an elog(FATAL), while holding
> TwoPhaseStateLock would cause us to deadlock with ourself. But there are no
> such elogs.

> I copied this design from async.c, which is quite similar, so if there's a
> problem that ought to be fixed too. And there are other more complicated
> before_shmem callbacks that worry me more, like createdb_failure_callback().
> But I think they're all all right.

Perhaps we should enforce that LWLockReleaseAll() is called first?
E.g. in shmem_exit()? It'll happen in ProcKill() atm, but that's
normally pretty much at the bottom of the stack.

> >It's not particularly nice to register shmem exit hooks in the middle of
> >normal processing because it makes it impossible to use
> >cancel_before_shmem_exit() previously registered hooks. I think this
> >should be registered at startup, if max_prepared_xacts > 0.
>
> <shrug>. async.c and namespace.c does the same, and it hasn't been a
> problem.

Well, it doesn't seem unreasonable to have C code using
PG_ENSURE_ERROR_CLEANUP/PG_END_ENSURE_ERROR_CLEANUP around a 2pc commit
to me. That'll break with this.
Perhaps we should just finally make cancel_before_shmem_exit search the
stack of callbacks.

Greetings,

Andres Freund

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