Обсуждение: jdbc xa patches

I have uploaded another version of my PostgreSQL JDBC XA support to

http://www.allman.ms/pgjdbcxa/pgjdbcxa-20050726.jar

I have improved the code documentation in this release.

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

Thanks.

Michael

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

Oliver Jowett wrote:

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

More generally: can we get XAExceptions thrown with useful messages? It
seems like there's no direct constructor that gives you both message and
error code, but the error code is a public field so you can set it after
construction before throwing.

Also, in the case where the failure is caused by another exception, we
should generally chain that exception rather than writing it to the log
writer. See how PSQLException does this for details..

There's enough meddling there that it might be worth subclassing
XAException to automate it all.. or have a helper method somewhere.

-O

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

On Wed, 27 Jul 2005, Oliver Jowett wrote:

> Oliver Jowett wrote:
>
>> 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)
>
> More generally: can we get XAExceptions thrown with useful messages? It
> seems like there's no direct constructor that gives you both message and
> error code, but the error code is a public field so you can set it after
> construction before throwing.

Short answer: yes.

Long answer:

Since the client of a PGXAResource instance is a transaction manager I see
no benefit.  The transaction manager will make a decision on what to do
based on the code carried by XAException and carry on.  Maybe it will log
a message.

> Also, in the case where the failure is caused by another exception, we
> should generally chain that exception rather than writing it to the log
> writer. See how PSQLException does this for details..
>
> There's enough meddling there that it might be worth subclassing
> XAException to automate it all.. or have a helper method somewhere.

I'm just not sure it's worth the effort.  I did put in log messages that I
think will help debug problems and whatnot.  But exceptions are going to
be handled by the TM and will not be rethrown to any user/app code.

Maybe we should do more logging?  I'm not quite sure that would be much
benefit either.  I've found the current level of logging to be adequate.

On a related note, I think we should do some kind of app server testing
before this code is released.  I think this calls for Cactus.  What do you
think?  I know how to use JBoss.  Maybe someone else could write build
files to deploy to other app servers.

Michael

Michael Allman wrote:
> On Wed, 27 Jul 2005, Oliver Jowett wrote:
>
>> 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?

perl? :-)

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

Yes, per JDBC spec. Still, it is generally tricky to distinguish a
particular error class from more general errors (SQLSTATE only gets you
so far).

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

Just a bare int?

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

Isn't that covered by tracking the state of the one transaction that's
currently in progress on the connection? i.e. if you get asked to
rollback() while it's active, you do a rollback(); otherwise you do a
ROLLBACK PREPARED.

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

Sorry, that's what I meant by "handle" as in "not complain about" aka
"ignore" :)

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

I guess that's a good reason.

-O

Michael Allman wrote:
> On Wed, 27 Jul 2005, Oliver Jowett wrote:
>
>> More generally: can we get XAExceptions thrown with useful messages? It
>> seems like there's no direct constructor that gives you both message and
>> error code, but the error code is a public field so you can set it after
>> construction before throwing.

> Since the client of a PGXAResource instance is a transaction manager I
> see no benefit.  The transaction manager will make a decision on what to
> do based on the code carried by XAException and carry on.  Maybe it will
> log a message.

I absolutely disagree with there being no benefit. We should provide as
much information as possible in the exception. All hhe TM *has* to log
is the exception we give it, really, so that better have all the
information we can give about any underlying problems.

-O

On Wed, 27 Jul 2005, Oliver Jowett wrote:

> Michael Allman wrote:
>> On Wed, 27 Jul 2005, Oliver Jowett wrote:
>>
>>> 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?
>
> perl? :-)

ugh :-(

>>> 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?
>
> Yes, per JDBC spec. Still, it is generally tricky to distinguish a particular
> error class from more general errors (SQLSTATE only gets you so far).

First, I'm assuming that any general release of the JDBC driver with XA
support will make it clear up front that it requires Postgres 8.1.

With that in mind, if some mortal is foolish enough to attempt to prepare
a transaction on an unsupported backend, executeUpdate() will barf up an
SQLException, which will cause PGXAResource to throw an XAException yada
yada yada which will cause the TM to rollback the transaction.  So at
least they won't be in Dangling Transaction Hell.

Anyway, I'm counting on Postgres users knowing what they're doing, esp.
with something as non-trivial as XA support.

>>> 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?
>
> Just a bare int?

In this kind of situation I prefer an enum or enum-like class to an int.
I don't dispute that it would work with just an int.  As a matter of
personal preference I like to use a more specific type.

>>> 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.
>
> Isn't that covered by tracking the state of the one transaction that's
> currently in progress on the connection? i.e. if you get asked to rollback()
> while it's active, you do a rollback(); otherwise you do a ROLLBACK PREPARED.

I think you're right.  I also think this is a matter of style/personal
preference.  I like to give classes some of the responsibility for their
correct usage.  It makes it easier to diagnose problems when a class
instance is used incorrectly.

Michael

On Wed, 27 Jul 2005, Oliver Jowett wrote:

> Michael Allman wrote:
>> On Wed, 27 Jul 2005, Oliver Jowett wrote:
>>
>>> More generally: can we get XAExceptions thrown with useful messages? It
>>> seems like there's no direct constructor that gives you both message and
>>> error code, but the error code is a public field so you can set it after
>>> construction before throwing.
>
>> Since the client of a PGXAResource instance is a transaction manager I see
>> no benefit.  The transaction manager will make a decision on what to do
>> based on the code carried by XAException and carry on.  Maybe it will log a
>> message.
>
> I absolutely disagree with there being no benefit. We should provide as much
> information as possible in the exception. All hhe TM *has* to log is the
> exception we give it, really, so that better have all the information we can
> give about any underlying problems.

In cases where an XAException is thrown due to another exception, say
SQLException, I believe I was consistent in writing the cause exception's
stack trace to the DriverManager's log writer.  I think that covers
logging cause exceptions for the most part.

I'm not really disputing that putting nice string messages in all our
thrown exceptions is bad, just that in this particular use case it's
probably not worth the effort.  (The effort being dealing with the fact
that XAException does not take an error code and string message at once.)

Michael

Michael Allman wrote:

> First, I'm assuming that any general release of the JDBC driver with XA
> support will make it clear up front that it requires Postgres 8.1.

Sure, but not everyone reads documentation before posting "problems" to
the list.

> With that in mind, if some mortal is foolish enough to attempt to
> prepare a transaction on an unsupported backend, executeUpdate() will
> barf up an SQLException, which will cause PGXAResource to throw an
> XAException yada yada yada which will cause the TM to rollback the
> transaction.  So at least they won't be in Dangling Transaction Hell.

Well, that's a given; I'd be complaining much more loudly if it silently
ate errors!

> Anyway, I'm counting on Postgres users knowing what they're doing, esp.
> with something as non-trivial as XA support.

You can't really assume that users of XA are familiar with the details
of our particular XA implemenation, like "a syntax error at this point
means your backend is too old".

Back to my original point: It Needs A Better Error Message. How you do
this I don't really care, checking the server version just seemed like
the most reliable way to distinguish "it doesn't support 2PC" from "some
random connection error".

>> Isn't that covered by tracking the state of the one transaction that's
>> currently in progress on the connection? i.e. if you get asked to
>> rollback() while it's active, you do a rollback(); otherwise you do a
>> ROLLBACK PREPARED.
>
> I think you're right.  I also think this is a matter of style/personal
> preference.  I like to give classes some of the responsibility for their
> correct usage.  It makes it easier to diagnose problems when a class
> instance is used incorrectly.

My point was you can still do 90% of this without the extra complexity,
and the other 10% will get handled by the server (if, for example, a
broken TM asks to commit a transaction that was never prepared)

Complexity is bad for maintenance. I'm probably going to end up at least
helping with maintenance of this code, which is why I'm complaining up
front.

-O

Michael Allman wrote:
> On Wed, 27 Jul 2005, Oliver Jowett wrote:
>
>> I absolutely disagree with there being no benefit. We should provide
>> as much information as possible in the exception. All hhe TM *has* to
>> log is the exception we give it, really, so that better have all the
>> information we can give about any underlying problems.
>
> In cases where an XAException is thrown due to another exception, say
> SQLException, I believe I was consistent in writing the cause
> exception's stack trace to the DriverManager's log writer.  I think that
> covers logging cause exceptions for the most part.

Logging stack traces to the log writer isn't acceptable really, you have
no idea whether that's going somewhere that is useful. The
implementation of the standard java.sql classes means that the log
writer isn't useful for much beyond debugging, you pretty much have to
disable it for any production use. Also, the rest of the driver chains
exceptions rather than use the log writer in similar situations -- why
is the XA code special?

> I'm not really disputing that putting nice string messages in all our
> thrown exceptions is bad, just that in this particular use case it's
> probably not worth the effort.  (The effort being dealing with the fact
> that XAException does not take an error code and string message at once.)

Put it this way: I consider generating useful exceptions a prerequisite
for new code going into CVS.

-O

On Wednesday 27 July 2005 00:33, Oliver Jowett wrote:
> > > 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?
>
> Just a bare int?

Java 1.4's typesafe enum pattern doesn't usually strike me as an
overkill, as long as you're aware of its problems:

  http://www.google.com/search?q=Java+typesafe+enum

On Tue, 26 Jul 2005, Michael Allman wrote:

> On Wed, 27 Jul 2005, Oliver Jowett wrote:
>
> > 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?
>

The source code may be indented using "astyle -j -p --convert-tabs".

Kris Jurka

On Wed, 27 Jul 2005, Michael Allman wrote:

> Anyway, I'm counting on Postgres users knowing what they're doing, esp. with
> something as non-trivial as XA support.

We all know it's non-trivial, but it looks very innocent to the casual
user who just registers two data sources to an application server. He
doesn't see all the transaction managers and two-phase commits involved.
To him it indeed looks trivial.

Keeping that in mind, I think it's very important to give helpful error
messages that actually give you a clue what the problem is. That means for
example, that if the server version is too old to use two-phase commit,
the error message should say "Your server version is too old for two-phase
commit", not "Error executing statement PREPARE TRANSACTION 12343242".

As pointed out elsewhere in this thread, you can set both the error code
and the error message, just not directly in the XAException constructor.
How about a little helper method to keep the rest of the code clean:

private static constructXAException(int errcode, String message) {
   XAException ex = new XAException(message);
   ex.errorCode = errcode;
   return ex;
}

- Heikki

On Wed, 27 Jul 2005, Heikki Linnakangas wrote:

> On Wed, 27 Jul 2005, Michael Allman wrote:
>
>> Anyway, I'm counting on Postgres users knowing what they're doing, esp.
>> with something as non-trivial as XA support.
>
> We all know it's non-trivial, but it looks very innocent to the casual user
> who just registers two data sources to an application server. He
> doesn't see all the transaction managers and two-phase commits involved.
> To him it indeed looks trivial.
>
> Keeping that in mind, I think it's very important to give helpful error
> messages that actually give you a clue what the problem is. That means for
> example, that if the server version is too old to use two-phase commit, the
> error message should say "Your server version is too old for two-phase
> commit", not "Error executing statement PREPARE TRANSACTION 12343242".
>
> As pointed out elsewhere in this thread, you can set both the error code and
> the error message, just not directly in the XAException constructor. How
> about a little helper method to keep the rest of the code clean:
>
> private static constructXAException(int errcode, String message) {
>  XAException ex = new XAException(message);
>  ex.errorCode = errcode;
>  return ex;
> }

Thanks for your feedback.

I'll upload another patchset with improved exceptions.

Michael

Vadim Nasardinov wrote:
> On Wednesday 27 July 2005 00:33, Oliver Jowett wrote:
>
>>>>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?
>>
>>Just a bare int?
>
>
> Java 1.4's typesafe enum pattern doesn't usually strike me as an
> overkill, as long as you're aware of its problems:
>
>   http://www.google.com/search?q=Java+typesafe+enum

Shrug, the main reason for an enum-like pattern is to get type safety,
but in this case there's only one field that ever uses the enum (and
it's never passed around as a parameter), so there's not much scope for
getting it wrong.

-O

On Wednesday 27 July 2005 18:37, Oliver Jowett wrote:
> >   http://www.google.com/search?q=Java+typesafe+enum
>
> Shrug, the main reason for an enum-like pattern is to get type
> safety, but in this case there's only one field that ever uses the
> enum (and it's never passed around as a parameter), so there's not
> much scope for getting it wrong.

Sure, you can get it right without the added benefit of type safety,
given that the offending piece of code is completely isolated within a
single file.  (Both TransactionState and ResourceAssociationState are
private inner classes of PGXAResource.)  It's a matter of taste and I
have no vested interest in the outcome of this particular
mini-discussion.

That said, from my cursory reading of
org/postgresql/xa/PGXAResource.java, it seems to me that both
TransactionState and ResourceAssociationState instances are used as
values in a Map.  If we replace these two classes with "bare ints"
without changing the rest of the code drastically, we will have to box
those ints into Integers or suchlike before putting them into their
respective maps.  It's not then a huge leap to turn those Integers
into proper typesafe enums, as is currently done.  Seems very
reasonable to me, when viewed on its own merits.  If, however, this
goes against some established pgjdbc prejudice against typesafe enums,
then sure, nuke it.  Better be globally consistent, than locally
correct.

Vadim Nasardinov wrote:

> That said, from my cursory reading of
> org/postgresql/xa/PGXAResource.java, it seems to me that both
> TransactionState and ResourceAssociationState instances are used as
> values in a Map.

Well, I was suggesting killing that map too.

-O