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

Hi,

On 2014-05-05 13:41:00 +0300, Heikki Linnakangas wrote:
> 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

> 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.

I am not convinced by the latter reasoning but you're right that any
such change would hardly be backpatchable.

> +/*
> + * Exit hook to unlock the global transaction entry we're working on.
> + */
> +static void
> +AtProcExit_Twophase(int code, Datum arg)
> +{
> + /* same logic as abort */
> + AtAbort_Twophase();
> +}
> +
> +/*
> + * Abort hook to unlock the global transaction entry we're working on.
> + */
> +void
> +AtAbort_Twophase(void)
> +{
> + if (MyLockedGxact == NULL)
> + return;
> +
> + /*
> + * If we were in process of preparing the transaction, but haven't
> + * written the WAL record yet, remove the global transaction entry.
> + * Same if we are in the process of finishing an already-prepared
> + * transaction, and fail after having already written the WAL 2nd
> + * phase commit or rollback record.
> + *
> + * After that it's too late to abort, so just unlock the GlobalTransaction
> + * entry. We might not have transfered all locks and other state to the
> + * prepared transaction yet, so this is a bit bogus, but it's the best we
> + * can do.
> + */
> + if (!MyLockedGxact->valid)
> + {
> + RemoveGXact(MyLockedGxact);
> + }
> + else
> + {
> + LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
> +
> + MyLockedGxact->locking_backend = InvalidBackendId;
> +
> + LWLockRelease(TwoPhaseStateLock);
> + }
> + MyLockedGxact = NULL;
> +}

Is it guaranteed that all paths have called LWLockReleaseAll()
before calling the proc exit hooks? Otherwise we might end up waiting
for ourselves...

> /*
> * MarkAsPreparing
> @@ -261,29 +329,15 @@ MarkAsPreparing(TransactionId xid, const char *gid,
> errmsg("prepared transactions are disabled"),
> errhint("Set max_prepared_transactions to a nonzero value.")));
>
> - LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
> -
> - /*
> - * First, find and recycle any gxacts that failed during prepare. We do
> - * this partly to ensure we don't mistakenly say their GIDs are still
> - * reserved, and partly so we don't fail on out-of-slots unnecessarily.
> - */
> - for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
> + /* on first call, register the exit hook */
> + if (!twophaseExitRegistered)
> {
> - gxact = TwoPhaseState->prepXacts[i];
> - if (!gxact->valid && !TransactionIdIsActive(gxact->locking_xid))
> - {
> - /* It's dead Jim ... remove from the active array */
> - TwoPhaseState->numPrepXacts--;
> - TwoPhaseState->prepXacts[i] = TwoPhaseState->prepXacts[TwoPhaseState->numPrepXacts];
> - /* and put it back in the freelist */
> - gxact->next = TwoPhaseState->freeGXacts;
> - TwoPhaseState->freeGXacts = gxact;
> - /* Back up index count too, so we don't miss scanning one */
> - i--;
> - }
> + before_shmem_exit(AtProcExit_Twophase, 0);
> + twophaseExitRegistered = true;
> }

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.

Greetings,

Andres Freund

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