Re: Prepared transaction releasing locks before deregistering its GID

On Wed, Feb 20, 2019 at 03:14:07PM +0900, Masahiko Sawada wrote:
> @@ -811,6 +811,9 @@ TwoPhaseGetGXact(TransactionId xid)
>         static TransactionId cached_xid = InvalidTransactionId;
>         static GlobalTransaction cached_gxact = NULL;
> 
> +       Assert(!lock_held ||
> +                  LWLockHeldByMeInMode(TwoPhaseStateLock, LW_EXCLUSIVE));
> +
> 
> It seems strange to me, why do we always require an exclusive lock
> here in spite of acquiring a shared lock?

Thanks for looking at the patch.  LWLockHeldByMe() is fine here.  It
seems that I had a brain fade.

> TwoPhaseGetDummyBackendId() is called by
> multixact_twophase_postcommit() which is called while holding
> TwoPhaseStateLock in exclusive mode in FinishPreparedTransaction().
> Since we cache the global transaction when we call
> lock_twophase_commit() I couldn't produce issue but it seems to have a
> potential of locking problem.

You are right: this could cause a deadlock problem, but it actually
cannot be triggered now because the repetitive calls of
TwoPhaseGetGXact() make the resulting XID to be cached, so the lock
finishes by never be taken, but that could become broken in the
future.  From a consistency point of view, adding the same option to
TwoPhaseGetGXact() and TwoPhaseGetGXact() to control the lock
acquisition is much better as well.  Please note that the recovery
tests stress multixact post-commit callbacks already, so you can see
the caching behavior with that one:
BEGIN;
CREATE TABLE t_009_tbl2 (id int, msg text);
SAVEPOINT s1;
INSERT INTO t_009_tbl2 VALUES (27, 'issued to stuff');
PREPARE TRANSACTION 'xact_009_13';
CHECKPOINT;
COMMIT PREPARED 'xact_009_13';

The assertion really needs to be before the cached XID is checked
though.

Attached is an updated patch.  Thanks for the feedback. 
--
Michael