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

2014-05-15 Thread Heikki Linnakangas
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 */ +

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

2014-05-15 Thread Andres Freund
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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-05-15 Thread Heikki Linnakangas
On 04/14/2014 11:55 AM, Marko Kreen wrote: On Sun, Apr 13, 2014 at 05:46:20PM -0400, Jan Wieck wrote: On 04/13/14 14:22, Jan Wieck wrote: On 04/13/14 08:27, Marko Kreen wrote: I think you need to do SET_VARSIZE also here. Alternative is to move SET_VARSIZE after sort_snapshot(). And it

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

2014-05-15 Thread Robert Haas
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.

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

2014-05-06 Thread Andres Freund
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

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

2014-05-05 Thread Heikki Linnakangas
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

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

2014-04-14 Thread Heikki Linnakangas
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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-14 Thread Marko Kreen
On Sun, Apr 13, 2014 at 05:46:20PM -0400, Jan Wieck wrote: On 04/13/14 14:22, Jan Wieck wrote: On 04/13/14 08:27, Marko Kreen wrote: I think you need to do SET_VARSIZE also here. Alternative is to move SET_VARSIZE after sort_snapshot(). And it seems the drop-double-txid logic should be

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-14 Thread Heikki Linnakangas
On 04/12/2014 05:03 PM, Andres Freund wrote: On 2014-04-12 09:47:24 -0400, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: On 04/12/2014 12:07 AM, Jan Wieck wrote: the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-14 Thread Andres Freund
On 2014-04-14 12:15:30 +0300, Heikki Linnakangas wrote: Hmm. There's a field in GlobalTransactionData called locking_xid, which is used to mark the XID of the transaction that's currently operating on the prepared transaction. At prepare, that ensures that the transaction cannot be committed

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

2014-04-14 Thread Tom Lane
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

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

2014-04-14 Thread Andres Freund
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

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

2014-04-14 Thread Tom Lane
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

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

2014-04-14 Thread Andres Freund
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

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

2014-04-14 Thread Heikki Linnakangas
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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-13 Thread Marko Kreen
On Sat, Apr 12, 2014 at 02:10:13PM -0400, Jan Wieck wrote: Since it doesn't seem to produce any side effects, I'd think that making the snapshot unique within txid_current_snapshot() and filtering duplicates on input should be sufficient and eligible for backpatching. Agreed. The attached

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-13 Thread Jan Wieck
On 04/13/14 08:27, Marko Kreen wrote: On Sat, Apr 12, 2014 at 02:10:13PM -0400, Jan Wieck wrote: Since it doesn't seem to produce any side effects, I'd think that making the snapshot unique within txid_current_snapshot() and filtering duplicates on input should be sufficient and eligible for

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

2014-04-13 Thread Heikki Linnakangas
On 04/12/2014 05:03 PM, Andres Freund wrote: If we don't, aren't we letting other backends see non-self-consistent state in regards to who holds which locks, for example? I think that actually works out ok, because the locks aren't owned by xids/xacts, but procs. Otherwise we'd be in deep

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-13 Thread Jan Wieck
On 04/13/14 14:22, Jan Wieck wrote: On 04/13/14 08:27, Marko Kreen wrote: I think you need to do SET_VARSIZE also here. Alternative is to move SET_VARSIZE after sort_snapshot(). And it seems the drop-double-txid logic should be added also to txid_snapshot_recv(). It seems weird to have it

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Heikki Linnakangas
On 04/12/2014 12:07 AM, Jan Wieck wrote: Hackers, the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that a txid_snapshot on output lists the same txid multiple times in the xip list part of the external representation. This string is

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Jan Wieck
On 04/12/14 03:27, Heikki Linnakangas wrote: On 04/12/2014 12:07 AM, Jan Wieck wrote: Hackers, the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that a txid_snapshot on output lists the same txid multiple times in the xip list part of

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Andres Freund
On 2014-04-12 10:27:16 +0300, Heikki Linnakangas wrote: On 04/12/2014 12:07 AM, Jan Wieck wrote: The symptom is that a txid_snapshot on output lists the same txid multiple times in the xip list part of the external representation. This string is later rejected by txid_snapshot_in() when trying

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Jan Wieck
On 04/12/14 08:38, Andres Freund wrote: Since it's sorted there, that should be fairly straightforward. Won't fix already created and stored datums tho. Maybe _in()/parse_snapshot() should additionally skip over duplicate values? Looks easy enough. There is the sort ... missed that when

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Tom Lane
Heikki Linnakangas hlinnakan...@vmware.com writes: On 04/12/2014 12:07 AM, Jan Wieck wrote: the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that a txid_snapshot on output lists the same txid multiple times in the xip list part of

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Andres Freund
On 2014-04-12 09:47:24 -0400, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: On 04/12/2014 12:07 AM, Jan Wieck wrote: the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that a txid_snapshot on output lists the

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Greg Stark
On 12 Apr 2014 08:35, Jan Wieck j...@wi3ck.info wrote: On 04/12/14 03:27, Heikki Linnakangas wrote: On 04/12/2014 12:07 AM, Jan Wieck wrote: Hackers, Hmm. Do we snapshots to be stored in tables, and included in a dump? I don't think we can guarantee that will work, at least not across

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Jan Wieck
On 04/12/14 10:09, Greg Stark wrote: A pg_restore would start a new xid space from FirstNormalXid which would obviously not work with any stored txids. http://www.postgresql.org/docs/9.3/static/app-pgresetxlog.html Regards, Jan -- Jan Wieck Senior Software Engineer http://slony.info --

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Andres Freund
On 2014-04-12 11:15:09 -0400, Jan Wieck wrote: On 04/12/14 10:09, Greg Stark wrote: A pg_restore would start a new xid space from FirstNormalXid which would obviously not work with any stored txids. http://www.postgresql.org/docs/9.3/static/app-pgresetxlog.html Using that as part of any

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Jan Wieck
On 04/12/14 11:18, Andres Freund wrote: On 2014-04-12 11:15:09 -0400, Jan Wieck wrote: On 04/12/14 10:09, Greg Stark wrote: A pg_restore would start a new xid space from FirstNormalXid which would obviously not work with any stored txids.

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-12 Thread Jan Wieck
On 04/12/14 10:03, Andres Freund wrote: On 2014-04-12 09:47:24 -0400, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: On 04/12/2014 12:07 AM, Jan Wieck wrote: the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that

[HACKERS] Problem with txid_snapshot_in/out() functionality

2014-04-11 Thread Jan Wieck
Hackers, the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that a txid_snapshot on output lists the same txid multiple times in the xip list part of the external representation. This string is later rejected by txid_snapshot_in()