Re: jdbc xa patches - Mailing list pgsql-jdbc

On Wed, 27 Jul 2005, Oliver Jowett wrote:

> I don't see any major problems with integrating this. Random comments:
>
> We now use 4-space indents, not tabs.

I'm away from my normal dev machine and am flailing away with unfamiliar
tools.  Do you have a utility that will indent as desired?

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

Yes.  I would send a diff but I don't have diff right now (see above).  I
am attaching the entire build.xml.

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

Hmmm . . . I'm not sure how you run tests.  I created an XATestSuite class
in anticipation of using it to run all the XA-related JUnit test case
classes.

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

In creating PGXADataSource I followed the example of
org.postgresql.ds.PGConnectionPoolDataSource.

Also, an XADataSource is special in certain ways other than simple having
a method to vend XAConnection instances, e.g. all XADataSource vended
connections are set to "manual commit" (whatever you call the opposite of
auto commit).

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

If we call statement.executeUpdate("SOME QUERY POSTGRESQL DOESN'T
UNDERSTAND") will it throw an instance of SQLException?

> 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()?

The underlying physical connection starts in auto-commit mode, as
required.  Before the client gets a handle to it, it is set to
non-auto-commit mode because the connection comes from an XADataSource.

I think the following from the JDBC 3.0 spec, section 10.1.1 supports my
position:

<quote>
The default is for auto-commit mode to be enabled when the
Connection object is created. If the value of auto-commit is changed in
the middle of a transaction, the current transaction is committed. It is
an error to enable auto-commit for a connection participating in a
distributed transaction, as described in Chapter 12 "Distributed
Transactions".
</quote>

Other than defaulting to non-auto-commit, I haven't given much thought to
checking its state in PGXAResource.  If someone sets a connection
participating in TPC to auto-commit, they're going to have problems one
way or another.

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

Maybe.  Java 1.4 doesn't have enums.  What do you suggest as an
alternative?

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

In the implementation of rollback() it is used to determine whether to
call physicalConnection.rollback() or issue a "ROLLBACK PREPARED" query.

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

Quite the opposite.  The TM is required to ignore xids that were not
generated by it.

Think about it --- how would TM1 know how to recover a transaction branch
created by TM2?  Only TM2 knows what resources where participating in that
branch.

Besides, if someone prepares a transaction with id "whatever", we aren't
going to be able to decode that to an xid.

Thanks for your attention.

Michael