Re: jdbc xa patches - Mailing list pgsql-jdbc

Michael Allman wrote:

> I would like to see this make its way into CVS.  Would a committer weigh
> in?

(caveat: this is just from reading the code, not actually running it; I
don't have a functional 8.1 build to hand at the moment)

I don't see any major problems with integrating this. Random comments:

We now use 4-space indents, not tabs.

Do you have changes to build.xml to handle conditional compilation of
the XA code only when the XA classes are available?

Is the test code ready to hook into the standard testsuite? Or are you
anticipating a separate test run in the 'tests' target that invokes
XATestSuite?

Is there any reason why the XA datasource needs to be a separate class?
i.e. can we roll that functionality into all our datasources and put it
at the top of the hierarchy somewhere? My only concern there is
availability of the XA classes over different JDBC versions, we may need
more conditional compilation, or use token substitution to comment out
the XA code when the classes aren't available.

Shouldn't the XAResource check the server version on construction or on
start()/recover() to make sure that it's actually going to be able to
use PREPARE TRANSACTION later? Or is erroring out with a syntax error at
the point of prepare() sufficient? (I'd like to see a better error
message there at least)

I'm not entirely happy about defaulting to autocommit=off on connections
retrieved from the datasource. I understand that it needs to be off
before you can actually do an XA start(), but I don't like defaulting to
off as the Connection docs do say that connections start out with
autocommit on. Also, can we check in start() rather than on getXAResource()?

Having ResourceAssociationState as a full-blown class seems like
overkill since you only have one instance anyway.

Is XID_TO_TRANSACTION_STATE_MAP actually necessary? It seems like it's
only there for detecting XA protocol errors (which would otherwise show
up as errors from the backend, e.g. trying to commit a nonexistant
transaction), and for tracking the state of the transaction that is
currently suspended or currently associated (of which there can only be
one in the current implementation)

I'm not sure it's necessary for recover() to avoid returning
non-JDBC-originated transactions; from memory the TM is already required
to handle recovered Xids that were not generated by it. (need to check
the XA spec here)

-O