Thread: Re: [pgjdbc] XADataSource support for resource sharing & interleaving. (#47)

I still can't get very excited about this. The XADataSource
implementation shouldn't have to maintain a connection pool just to
support some obscure JTA features. It doesn't make sense to require
suspend/resume and interleaving support when the underlying protocol
doesn't support them. The point of JTA is two-phase commit, and you can
do two-phase commit just as well without those features.

On the whole, I think it would be best if Glassfish developers would add
an option to not rely on interleaving - all other major Transaction
Managers do. They will need that to support other XA implementations
that don't support suspend/resume and interleaving, anyway.

If we must have this anyway, I think we should try to implement it as a
generic wrapper that could be used with any XADataSource implementation.

I also wonder if we could grab some low-hanging fruit without doing the
whole pooling thing. For example, if you try to call (2nd phase)
commit() on an connection that's currently busy with another
transaction, instead of throwing an error we could open a new temporary
connection, issue the COMMIT PREPARED, and disconnect. Sure, that would
be slow, so you'd definitely still want to configure the TM to not do
that, but at least it would work.

On 15.03.2013 19:14, Dave Cramer wrote:
> I'm looking for opinions on this patch.
>
> Heikki ? Kevin Grittner ?
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>
> On Fri, Mar 15, 2013 at 12:45 PM, Bryan Varner<notifications@github.com>wrote:
>
>> This reimplementation of the PGXADataSource (and supporting classes)
>> provides more complete fulfillment of the JTA 1.0.1 specification.
>>
>> It properly implements resource sharing (3.4.6) allowing all connections
>> to the same underlying XAResource to control the global transactions
>> created by other instances of that XAResource.
>>
>> It addresses proper interleaving and isolation of local&  global
>> transaction scopes. (3.4.7)
>>
>> The support for interleaving and sharing is implemented by (potentially,
>> only when needed) returning 'logical' connections from the XADataSource,
>> and opening new physical connections to the backend server (when necessary)
>> to handle preforming work on behalf of a global TX or local TX.
>>
>> The behavior of opening new physical backends (implementing interleaving)
>> can be disabled by setting the XAAcquireTimeout = 0, in which case the
>> behavior of the old XADataSource implementation is retained, with a minimal
>> performance impact.
>>
>> It tries hard to only open as many physical connections as necessary to
>> service the logical connections, and prunes extraneous connections created
>> as a result of supporting interleaving at each logical connection handle
>> close().
>>
>> In our production environment, where we have allocated our pool sizes to
>> the actual observed in-use highwater mark + 30%, we are seeing zero
>> connection churn, as there are enough free connections at any given time to
>> properly service the checked-out, in-use logical connections and the
>> interleaving&  resource sharing being invoked by the container.
>>
>> This implementation passes:
>>
>>     - all the existing XADataSource test cases
>>     - the many we've added to test corner cases of XA handling in
>>     concurrent and pooled workloads.
>>     - the JBoss narayana Transaction Manager jdbc unit tests. (
>>     https://github.com/jbosstm/narayana)
>>
>> In addition to the XADataSource changes
>>
>>     - the build.xml has been update to respect (fully) the srcdir property
>>     - AbstractJdbc23PooledConnection exposes getBackingConnection() to
>>     sub-classes, allowing subclasses access to the underlying Connection object
>>     without needing to cache the instance in their constructors.
>>
>> We have been successfully using this snapshot of code in our production
>> environment (GlassFish 3.1.2.2) with PostgreSQL 9.1, and are pleased to
>> report that it is working very well at this time.
>> ------------------------------
>> You can merge this Pull Request by running
>>
>>    git pull https://github.com/polarislabs/pgjdbc POLARIS_XA
>>
>> Or view, comment on, or merge it at:
>>
>>    https://github.com/pgjdbc/pgjdbc/pull/47
>> Commit Summary
>>
>>     - This is an in-progress refactor attempt with the express goals of
>>     providing a complete, and robust JTA compliant XA Implementation.
>>     - Stubbing out / making progress on this.
>>     - Ok, whoops.
>>     - I'm not totally sold on logicalConnectionId at this point, but it
>>     might work.
>>     - Started stubbing out / reconciling original precondition /
>>     deficiences, this should help cement the types of things we'll need the
>>     XADataSource to handle.
>>     - "A broken build is a sign of progress!"
>>     - implementation should be somewhat possible to execute.
>>     - That shouldn't be there.
>>     - helps to remember to unignore things.
>>     - updated the gitignore
>>     - I realize this this may mean some of our work does not get accepted
>>     upstream, but I can't abide not having the source in a source directory
>>     anymore, where netbeans (i know, i know) can actually resolve the proper
>>     package names and what-not. It's amazing I lasted this long without doing
>>     this. Seriously, I'm tempted to make this a maven build.
>>     - Some cleanup, and getting the build.xml to behave a little better.
>>     - Forgot I moved the getPhysicalConnection(), wow I should back off
>>     the coffee.
>>     - This is actually starting to pass test cases.
>>     - FUNCTIONAL PARITY.
>>     - Structuring the build to renable the test suite.
>>     - Minor changes, not really sure what...
>>     - Adding the nbignore...
>>     - added idea files to gitignore
>>     - Updating a few things.
>>     - we just passed the old interleaving tests.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Added docs, reformatted some stuff, cleaned up some code
>>     - Merged from Bryan's chanages to pass all the tests.
>>     - Implements Referenceable, Serializable.
>>     - Tweaking these to be more aggressive by default.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Code paranoia
>>     - Added some documentation
>>     - Abstracted connection creation, yes I actually have a use-case for
>>     this.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Moved logical connection if checks to the top to throw a
>>     IllegalArgumentException since it is required for the method.
>>     - Added some debug logging
>>     - Don't commit the out folder
>>     - Added idea's out folder to the ignore
>>     - Moved the autocommit bits to the logical connection part. I think
>>     this may help.
>>     - Updated docs and wrapped debug statement to avoid performance hit
>>     when logging not desired
>>     - Logger might as well be local if it's only going to be used in the
>>     one place.
>>     - Changing up the physical pooling policies and connection cleanup
>>     cycle.
>>     - Disassociate so we have a shot at closing things.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Cleaning up some leftovers.
>>     - Improved the first interleaving test, to exercise isolation checks
>>     in the midst of interleaving.
>>     - Added a test case for join, which is exposing an actual bug.
>>     - Added a few tests cases which are exposing some problems centered
>>     around connection.close(), and the handling of things on the backends.
>>     - Added a lot of debug logging to PGXADataSource and
>>     PhysicalXAConnection.
>>     - Added lib folder to gitignore to allow ant builds directly from idea
>>     without committing the junit jar.
>>     - Added a TODO to indicate improvement needed on Logger creation.
>>     - Added some extra logging and documentation.
>>     - Fixed typo
>>     - Fixed local thread test. The thread wasn't being joined before
>>     checking the error value. So, the error value was being checked before the
>>     query had a chance to complete.
>>     - Cleaned up imports.
>>     - Getting the physical connection-based logger would require too much
>>     rework at this point, so just use the Driver-base logger instead.
>>     - Fixed null pointer exception since the fake xid class doesn't null
>>     check in its equals method
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - synchronized a section of code that might cause a race condition
>>     while trying to get a logical connection that doesn't have a transaction
>>     started yet in order to issue a commit prepared.
>>     - This test can expose a couple of race conditions. Though because of
>>     the nature of them, the test passes ... sometimes.
>>     - Blunt-Edged, Brute-Force approach. Lock all the things!
>>     - Might as well go all Reentrant
>>     - Now start to rein it in
>>     - Completely re-implemented.
>>     - Clean up / documentation of a failure point.
>>     - Removed all the rediculous local TX tracking in the logical
>>     connection side. Really we need to care about the autocommit of the
>>     physical TX, which we should only be mangling when we associate /
>>     disassociate to an XID. A logical connection should always defer it's
>>     autocommit status to that of the physical connection servicing it.
>>     - If a physical connection is in local TX mode, try to keep it to one
>>     logical connection per physical...
>>     - Added a simple test of a transaction commit on a pooled connection
>>     to simulate app servers connection pooling. This reproduces the
>>     "double-close" issue we have been seeing.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Close behavior now waits for the logical connection to close, rather
>>     than the handle.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Keep the username / password around, so that we can allocate new
>>     physical connections if all of them are closed and a logical connection
>>     tries to do something.
>>     - Commiting changes to the wait timeouts and strategy if you set the
>>     timeout to 0.
>>     - Added threaded pool test that exhausts the pool.
>>     - Merge branch 'POLARIS_XA' of github.com:polarislabs/pgjdbc into
>>     POLARIS_XA
>>     - Allow for disabling interleaving support. We'll probably want to do
>>     this by default, and let people enable it when they need it.
>>     - Hey, we weren't using that logger...
>>     - Hey, we weren't using that logger...
>>     - Better support for people who want to disable interleaving, and have
>>     a hard&  fast 1 - 1 logical to physical mapping.
>>     - Added a test case that exercises (intermittently, however) the race
>>     condition between prepare&  commit / close.
>>     - These test cases are duplicating the exact type of intermittent
>>     failure we're seeing in our deployed environment.
>>     - More updates, we can close anything IDLE. But not OPEN or marked for
>>     ROLLBACK (at least not without allowing the rollback first!).
>>     - I haven't run this against the JBOSS TM QA tests yet, but at this
>>     point, it's passing all the tests we have (including the race-condition
>>     tests for close() logic) that we have.
>>     - Whoops. build.xml changes crept in.
>>     - Rollback only disassociates the xid, not the entire physical
>>     connection. In the case of a Connection pegged to a thread, the next
>>     invocation could end up royally messing with the state of the pool.
>>     - Passes the test where the last logical connection closed closes any
>>     non-suspended TX.
>>     - Verify that we end up on the same LocalTX transaction (backend pg
>>     PID) as we did when we started.
>>     - Moving things back to the original location.
>>     - Cleaning up build.xml changes
>>     - More cleanup, finer-grained locking in a few spots.
>>     - Added 'debugtest' target which sets up a dt_socket server for
>>     debugging test cases.
>>     - Broke resource sharing, where the XAResource can outlive the logical
>>     connection (apparently).
>>     - Fixed a few issues with close before rollback.
>>     - Eager association of logicals to physicals was causing connection
>>     bloat.
>>     - Greatly reduced connection usage, by improving re-use, and only
>>     allocating physical backends when it's necessary to provide a reasonable
>>     level of performance.
>>     - Added a test case to verify that we weren't closing connections we
>>     shouldn't be.
>>     - Even more tests about connection association...
>>     - Fixed the wait strategy to wait up to xaAcquireTimeout milliseconds
>>     (in multiple passes if necessary) before allocating a new physical
>>     connection.
>>     - RecoveredXid is blowing up when GlassFish tries to ping the
>>     XAResource with null xids.
>>     - Merge remote-tracking branch 'upstream/master' into POLARIS_XA
>>     - Adding a test case that I think will duplicate the prepared
>>     statement / cursor issue we're seeing with connection closing.
>>     - Uh, we don't need 10k of these..
>>     - Updated test case to use cursors.
>>     - Cleaned up / simplified some of the close() code.
>>
>> File Changes
>>
>>     - *M* .gitignore<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-0>(5)
>>     - *M* build.properties<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-1>(1)
>>     - *M* build.xml<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-2>(97)
>>     - *M*
org/postgresql/ds/jdbc23/AbstractJdbc23PooledConnection.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-3>(9)
>>     - *A*
org/postgresql/test/xa/PooledXADataSourceTest.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-4>(250)
>>     - *M* org/postgresql/test/xa/XADataSourceTest.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-5>(859)
>>     - *M* org/postgresql/test/xa/XATestSuite.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-6>(1)
>>     - *M* org/postgresql/xa/.gitignore<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-7>(2)
>>     - *R* org/postgresql/xa/AbstractPGXADataSource.java.in<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-8>(3)
>>     - *M* org/postgresql/xa/PGXAConnection.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-9>(710)
>>     - *A* org/postgresql/xa/PGXADataSource.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-10>(815)
>>     - *M* org/postgresql/xa/PGXADataSourceFactory.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-11>(20)
>>     - *A* org/postgresql/xa/PhysicalXAConnection.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-12>(385)
>>     - *M* org/postgresql/xa/RecoveredXid.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-13>(46)
>>     - *M*
org/postgresql/xa/jdbc3/AbstractJdbc3XADataSource.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-14>(29)
>>     - *M*
org/postgresql/xa/jdbc4/AbstractJdbc4XADataSource.java<https://github.com/pgjdbc/pgjdbc/pull/47/files#diff-15>(2)
>>
>> Patch Links:
>>
>>     - https://github.com/pgjdbc/pgjdbc/pull/47.patch
>>     - https://github.com/pgjdbc/pgjdbc/pull/47.diff
>>
>>
>


--
- Heikki

> If we must have this anyway, I think we should try to implement it as a
> generic wrapper that could be used with any XADataSource implementation.

I think you mean DataSource implementation. So long as the underlying
RDBMS supports prepared transactions, there's no reason a wrapper
couldn't handle -all- of the XA bits.

For the purposes POLARIS Laboratories had, it was easier / faster for us
to get the behavior we needed starting from a stable build of the pg
driver and implementing this stuff in the driver code.

The immediate problem POLARIS faced was the driver implementing
XADataSource properly -- that is, as defined in the javadocs / JTA 1.0.1
spec. This patch addresses those shortcomings.

I'm not opposed to pulling it out into a wrapper, but what you're
describing in doing so is a new F/OSS project to accomplish that goal.
There will be some overlap with other existing projects. DBCP provides a
pooling data source wrapper for drivers which don't provide it.
Hibernate has a system for providing dialect-specific SQL, which we'll
need to handle prepare() commit() rollback() recover() of 2pc operations
on a per-dialect basis. This becomes a much larger project with a much
larger scope -- one I'm not sure I can make a case for my employer to
support.

> I also wonder if we could grab some low-hanging fruit without doing the
> whole pooling thing.

It's not really a 'pool'. It's a list of physical connections bound to a
logical connection context.

> For example, if you try to call (2nd phase)
> commit() on an connection that's currently busy with another
> transaction, instead of throwing an error we could open a new temporary
> connection, issue the COMMIT PREPARED, and disconnect.

And then you'll try to mitigate the performance issue (which is very
noticeable in that situation), and end up keeping track of a list of
connections.

> Sure, that would
> be slow, so you'd definitely still want to configure the TM to not do
> that, but at least it would work.

We evaluated what it would take to get pgjdbc to work with GlassFish's
TM as is. There are cases in our application where it does SUSPEND. What
you are proposing will not fix the missing support for SUSPEND / JOIN.

Honestly, I don't understand why all these drivers did such a half-baked
job of implementing the spec, or why you're so vocally defending a
half-implementation when it took two people bouncing ideas, code, and
testing a week to get it workable, and a few additional weeks to
stabilize it.

I think it's completely unreasonable to expect the reference
implementation of a container that follows specifications to provide a
workaround for non-compliant drivers which incorrectly implement a
well-documented interface. It's not the fault of the GlassFish project
that the standing XA implementation in pgjdbc (or any other driver) sucks.

Regards,
-Bryan Varner

---------------
The views expressed in this communication are the sole responsibility of
the author and do not reflect those of POLARIS Laboratories.

On 15.03.2013 20:48, Bryan Varner wrote:
>> If we must have this anyway, I think we should try to implement it as a
>> generic wrapper that could be used with any XADataSource implementation.
>
> I think you mean DataSource implementation. So long as the underlying
> RDBMS supports prepared transactions, there's no reason a wrapper
> couldn't handle -all- of the XA bits.

Yeah, but there is no standardized way to work with the underlying
RDBM's prepared transactions, other than XADataSource. PostgreSQL uses
PREPARE TRANSACTION, other RDBMs have something else. I was thinking of
a wrapper that calls the native XADataSource implementation, which
doesn't need to support suspend/resume and interleaving, and presents a
fully-compliant XADataSource implementation to the outside, using the
technique you used.

> I'm not opposed to pulling it out into a wrapper, but what you're
> describing in doing so is a new F/OSS project to accomplish that goal.
> There will be some overlap with other existing projects. DBCP provides a
> pooling data source wrapper for drivers which don't provide it.

Yeah, possibly. Or it could still be included in the org.postgresql
driver. I guess it depends on the details, on what exactly the wrapper
will look like.

> Hibernate has a system for providing dialect-specific SQL, which we'll
> need to handle prepare() commit() rollback() recover() of 2pc operations
> on a per-dialect basis.

I didn't understand that part. What does Hibernate have to do with this?

>> Sure, that would
>> be slow, so you'd definitely still want to configure the TM to not do
>> that, but at least it would work.
>
> We evaluated what it would take to get pgjdbc to work with GlassFish's
> TM as is. There are cases in our application where it does SUSPEND. What
> you are proposing will not fix the missing support for SUSPEND / JOIN.

Did you evaluate what it would take to add the option to Glassfish?

> Honestly, I don't understand why all these drivers did such a half-baked
> job of implementing the spec, or why you're so vocally defending a
> half-implementation when it took two people bouncing ideas, code, and
> testing a week to get it workable, and a few additional weeks to
> stabilize it.

The JTA specification is crap. It imposes requirements on the drivers
that have nothing to do with the real goal of the spec: supporting
two-phase commit. It's silly that drivers have to be complicated to
support those things.

If you look at the javadocs for XADataSource, it says for getXAConnection():

> Attempts to establish a *physical* database connection that can be used in a distributed transaction.

(emphasis mine). It's pretty clear what the intention of the authors is.
"physical" is a vague term, but you'd expect that typically to be a
single TCP connection, wouldn't you? It's not expected that the driver
needs a multiplexing layer, mapping a single XAConnection to multiple
physical connections.

Now, maybe it works fine with the multiplexing layer, but even if that's
the way to meet the letter of the JTA spec, I don't think it's in the
spirit of the spec that you'd have to do that.

> I think it's completely unreasonable to expect the reference
> implementation of a container that follows specifications to provide a
> workaround for non-compliant drivers which incorrectly implement a
> well-documented interface.

Doesn't hurt to ask... They do have
xa-driver-does-not-support-non-tx-operations and
oracle-xa-recovery-workaround options, which are similar workarounds.

Regarding the patch itself:

Does it work correctly if you prepare a PreparedStatement in one
transaction, suspend, and try to use the prepared statement in another
transaction? I'm not sure what "correctly" is here, I don't think the
JTA spec says anything explicit about that scenario (it's crap, after
all ;-)), but I'd expect that to work.

It's a bit worrying that you can exceed your connection pool size with
this. In the worst case, a single logical connection can use an
arbitrary number of physical connections. That makes it more difficult
to set max_connections, so that you don't run out of slots. To an admin,
it's pretty surprising to see 50 connections from a host, when you've
configured the max connection pool size to 40.

- Heikki

 > I was thinking of
> a wrapper that calls the native XADataSource implementation, which
> doesn't need to support suspend/resume and interleaving, and presents a
> fully-compliant XADataSource implementation to the outside, using the
> technique you used.

To me, if you're trying to solve a problem of an insufficient or
non-functioning XA implementation, you don't do that by relying upon
said insufficient XA implementation.

The way the code in the POLARIS_XA branch works would have been a pain
to wrap around the existing PGXAConnection and would have required
_more_ physical connections to handle interleaving by wrapping the
existing pgjdbc implementation. IIRC, the existing pgjdbc implementation
did not allow for a connection to switch global transactions after
prepare but before commit or rollback. This is a use-case that GlassFish
does frequently, where as soon as a connection is done with a Xid, it's
aggressively used for another Xid, and an XAResource returning true for
isSameRM is used to commit / rollback the prepared TX.

If you were to implement sharing properly atop the old PGXAConnection
our patch replaces, it would take a lot more physical (PGXAConnections
dedicated to xids which have yet to commit / rollback) than what we've
created a pull request for.

>> Hibernate has a system for providing dialect-specific SQL, which we'll
>> need to handle prepare() commit() rollback() recover() of 2pc operations
>> on a per-dialect basis.
>
> I didn't understand that part. What does Hibernate have to do with this?

Nothing. I'm pointing out that if I really want to make the wrapper
generic, and not rely upon the idiosyncrasies of faulty XADataSource
implementations, that you'll need to account for dialect in the wrapper
-- otherwise you wouldn't need the wrapper because the XADataSource from
the driver would be sufficient as implemented.

I was merely pointing out that there are other projects that:
  * wrap DataSource or Driver into a ConnectionPoolDataSource
  * implement different RDBMS requirements via specialized Dialects.

Refer to my above comments regarding resource sharing and the impact it
would have if you just wrapped the existing PGXAConnection.

>>> Sure, that would
>>> be slow, so you'd definitely still want to configure the TM to not do
>>> that, but at least it would work.
>>
>> We evaluated what it would take to get pgjdbc to work with GlassFish's
>> TM as is. There are cases in our application where it does SUSPEND. What
>> you are proposing will not fix the missing support for SUSPEND / JOIN.
>
> Did you evaluate what it would take to add the option to Glassfish?

No.

The GlassFish code base is not one I could familiarize myself with in a
reasonable amount of time. Given the choice between running custom
builds of an entire container or just a database driver in a production
environment, I'm going to pick the smallest, easiest to understand, and
most clearly documented piece of code to customize. In this case, that
was clearly pgjdbc.

>> Honestly, I don't understand why all these drivers did such a half-baked
>> job of implementing the spec, or why you're so vocally defending a
>> half-implementation when it took two people bouncing ideas, code, and
>> testing a week to get it workable, and a few additional weeks to
>> stabilize it.
>
> The JTA specification is crap. It imposes requirements on the drivers
> that have nothing to do with the real goal of the spec: supporting
> two-phase commit. It's silly that drivers have to be complicated to
> support those things.

Agreed. Which is why if I was going to write this as a generic wrapper,
I'd do it against DataSource instead of XADataSource, removing the
obvious weak-link of the quality of a driver-specific XA implementation.

There was zero reason they couldn't have added callbacks to the JDBC
interfaces to obtain statements to execute the 2pc invocations in a
driver-specific manner and implemented the rest of the X/Open spec as
.... A Wrapper! I think this pull request proves that to some extent.

> If you look at the javadocs for XADataSource, it says for
> getXAConnection():
>
>> Attempts to establish a *physical* database connection that can be
>> used in a distributed transaction.
>
> (emphasis mine). It's pretty clear what the intention of the authors is.
> "physical" is a vague term, but you'd expect that typically to be a
> single TCP connection, wouldn't you?

Yes. That's why this implementation aggressively closes those extra
connections, why it allows you to disable the behavior completely, and
why it allows you to set a 'wait for a free connection before opening a
new physical connection' timeout.

> It's not expected that the driver
> needs a multiplexing layer, mapping a single XAConnection to multiple
> physical connections.

It's also not expected that the driver won't work or will lead to data
loss when a Transaction Manager written to spec tries to use the other
methods in the API you chose not to copy / paste for reference. It's not
expected that someone arbitrarily decide they don't like the spec, so
they simply don't implement the functionality at all.

> Now, maybe it works fine with the multiplexing layer, but even if that's
> the way to meet the letter of the JTA spec, I don't think it's in the
> spirit of the spec that you'd have to do that.

I'm pretty certain it wasn't the spirit of the spec to have insufficient
implementations defended by strawman arguments about a bad spec. While I
agree that the spirit of the spec expects a single connection I think
it's far more dangerous to just completely ignore other parts of the
specification based on 'I don't like it' which can lead to people
attempting to use your software suffering data loss in production
environments.

We can agree there is no optimal solution here. The JTA spec is bad.

You can say what you want about our approach with multiplexing, but at
least it works consistently, reliably, under load, and hasn't lost data
in a production environment.

That's more than I can say for the implementation in upstream/master.

> Regarding the patch itself:
>
> Does it work correctly if you prepare a PreparedStatement in one
> transaction, suspend, and try to use the prepared statement in another
> transaction? I'm not sure what "correctly" is here, I don't think the
> JTA spec says anything explicit about that scenario (it's crap, after
> all ;-)), but I'd expect that to work.

If it hasn't hit the prepare threshold I'd expect it to work.

If it has hit the prepare threshold, I'd expect it to fail.

This is certainly a consideration that would need to be taken into
account. Especially if one were to bother with implementing JDBC3
statement pooling. PostgreSQL (due to the way it prepares statements) is
going to have to violate the 'spirit' of the JDBC 3 API and
specification in this regard too, if anyone ever adds statement pooling.
The C3P0 connection pool does statement pooling per-connection rather
than globally for non-XA datasource pools and works marvelously. There
is precedent in going outside the 'spirit' of an API if having a robust
implementation that covers the rest of the API is the result.

> It's a bit worrying that you can exceed your connection pool size with
> this. In the worst case, a single logical connection can use an
> arbitrary number of physical connections. That makes it more difficult
> to set max_connections, so that you don't run out of slots. To an admin,
> it's pretty surprising to see 50 connections from a host, when you've
> configured the max connection pool size to 40.

We did consider putting constraint properties in place to allow setting
maximum overhead connections, and even potentially pooling those -- it
was just easier and faster for us to measure the average connection
usage while running the application, and size the pool sufficiently
large to keep it from exceeding that limit under normal conditions,
while allowing it to go bonkers (if necessary) to service load in
abnormal conditions.

The choice here is between allowing extra short-lived connections to
handle load (which _may_ go over your pool size) and loosing customer data.

Keep in mind that when it's trying to service a logical connection it
will wait (for a time) for an existing connection to free up before
opening a new physical connection. That wait period is configurable, and
will have the side-effect of throttling the applications using the pool,
allowing for a more stable pool size, if you set the delay high enough.

Regards,
-Bryan Varner


---------------
The views expressed in this communication are the sole responsibility of
the author and do not reflect those of POLARIS Laboratories.

On 15.03.2013 23:29, Bryan Varner wrote:
> IIRC, the existing pgjdbc implementation
> did not allow for a connection to switch global transactions after
> prepare but before commit or rollback.

It does. After prepare, the connection is in "idle" state, and you can
start a new transaction in it.

- Heikki

On 15.03.2013 23:29, Bryan Varner wrote:
> IIRC, the existing pgjdbc implementation
> did not allow for a connection to switch global transactions after
> prepare but before commit or rollback.

It does. After prepare, the connection is in "idle" state, and you can
start a new transaction in it.

That's correct, the issue was that you couldn't commit or rollback a global transaction that was prepared that you
weren'tworking on anymore. 

Dave Cramer <pg@fastcrypt.com> wrote:

> I'm looking for opinions on this patch.
>
> Heikki ? Kevin Grittner ?

To introduce myself to the POLARIS folks and put my comments in
perspective, I'm a committer on the underlying database product,
and spent a few years coding in Java -- including debugging a few
small JDBC issues.  I'm going to approach this more-or-less as I
would a review of source code for the core product, and Dave may
reign me in if the approach here is different.

First off, I would like to thank the POLARIS team for tackling this
and having the patience to work with the community in sharing your
work.  I promise we're not trying to be difficult, but a rigorous
process is important to the long-term viability of the product, and
I hope to keep the pain to a minimum.  My initial points may seem
picky, but experience has shown that these things do matter.

One things is the means of delivery.  It's great to use github to
develop "in the open".  I do that and applaud your use of this
technique.  The core developers, however, have a policy of asking
the submitter to email a patch to the community list.  The creates
a permanent record of who submitted what, when, and is considered
to be proof that you are offering the code to the community under
the terms of the project's license.  INAL, but apparently this is
considered important to avoid disputes about the right to
distribute the work.  So, based on the subsequent discussion, I
request that someone authorized to offer this to the community
submit a context or unified diff of the work to this list, as an
attachment to an email.

In comparing your github repository to the master branch, I see a
lot of whitespace changes that are not functionally necessary, and
I see changes to support relocation of the source code.  I know you
already moved the source code itself back to its historical
location, but the changes to support making it relocatable should
really be submitted as a separate patch, and whitespace changes
should also be submitted in a patch which does absolutely nothing
but modify whitespace and maybe comments.  As a reviewer I can say
that it helps tremendously to be able to focus on just the single
feature without these other "distractions".

Personally, I like the changes to make the source code relocatable.
Perhaps you could pull out just those changes as a fairly small
patch and submit that for discussion, review, and possible
inclusion in the community code?  That will be once step to keeping
the main patch smaller and more focused.

Then the whitespace changes.  In the core code we prohibit trailing
whitespace in source code.  If you run `git diff
master...POLARIS_XA` you will see the trailing whitespace in red.
If you could eliminate that, I would appreciate it.  If you could
then break out any whitespace changes (like bringing an opening
brace up from its own line to the end of the previous line) as a
separate patch, that would likewise minimize distractions in
reviewing the main patch.  Might as well do that first, too, so
whitespace in the main part of the patch can be in matching style.
There may be some discussion about how things *should* be
formatted; in the core project we have a piece of software that
standardizes all of that and saves a lot of arguments on such
issues.  Here we may have to go through a (hopefully brief)
religious war on the topic.

Of course, none of this is intended to substitute for a review of
the substance of the patch, which will take me a bit longer -- it
is intended to make review easier and more focused and allow those
looking back through the revision history to better understand the
changes.

Thanks again for tackling an ambitious feature set!

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn@ymail.com> wrote:

> If you run `git diff master...POLARIS_XA` you will see the
> trailing whitespace in red.

This is probably obvious, but I meant:

git diff --color master...POLARIS_XA

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I'm looking for opinions on this patch.

On Fri, Mar 15, 2013 at 12:45 PM, Bryan Varner <notifications@github.com> wrote:

The support for interleaving and sharing is implemented by (potentially, only when needed) returning 'logical' connections from the XADataSource, and opening new physical connections to the backend server (when necessary) to handle [performing] work on behalf of a global TX or local TX.

This sounds like you're essentially implementing a connection pool inside PgJDBC to handle transaction suspend, interleave, etc. This has generally been recognised as the only way to achieve these features on most real-world database products, so I don't have a problem with that so long as it's set up and torn down properly, avoids resource leaks, etc. I haven't taken a proper look at the code yet so I'm asking to help me understand what I should be looking for.

Since JTA and the X/Open XA spec it is based on rather unfortunately specifies transaction suspend and interleave as required features it was always going to be necessary to do something awful like this at some level. I thought that JBoss AS 7's JTA implementation took care of this behind the scenes in cases where the JDBC driver didn't support tx suspend and resume, though? Is that not the case? Do other XA clients not do the same thing?

(Looks at some other project code)

Yep. I was using this with JTA, and JBoss AS 7 was taking care of all the transaction suspend/interleave emulation its self:

<?xml version="1.0" encoding="UTF-8"?> 
<datasources> 
  <datasource jndi-name="java:/datasources/XXXXXX" enabled="true" use-java-context="true" 
        pool-name="XXXXX-pool"> 
    <connection-url>jdbc:postgresql:XXXXX</connection-url> 
    <driver>postgresql-driver</driver> 
    <security> 
      <user-name>XXXXX</user-name>
      <password>XXXXX</password>
    </security> 
  </datasource> 
</datasources> 

This isn't a good reason not to fix XADataSource, but it might be informative. If nothing else you can probably learn more about the JTA/XA requirements and tricks for interleaving, etc, from AS7's code.

I was not using 2PC and co-ordinating with other transaction managers, so it's entirely possible this only works for the one-datasource-per-XA-transaction case.

The behavior of opening new physical backends (implementing interleaving) can be disabled by setting the XAAcquireTimeout = 0, in which case the behavior of the old XADataSource implementation is retained, with a minimal performance impact.

Great, we'll almost certainly need that since someone's certain to be relying on it - intentionally or not - and unwilling/unable to change their code or the library they're using.
 

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

> Kevin Grittner<kgrittn@ymail.com>  wrote:
>
>> If you run `git diff master...POLARIS_XA` you will see the
>> trailing whitespace in red.
>
> This is probably obvious, but I meant:
>
> git diff --color master...POLARIS_XA


Thank you for the constructive feedback, Kevin.

It will likely take us a few days (perhaps longer) to get these requests
addressed. I don't think any requests like this are unreasonable, and
we'll do whatever we can to make the process of improving the driver for
everyone as smooth as possible.

In the mean time, I'm going to leave the existing pull request in place.
I will retract it when I break these into smaller patches.

Is it possible to setup github so that a pull request emails the list
with the diff attached? :-)


Regards,
  -Bryan

> This isn't a good reason not to fix XADataSource, but it might be
> informative. If nothing else you can probably learn more about the
> JTA/XA requirements and tricks for interleaving, etc, from AS7's code.

We were in contact with some of the folks from the JBoss community
throughout our development. They were very helpful.


 > I was not using 2PC and co-ordinating with other transaction managers,
 > so it's entirely possible this only works for the
 > one-datasource-per-XA-transaction case.

It sounds like you may have hit a use-case where you were
'automagically' getting the advantages of a Last Agent Optimization
approach. For our applications, we have multiple XA participants and the
LAO is not a feasible solution for us at this time.

There are properties in the JBoss TMs for disabling the use of
interleaving. Most TMs have properties like this. The GlassFish TM does
not. We are re-evaluating our container options. That will be a
long-term project and may solve our XA issues by allowing us to fiddle
with the TM behavior, but would not result in the PG jdbc driver
becoming more compliant with published specs. I don't hold the
unrealistic expectation that everyone will use TMs and containers that
work around bugs for specific drivers.

> Great, we'll almost certainly need that since someone's certain to be
> relying on it - intentionally or not - and unwilling/unable to change
> their code or the library they're using.

I would not have submitted a patch if the behavior to open additional
physical connections and disable interleaving support were not included.

It may even be more appropriate for this support to be disabled by
default. I'm somewhat surprised no one has demanded that rather than
debating the merits of weather or not suspend / join and resource
sharing should be supported at all.

Regards,
-Bryan