Thread: patch: fix race conditions in Statement.cancel()

patch: fix race conditions in Statement.cancel()

From
Oliver Jowett
Date:
This patch (vs. CVS HEAD) fixes the race condition I reported earlier where
Statement.cancel() could cancel the wrong query, and some related cases
which would bite us if there were multiple Statements being executed
simultaneously and one was cancelled.

I've also moved the cancel() implementation from AbstractJdbc2Statement to
AbstractJdbc1Statement .. I couldn't see an obvious reason for the JDBC1
version to throw not-implemented while we had an implementation in JDBC2.

I needed to add a reasonable amount of infrastructure to do this correctly
while preserving the ability to have different Statements from the same
Connection executing simultaneously from separate threads. I've tried to
keep the overhead of this down; in the common case I think we've gone from
one uncontended monitor entry to two plus a couple of assignments. When
there's contention for the shared Connection, there's a now a wait-notify
loop and some Vector manipulation involved.

The approach I took was to move the mutual exclusion on the backend
connection from a synchronized(pgStream) {} block in QueryExecutor into an
explicit lock structure managed by AbstractJdbc1Connection itself. Queries
are explicitly queued when there is >1 query involved (this is where the
wait-notify loop is). There is a fastpath that gets used when there is no
outstanding query or cancellation request.

Cancellation requests now provide a statement context to the connection,
which handles working out whether the cancel is a no-op, a local-only cancel
(for queries that are queued waiting for the connection), or a real backend
cancel.

As suggested by Tom Lane, in the case of a real backend cancel being needed
we now wait for an EOF from the postmaster on the cancellation connection
before allowing further queries to start.

I've also added a couple of test cases that try to exercise this area ..
although it's hard to properly test for thread race conditions with a
black-box test.

Note that the changes to QueryExecutor are not as bad as it looks, most of
it is indentation changes caused by removing two synchronized blocks.

-O

Attachment