The attached patch (1a) seems to fix the bug and allow XML operators to be executed concurrently.
The patch makes the following changes:
1) The SqlXmlUtil instance isn't stored in the compiled plan. Instead, byte code is generated so that a new instance is created the first time the operator is used in an activation (and reused subsequently in the same activation only).
2) Since the SqlXmlUtil instance is no longer persisted, it no longer implements Formatable, which allowed for removal of a number of methods in the class, as well as entries for the class in RegisteredFormatIds and StoredFormatIds.
3) Created a new abstract super-class of UnaryOperatorNode, BinaryOperatorNode and TernaryOperatorNode, in which code that generates byte-code for the different XML operators could be shared. Currently, their only common super-class is ValueNode, which sounds too general for code that's only used by operator nodes.
4) Enables XMLConcurrencyTest in the regression test suite.
I've verified that we still don't create/compile the SqlXmlUtil instance for each row with the patch. We do create an instance and compile the XML query on the first row that uses the operator per activation. This also means that if you re-execute a previously executed PreparedStatement, the cached value will be used.
In addition to the per-activation compilation of the XML query, we still compile the XML query when creating the SQL query execution plan. This isn't strictly necessary anymore, since the compiled XML query will now be thrown away instead of being stored in the plan. I left the code in there so that syntax errors in the XML query would still generate a compile-time error instead of being delayed to execution-time. I'm open to changing this, but wanted to prevent any behaviour changes in the first iteration.
Other potential improvements:
- More common code can probably be pulled from the *naryOperatorNode classes up to their new parent class. But that's outside the scope of this issue.
- As far as I can see, there's no reason to create a new SqlXmlExecutor instance for every row, as we currently do. The instances are (effectively) immutable, and contain the same values for all rows, so it may make sense to cache the SqlXmlExecutor instances instead of the SqlXmlUtil instances (which can be retrieved from the executors anyway).
- The SqlXmlExecutor class contains a comment indicating that it belongs in the types package, but it was placed in the execute package because it accessed the Activation class, and the types layer shouldn't depend on the SQL layer. SqlXmlExecutor no longer accesses the Activation class, so it may be possible to move it to the types package now.
I've run all the regression tests on an earlier version of the patch without seeing any failures. Rerunning the tests now just to be safe, since I've made some small changes to the patch later.