Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
On 05/06/2014 02:44 PM, Andres Freund wrote: On 2014-05-05 13:41:00 +0300, Heikki Linnakangas wrote: +/* + * 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... 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. /* * 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. shrug. async.c and namespace.c does the same, and it hasn't been a problem. I committed this now, but please let me know if you see a concrete problem with the locks. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
On Thu, May 15, 2014 at 10:38 AM, Andres Freund and...@2ndquadrant.com wrote: 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. Yes, please. And while we're at it, perhaps we should make it Trap() or fail an Assert() if it doesn't find the callback it was told to remove. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
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 diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 66dbf58..995f51f 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -58,6 +58,7 @@ #include replication/walsender.h #include replication/syncrep.h #include storage/fd.h +#include storage/ipc.h #include storage/predicate.h #include storage/proc.h #include storage/procarray.h @@ -82,25 +83,25 @@ int max_prepared_xacts = 0; * * The lifecycle of a global transaction is: * - * 1. After checking that the requested GID is not in use, set up an - * entry in the TwoPhaseState-prepXacts array with the correct XID and GID, - * with locking_xid = my own XID and valid = false. + * 1. After checking that the requested GID is not in use, set up an entry in + * the TwoPhaseState-prepXacts array with the correct GID and valid = false, + * and mark it as locked by my backend. * * 2. After successfully completing prepare, set valid = true and enter the * referenced PGPROC into the global ProcArray. * - * 3. To begin COMMIT PREPARED or ROLLBACK PREPARED, check that the entry - * is valid and its locking_xid is no longer active, then store my current - * XID into locking_xid. This prevents concurrent attempts to commit or - * rollback the same prepared xact. + * 3. To begin COMMIT PREPARED or ROLLBACK PREPARED, check that the entry is + * valid and not locked, then mark the entry as locked by storing my current + * backend ID into locking_backend. This prevents concurrent attempts to + * commit or rollback the same prepared xact. * * 4. On completion of COMMIT PREPARED or ROLLBACK PREPARED, remove the entry * from the ProcArray and the TwoPhaseState-prepXacts array and return it to * the freelist. * * Note that if the preparing transaction fails between steps 1 and 2, the - * entry will remain in prepXacts until recycled. We can detect recyclable - * entries by checking for valid = false and locking_xid no longer active. + * entry must be removed so that the GID and the GlobalTransaction struct + * can be reused. See AtAbort_Twophase(). * * typedef struct GlobalTransactionData *GlobalTransaction appears in * twophase.h @@ -115,8 +116,8 @@ typedef struct GlobalTransactionData TimestampTz prepared_at; /* time of preparation */ XLogRecPtr prepare_lsn; /* XLOG offset of prepare record */ Oid owner; /* ID of user that executed the xact */ -
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
On 04/13/2014 11:39 PM, Heikki Linnakangas wrote: However, I just noticed that there's a race condition between PREPARE TRANSACTION and COMMIT/ROLLBACK PREPARED. PostPrepare_Locks runs after the prepared transaction is already marked as fully prepared. That means that by the time we get to PostPrepare_Locks, another backend might already have finished and removed the prepared transaction. That leads to a PANIC (put a breakpoint just before PostPrepare_Locks): postgres=# commit prepared 'foo'; PANIC: failed to re-find shared proclock object PANIC: failed to re-find shared proclock object The connection to the server was lost. Attempting reset: Failed. FinishPrepareTransaction reads the list of locks from the two-phase state file, but PANICs when it doesn't find the corresponding locks in the lock manager (because PostPrepare_Locks hasn't transfered them to the dummy PGPROC yet). I think we'll need to transfer of the locks earlier, before the transaction is marked as fully prepared. I'll take a closer look at this tomorrow. Here's a patch to do that. It's very straightforward, I just moved the calls to transfer locks earlier, before ProcArrayClearTransaction. PostPrepare_MultiXact had a similar race - it also transfer state from the old PGPROC entry to the new, and needs to be done before allowing another backend to remove the new PGPROC entry. I changed the names of the functions to distinguish them from the other PostPrepare_* functions that now happen at a different time. The patch is simple, but it's a bit scary to change the order of things like this. Looking at all the calls that now happen after transferring the locks, I believe this is OK. The change also applies to the callbacks called by the RegisterXactCallback mechanism, which means that in theory there might be a 3rd party extension out there that's affected. All the callbacks in contrib and plpgsql are OK, and it's questionable to do anything complicated that would depend on heavy-weight locks to be held in those callbacks, so I think this is OK. Warrants a note in the release notes, though. - Heikki diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index d4ad678..b505c62 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1580,11 +1580,12 @@ AtPrepare_MultiXact(void) } /* - * PostPrepare_MultiXact - * Clean up after successful PREPARE TRANSACTION + * TransferPrepare_MultiXact + * Called after successful PREPARE TRANSACTION, before releasing our + * PGPROC entry. */ void -PostPrepare_MultiXact(TransactionId xid) +TransferPrepare_MultiXact(TransactionId xid) { MultiXactId myOldestMember; diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b20d973..c60edf1 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2224,6 +2224,16 @@ PrepareTransaction(void) XactLastRecEnd = 0; /* + * Transfer any heavy-weight locks we're holding to the dummy ProcArray. + * + * NB: this has to be done before clearing our own ProcArray entry. + * This is different from normal commit, where locks are released after + * clearing the ProcArray entry! + */ + TransferPrepare_MultiXact(xid); /* also transfer our multixact state */ + TransferPrepare_Locks(xid); + + /* * Let others know about no transaction in progress by me. This has to be * done *after* the prepared transaction has been marked valid, else * someone may think it is unlocked and recyclable. @@ -2234,6 +2244,10 @@ PrepareTransaction(void) * This is all post-transaction cleanup. Note that if an error is raised * here, it's too late to abort the transaction. This should be just * noncritical resource releasing. See notes in CommitTransaction. + * + * NB: we already transfered the locks to the prepared ProcArray entry, + * so even the cleanup before ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) + * cannot rely on any heavy-weight locks being held! */ CallXactCallbacks(XACT_EVENT_PREPARE); @@ -2256,11 +2270,12 @@ PrepareTransaction(void) PostPrepare_smgr(); - PostPrepare_MultiXact(xid); - - PostPrepare_Locks(xid); PostPrepare_PredicateLocks(xid); + /* + * we're not actually holding any locks anymore, but clean up any other + * resources that might need to be cleaned up at this stage. + */ ResourceOwnerRelease(TopTransactionResourceOwner, RESOURCE_RELEASE_LOCKS, true, true); diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 6825063..779f0cb 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -3069,8 +3069,8 @@ AtPrepare_Locks(void) } /* - * PostPrepare_Locks - * Clean up after successful PREPARE + * TransferPrepare_Locks + * Transfer locks to prepared transaction after successful PREPARE * * Here, we want to transfer ownership of our locks to a dummy PGPROC
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
Heikki Linnakangas hlinnakan...@vmware.com writes: I think we'll need to transfer of the locks earlier, before the transaction is marked as fully prepared. I'll take a closer look at this tomorrow. Here's a patch to do that. It's very straightforward, I just moved the calls to transfer locks earlier, before ProcArrayClearTransaction. PostPrepare_MultiXact had a similar race - it also transfer state from the old PGPROC entry to the new, and needs to be done before allowing another backend to remove the new PGPROC entry. I changed the names of the functions to distinguish them from the other PostPrepare_* functions that now happen at a different time. Why didn't you also move up PostPrepare_PredicateLocks? Seems like its access to MySerializableXact is also racy. The patch is simple, but it's a bit scary to change the order of things like this. Yeah. There are a lot of assumptions in there about the order of resource release, in particular that it is safe to do certain things because we're still holding locks. I poked around a bit and noticed one theoretical problem sequence: if the prepared xact drops some relation that we're still holding buffer pins on. This shouldn't really happen (why are we still pinning some rel we think we dropped?) but if it did, the commit would do DropRelFileNodeBuffers which would end up busy-looping until we drop our pins (see InvalidateBuffer, which thinks this must be an I/O wait situation). So it would work, more or less, but it seems pretty fragile. I'm afraid there are more assumptions like this one. The whole thing feels like we are solving the wrong problem, anyway. IIUC, the complaint arises because we are allowing COMMIT PREPARED to occur before the source transaction has reported successful prepare to its client. Surely that does not need to be a legal case? No correctly-operating 2PC xact manager would do that. 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
On 2014-04-14 12:51:02 -0400, Tom Lane wrote: The whole thing feels like we are solving the wrong problem, anyway. IIUC, the complaint arises because we are allowing COMMIT PREPARED to occur before the source transaction has reported successful prepare to its client. Surely that does not need to be a legal case? No correctly-operating 2PC xact manager would do that. I agree here. This seems somewhat risky, just to support a case that shouldn't happen in reality - as somewhat evidenced by the fact that there don't seem to be field reports around this. 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. I wonder if the most natural way to express this wouldn't be to have a heavyweight lock for every 2pc xact 'slot'. ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) should be scheduled correctly to make error handling for this work. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
Andres Freund and...@2ndquadrant.com writes: I wonder if the most natural way to express this wouldn't be to have a heavyweight lock for every 2pc xact 'slot'. ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) should be scheduled correctly to make error handling for this work. That seems like not a bad idea. Could we also use the same lock to prevent concurrent attempts to commit/rollback the same already-prepared transaction? I forget what we're doing to forestall such cases right now. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
On 2014-04-14 13:47:35 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: I wonder if the most natural way to express this wouldn't be to have a heavyweight lock for every 2pc xact 'slot'. ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) should be scheduled correctly to make error handling for this work. That seems like not a bad idea. Could we also use the same lock to prevent concurrent attempts to commit/rollback the same already-prepared transaction? I forget what we're doing to forestall such cases right now. GlobalTransaction-locking_xid is currently used. If it points to a live transaction by another backned prepared transaction with identifier \%s\ is busy will be thrown. ISTM if there were using a lock for every slot, that logic couldbe thrown away. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)
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. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers