Обсуждение: patch for passing the cts

Well the good news is that the driver and the backend will pass the
cts with this patch.

Rather than commit this as is, I'd like to discuss the patch as it is.

I'm really not happy with the handling of IN/OUT parameters in the
query executor, however deferring parsing doesn't seem to be an
option either.

We need to know how many parameters there are  before
CallableStatment.setXXX, and CallableStatement.registerOutParameter
are called.

The real solution is to extend the FE/BE protocol to specify IN/OUT
parameters and send them down the wire.


I do intend to move all of the 8.1 specific callable statement tests
out into their own file and optionally run them if the backend is 8.1
or greater.

Dave Cramer
davec@postgresintl.com
www.postgresintl.com
ICQ #14675561
jabber davecramer@jabber.org
ph (519 939 0336 )

Dave Cramer wrote:
> Well the good news is that the driver and the backend will pass the  cts
> with this patch.
>
> Rather than commit this as is, I'd like to discuss the patch as it is.
>
> I'm really not happy with the handling of IN/OUT parameters in the
> query executor, however deferring parsing doesn't seem to be an  option
> either.

I don't think your approach works, though; consider this (untested) example:

 CallableStatement cs = conn.prepareCall("{call f(?*4,?)}");
 cs.setInt(1,42);
 cs.registerOutParameter(2,Types.INTEGER);

The query gets parsed into these fragments:

 0:"select * from f("
 1:"*4,"
 2:") as result"

As I read your code, that's going to discard fragment 1 blindly in the
query executor, resulting in this query:

  "select * from f($1) as result"

which is wrong.

The logic that decides to remove OUT placeholders needs to be aware of
the rest of the {call} structure (mainly, comma separation of
parameters) to do it correctly, and I really don't think that the
QueryExecutor implementations are the right place for that..

-O

Oliver,

So if we defer parsing, this will require changing the strategy to
create the parameters as they are set, or registered.

I would suggest using an ArrayList of Parameters, and grow this as
required, then parse when it is executed.

Do you have any suggestions ?

Dave
On 14-Jun-05, at 7:21 PM, Oliver Jowett wrote:

> Dave Cramer wrote:
>
>> Well the good news is that the driver and the backend will pass
>> the  cts
>> with this patch.
>>
>> Rather than commit this as is, I'd like to discuss the patch as it
>> is.
>>
>> I'm really not happy with the handling of IN/OUT parameters in the
>> query executor, however deferring parsing doesn't seem to be an
>> option
>> either.
>>
>
> I don't think your approach works, though; consider this (untested)
> example:
>
>  CallableStatement cs = conn.prepareCall("{call f(?*4,?)}");
>  cs.setInt(1,42);
>  cs.registerOutParameter(2,Types.INTEGER);
>
> The query gets parsed into these fragments:
>
>  0:"select * from f("
>  1:"*4,"
>  2:") as result"
>
> As I read your code, that's going to discard fragment 1 blindly in the
> query executor, resulting in this query:
>
>   "select * from f($1) as result"
>
> which is wrong.
>
> The logic that decides to remove OUT placeholders needs to be aware of
> the rest of the {call} structure (mainly, comma separation of
> parameters) to do it correctly, and I really don't think that the
> QueryExecutor implementations are the right place for that..
>
> -O
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>
>

Dave Cramer wrote:
> Oliver,
>
> So if we defer parsing, this will require changing the strategy to
> create the parameters as they are set, or registered.
>
> I would suggest using an ArrayList of Parameters, and grow this as
> required, then parse when it is executed.
>
> Do you have any suggestions ?

I'd go with storing a local array of parameters on the statement when
you're handling a {call}, and create the final query and assign
parameters just before execution.

You'll probably need to expose some parameter factory methods or
something similar on QueryExecutor since different implementations want
to store parameters differently.

-O

Dave Cramer wrote:

> Do you have any suggestions ?

Also, the parser for {call} is going to have to get a lot smarter. A
first cut at what it needs to do..

Extract this information from the query:

- Name of the called function
- Number of ? parameter placeholders
- Text of each function argument
- Mapping from parameter index to function argument (many-to-one)
- Eligibility of each parameter for being an OUT parameter (this can be
determined from the text the parameter maps to -- if it's a bare "?"
then it's eligible)

On execute, you reassemble a query from the IN function arguments text only.

e.g. for the query "{call f(?+?,?)}" you'll get:

 function = "f"
 3 parameters
 2 function arguments:
   argument 1 = "?+?"
   argument 2 = "?"
 parameter 1 maps to function arg 1 and can't be OUT
 parameter 2 maps to function arg 1 and can't be OUT
 parameter 3 maps to function arg 2 and can be OUT

If you set all 3 parameters as IN parameters, it assembles a SELECT
including all function arguments:

  "select * from f(?+?,?) as result"

and gives it to the query executor which executes:

  "select * from f($1+$2,$3) as result"

If you set parameter 3 to OUT, it excludes the corresponding function
argument (argument 2) when assembling the SELECT:

  "select * from f(?+?) as result"

resulting in the actual query:

  "select * from f($1+$2) as result"


Hmm.. perhaps you could get a similar effect by doing the normal query
fragmentation then stripping a comma (only) from one of the adjacent
fragments to an OUT parameter, then merging the two adjacent fragments
as you remove the parameter; but that seem pretty hairy..

-O

Oliver,

Good observation. However aren't we back to the parsing before
register, set ?

What about making the output of the current query parser a little
more flexible, and putting some of the intelligence in the executor.

More specifically, the parser currently breaks things up into
fragments which are very easily re-assembled by the executor. Instead
use the strategy below


Dave
On 15-Jun-05, at 12:46 AM, Oliver Jowett wrote:

> Dave Cramer wrote:
>
>
>> Do you have any suggestions ?
>>
>
> Also, the parser for {call} is going to have to get a lot smarter. A
> first cut at what it needs to do..
>
> Extract this information from the query:
>
> - Name of the called function
> - Number of ? parameter placeholders
> - Text of each function argument
> - Mapping from parameter index to function argument (many-to-one)
> - Eligibility of each parameter for being an OUT parameter (this
> can be
> determined from the text the parameter maps to -- if it's a bare "?"
> then it's eligible)
>
> On execute, you reassemble a query from the IN function arguments
> text only.
>
> e.g. for the query "{call f(?+?,?)}" you'll get:
>
>  function = "f"
>  3 parameters
>  2 function arguments:
>    argument 1 = "?+?"
>    argument 2 = "?"
>  parameter 1 maps to function arg 1 and can't be OUT
>  parameter 2 maps to function arg 1 and can't be OUT
>  parameter 3 maps to function arg 2 and can be OUT
>
> If you set all 3 parameters as IN parameters, it assembles a SELECT
> including all function arguments:
>
>   "select * from f(?+?,?) as result"
>
> and gives it to the query executor which executes:
>
>   "select * from f($1+$2,$3) as result"
>
> If you set parameter 3 to OUT, it excludes the corresponding function
> argument (argument 2) when assembling the SELECT:
>
>   "select * from f(?+?) as result"
>
> resulting in the actual query:
>
>   "select * from f($1+$2) as result"
>
>
> Hmm.. perhaps you could get a similar effect by doing the normal query
> fragmentation then stripping a comma (only) from one of the adjacent
> fragments to an OUT parameter, then merging the two adjacent fragments
> as you remove the parameter; but that seem pretty hairy..
Yeah, as shown, it gets ugly

>
> -O
>
>

Dave Cramer wrote:

> What about making the output of the current query parser a little  more
> flexible, and putting some of the intelligence in the executor.
>
> More specifically, the parser currently breaks things up into  fragments
> which are very easily re-assembled by the executor. Instead  use the
> strategy below

The parsing is protocol-specific (different handling of
multiple-statement queries is the main thing), and we only need the
extra parsing complexity when we are handling a {call} escape.

We need an escape parser anyway since the backend doesn't know anything
about the JDBC escapes, so we'll have two parse steps going on anyway
unless you want to integrate that, too, into the query executor.

I can't see an obvious clean way of integrating this with the call
escape handling without duplicating lots of code and entangling the
high-level statement code with the low-level protocol details. I'd
prefer to see the query rewriting for {call} all happen in one place in
a protocol-independent way, and ideally only for the case when {call} is
actually used.

I really don't have any spare time for working on this in any detail :(
 I'll swallow my objections if nothing better comes along, but I fear
that code turning into an unmaintainable mess.

-O