SEO

Dave Cramer wrote:
> Attached is the patch for review. QueryExecutor is unchanged now.
>
> The only iffy bit is where I check for the in parameter being bound to
> void type. It could be done in checkAllParametersSet
>
> I'd like to commit this shortly.

Ok, comments from just reading the patch.. I like the approach much
better than the previous one, but the details need some cleanup.

Rather than putting knowledge of parameter direction into ParameterList,
how about just allowing access to the type OID, and the caller checks
for Oid.VOID? Then the ParameterList interface changes less and the
change to use a Parameter class is unnecessary as the list doesn't need
to store a direction value.

If you must store a direction value in the list, then another array
might be better than wrapping everything up in an object (generates less
garbage overall). Also, there are a few places that return magic values
for the direction that should be using the symbolic constants you've
defined elsewhere.

The change to checkAllParametersSet to not check OUT parameters seems
unnecessary -- won't OUT parameters be set to the (non-null) NULL_OBJECT
anyway?

> ! // this is here for the sole purpose of passing the cts
> ! if ( columnType == Types.DOUBLE && functionReturnType[i] == Types.REAL )
> ! {
> ! // return it as a float
> ! if ( callResult[i] != null)
> ! callResult[i] = new Float(((Double)callResult[i]).floatValue());
> ! }

We can't do that! If it's failing that's probably because we're not
doing the necessary implicit typecasts required by the spec..

Please explain what's going on here:

> + case Types.FLOAT: // TODO: FLOAT and REAL were FLOAT8 for the cts

> public void setFloat(int parameterIndex, float x) throws SQLException
> {
> checkClosed();
> ! bindLiteral(parameterIndex, Float.toString(x), Oid.FLOAT8);
> }
>

(the bind changed from Oid.FLOAT4 to Oid.FLOAT8..)

You seem to have reverted your earlier changes and put back the types/*
classes -- why?

adjustParamIndex() should be removed entirely if it's now always a no-op.

Why is type translation for JDBC2 types only done in the JDBC3 code (in
registerOutParameter)? -- shouldn't that be in the JDBC2 code?

There are a bunch of gratuitous changes to the test code that probably
shouldn't be committed, e.g. in BatchExecuteTest:

> + try
> + {
> + Class.forName("org.postgresql.Driver");
> + }
> + catch( Exception ex){}

similarly in many other test classes.

.....

It might be an idea to break this up into "support new-style OUT
parameters" and "other fixes necessary to pass the CTS" as currently
it's quite unclear which is which..

-O

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

On 29-Jun-05, at 9:17 AM, Oliver Jowett wrote:

> Dave Cramer wrote:
>
>> Attached is the patch for review. QueryExecutor is unchanged now.
>> The only iffy bit is where I check for the in parameter being
>> bound to void type. It could be done in checkAllParametersSet
>> I'd like to commit this shortly.
>>
>
> Ok, comments from just reading the patch.. I like the approach much
> better than the previous one, but the details need some cleanup.
>
> Rather than putting knowledge of parameter direction into
> ParameterList, how about just allowing access to the type OID, and
> the caller checks for Oid.VOID? Then the ParameterList interface
> changes less and the change to use a Parameter class is unnecessary
> as the list doesn't need to store a direction value.
>
> If you must store a direction value in the list, then another array
> might be better than wrapping everything up in an object (generates
> less garbage overall). Also, there are a few places that return
> magic values for the direction that should be using the symbolic
> constants you've defined elsewhere.
I thought of this, however even arrays have to be garbage collected..
Is there really much difference internally between new Parameter and
new int?

I'm ambivalent though it's not a big change either way.
>
> The change to checkAllParametersSet to not check OUT parameters
> seems unnecessary -- won't OUT parameters be set to the (non-null)
> NULL_OBJECT anyway?
>
>
>> ! // this is here for the sole purpose of
>> passing the cts
>> ! if ( columnType == Types.DOUBLE &&
>> functionReturnType[i] == Types.REAL )
>> ! {
>> ! // return it as a float
>> ! if ( callResult[i] != null)
>> ! callResult[i] = new Float(((Double)
>> callResult[i]).floatValue());
>> ! }

I'll go through my notes, but I can tell you it was a catch 22
problem mostly likely an artifact of Oracle not having a REAL type.
> We can't do that! If it's failing that's probably because we're not
> doing the necessary implicit typecasts required by the spec..
>
> Please explain what's going on here:

from an SQL point of view FLOAT, and double are FLOAT8 types, REAL is
the only one that is FLOAT4
>
>
>> + case Types.FLOAT: // TODO: FLOAT and REAL
>> were FLOAT8 for the cts
>>
>
>
>> public void setFloat(int parameterIndex, float x) throws
>> SQLException
>> {
>> checkClosed();
>> ! bindLiteral(parameterIndex, Float.toString(x), Oid.FLOAT8);
>> }
>>
>>
>
> (the bind changed from Oid.FLOAT4 to Oid.FLOAT8..)
>
> You seem to have reverted your earlier changes and put back the
> types/* classes -- why?
huh ? they should be in there in HEAD, I did remove the creation of
an object, and went to static methods
>
> adjustParamIndex() should be removed entirely if it's now always a
> no-op.
yeah
>
> Why is type translation for JDBC2 types only done in the JDBC3 code
> (in registerOutParameter)? -- shouldn't that be in the JDBC2 code?
>
Good point
> There are a bunch of gratuitous changes to the test code that
> probably shouldn't be committed, e.g. in BatchExecuteTest:
>
agreed, they are for my own testing
>
>> + try
>> + {
>> + Class.forName("org.postgresql.Driver");
>> + }
>> + catch( Exception ex){}
>>
>
> similarly in many other test classes.
>
> ....
>
> It might be an idea to break this up into "support new-style OUT
> parameters" and "other fixes necessary to pass the CTS" as
> currently it's quite unclear which is which..
Well, they are all related OUT parameters are required to support the
CTS
>
> -O
>
>

Oliver

regarding the real/double muck below

They have a "real table" which ostensibly holds REAL values, however

one test does the following

takes the max value from the int_table which is a large integer, then
stores it into the 'REAL' table and then reads it back as an integer

this was failing due to precision issues until I change the
underlying type of the REAL table to FLOAT8 (which is what Oracle,
and Mysql do )

then they have another test

which registers an out parameter as a REAL and reads from the
'REAL_TABLE' which is now uses the underlying type double

if someone else can find a way to get this to pass, I'm all ears.

Dave
On 29-Jun-05, at 10:28 AM, Dave Cramer wrote:

>
> On 29-Jun-05, at 9:17 AM, Oliver Jowett wrote:
>
>> Dave Cramer wrote:
>>
>>> Attached is the patch for review. QueryExecutor is unchanged now.
>>> The only iffy bit is where I check for the in parameter being
>>> bound to void type. It could be done in checkAllParametersSet
>>> I'd like to commit this shortly.
>>>
>>
>> Ok, comments from just reading the patch.. I like the approach
>> much better than the previous one, but the details need some cleanup.
>>
>> Rather than putting knowledge of parameter direction into
>> ParameterList, how about just allowing access to the type OID, and
>> the caller checks for Oid.VOID? Then the ParameterList interface
>> changes less and the change to use a Parameter class is
>> unnecessary as the list doesn't need to store a direction value.
>>
>> If you must store a direction value in the list, then another
>> array might be better than wrapping everything up in an object
>> (generates less garbage overall). Also, there are a few places
>> that return magic values for the direction that should be using
>> the symbolic constants you've defined elsewhere.
> I thought of this, however even arrays have to be garbage
> collected.. Is there really much difference internally between new
> Parameter and new int?
>
> I'm ambivalent though it's not a big change either way.
>>
>> The change to checkAllParametersSet to not check OUT parameters
>> seems unnecessary -- won't OUT parameters be set to the (non-null)
>> NULL_OBJECT anyway?
>>
>>
>>> ! // this is here for the sole purpose of
>>> passing the cts
>>> ! if ( columnType == Types.DOUBLE &&
>>> functionReturnType[i] == Types.REAL )
>>> ! {
>>> ! // return it as a float
>>> ! if ( callResult[i] != null)
>>> ! callResult[i] = new Float(((Double)
>>> callResult[i]).floatValue());
>>> ! }
>
> I'll go through my notes, but I can tell you it was a catch 22
> problem mostly likely an artifact of Oracle not having a REAL type.
>> We can't do that! If it's failing that's probably because we're
>> not doing the necessary implicit typecasts required by the spec..
>>
>> Please explain what's going on here:
>
> from an SQL point of view FLOAT, and double are FLOAT8 types, REAL
> is the only one that is FLOAT4
>>
>>
>>> + case Types.FLOAT: // TODO: FLOAT and
>>> REAL were FLOAT8 for the cts
>>>
>>
>>
>>> public void setFloat(int parameterIndex, float x) throws
>>> SQLException
>>> {
>>> checkClosed();
>>> ! bindLiteral(parameterIndex, Float.toString(x),
>>> Oid.FLOAT8);
>>> }
>>>
>>>
>>
>> (the bind changed from Oid.FLOAT4 to Oid.FLOAT8..)
>>
>> You seem to have reverted your earlier changes and put back the
>> types/* classes -- why?
> huh ? they should be in there in HEAD, I did remove the creation of
> an object, and went to static methods
>>
>> adjustParamIndex() should be removed entirely if it's now always a
>> no-op.
> yeah
>>
>> Why is type translation for JDBC2 types only done in the JDBC3
>> code (in registerOutParameter)? -- shouldn't that be in the JDBC2
>> code?
>>
> Good point
>> There are a bunch of gratuitous changes to the test code that
>> probably shouldn't be committed, e.g. in BatchExecuteTest:
>>
> agreed, they are for my own testing
>>
>>> + try
>>> + {
>>> + Class.forName("org.postgresql.Driver");
>>> + }
>>> + catch( Exception ex){}
>>>
>>
>> similarly in many other test classes.
>>
>> ....
>>
>> It might be an idea to break this up into "support new-style OUT
>> parameters" and "other fixes necessary to pass the CTS" as
>> currently it's quite unclear which is which..
> Well, they are all related OUT parameters are required to support
> the CTS
>>
>> -O
>>
>>
>

Dave Cramer wrote:

>> You seem to have reverted your earlier changes and put back the
>> types/* classes -- why?
>
> huh ? they should be in there in HEAD, I did remove the creation of an
> object, and went to static methods

HEAD went through this progression:

(1) no types/*
(2) you added types/PGByte, etc
(3) you removed types/* and added static methods

Your patch does things like this:

> --- 1500,1520 ----
> preparedParameters.clear();
> }
>
> ! private PGType createInternalType( Object x, int targetType ) throws PSQLException
> {
> ! if ( x instanceof Byte ) return PGByte.castToServerType((Byte)x, targetType );
> ! if ( x instanceof Short ) return PGShort.castToServerType((Short)x, targetType );
> ! if ( x instanceof Integer ) return PGInteger.castToServerType((Integer)x, targetType );
> ! if ( x instanceof Long ) return PGLong.castToServerType((Long)x, targetType );
> ! if ( x instanceof Double ) return PGDouble.castToServerType((Double)x, targetType );
> ! if ( x instanceof Float ) return PGFloat.castToServerType((Float)x, targetType );
> ! if ( x instanceof BigDecimal) return PGBigDecimal.castToServerType((BigDecimal)x, targetType );
> ! // since all of the above are instances of Number make sure this is after them
> ! if ( x instanceof Number ) return PGNumber.castToServerType((Number)x, targetType );
> ! if ( x instanceof Boolean) return PGBoolean.castToServerType((Boolean)x, targetType );
> ! return new PGUnknown(x);
>
> + }
> // Helper method for setting parameters to PGobject subclasses.
> private void setPGobject(int parameterIndex, PGobject x) throws SQLException {
> String typename = x.getType();

which seems to be reverting back to step (2)

I'm guessing you forgot to apply the changes in (3) to your working copy
of the driver, then diffed against current HEAD..

-O

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

Dave Cramer wrote:

> I've changed direction to an array

Ok. What did you think about removing it entirely from the parameterlist
level? It just seems like extra complexity that doesn't need to be there..

> regarding the jdbc3 type conversion in registerOutParameter, an
> existing conversion (BIT to BOOLEAN) was there, do all of them need to
> be in jdbc2 ?

BOOLEAN is only defined in JDBC3, so that conversion needs to be in the
JDBC3 code or the driver won't build under JDBC2.

All the others should be in JDBC2 code.

>>>> You seem to have reverted your earlier changes and put back the
>>>> types/* classes -- why?
>>>>
>>>
>>> huh ? they should be in there in HEAD, I did remove the creation of an
>>> object, and went to static methods
>>>
> I moved them from to core/types, but they have always been there

Oh, ok, your patch didn't show those renames. I also thought you'd
removed the types/ stuff earlier, guess I misread your commit :/

I still think they are redundant and should be entirely removed. We can
do that afterwards though.

Why the repackaging?

-O

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

Did you read my reasons for the ugly if double, then float stuff ?

On 30-Jun-05, at 8:04 PM, Oliver Jowett wrote:

> Dave Cramer wrote:
>
>
>> I've changed direction to an array
>>
>
> Ok. What did you think about removing it entirely from the
> parameterlist
> level? It just seems like extra complexity that doesn't need to be
> there..

I think it can be removed, however I think sooner than later we will
be dealing
with more complex parameters when stored procedures with real IN/OUT
parms
>
>
>> regarding the jdbc3 type conversion in registerOutParameter, an
>> existing conversion (BIT to BOOLEAN) was there, do all of them
>> need to
>> be in jdbc2 ?
>>
>
> BOOLEAN is only defined in JDBC3, so that conversion needs to be in
> the
> JDBC3 code or the driver won't build under JDBC2.
>
> All the others should be in JDBC2 code.
>
Done
>
>>>>> You seem to have reverted your earlier changes and put back the
>>>>> types/* classes -- why?
>>>>>
>>>>>
>>>>
>>>> huh ? they should be in there in HEAD, I did remove the
>>>> creation of an
>>>> object, and went to static methods
>>>>
>>>>
>> I moved them from to core/types, but they have always been there
>>
>
> Oh, ok, your patch didn't show those renames. I also thought you'd
> removed the types/ stuff earlier, guess I misread your commit :/
>
> I still think they are redundant and should be entirely removed. We
> can
> do that afterwards though.

If absolutely necessary, however I don't think setObject with a
different type is
in the critical path
>
> Why the repackaging?

can't remember now.
>
> -O
>
>


---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

Dave Cramer wrote:
> Did you read my reasons for the ugly if double, then float stuff ?

Haven't had time to get my head around that yet.. can you translate it
to an explanation of something in the JDBC spec that we don't do
correctly? It's a bit hard to understand what's going wrong without the
CTS code to hand..

> I think it can be removed, however I think sooner than later we will be
> dealing
> with more complex parameters when stored procedures with real IN/OUT parms

Well, let's add the complexity only when we need it..

>> I still think they are redundant and should be entirely removed. We can
>> do that afterwards though.
>
> If absolutely necessary, however I don't think setObject with a
> different type is
> in the critical path

I'll hack on it if I have time after this is all applied; don't worry
about it for now since it's already in HEAD.

>> Why the repackaging?
>
> can't remember now.

Can you avoid repackaging then? Less CVS churn..

-O

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

On 30-Jun-05, at 8:56 PM, Oliver Jowett wrote:

> Dave Cramer wrote:
>
>> Did you read my reasons for the ugly if double, then float stuff ?
>>
>
> Haven't had time to get my head around that yet.. can you translate it
> to an explanation of something in the JDBC spec that we don't do
> correctly? It's a bit hard to understand what's going wrong without
> the
> CTS code to hand..

If you consider that Oracle doesn't have a FLOAT4 type this is a non-
issue for them
they will always convert a FLOAT8 to a java float if that is what is
registered, and they
will never have an issue storing a large integer into a FLOAT type.

Not that "Oracle doesn't do it" is a good argument, but I would
imagine they were instrumental
in the creation of the test suite.

FWIW, the spec, and the CTS aren't always in agreement. I have
pointed out the
issues to them, and finally gave up, the CTS is the defacto standard,
regardless of it's short comings

I'll try to dig around for the actual tests that failed later

>
>
>> I think it can be removed, however I think sooner than later we
>> will be
>> dealing
>> with more complex parameters when stored procedures with real IN/
>> OUT parms
>>
>
> Well, let's add the complexity only when we need it..
I still want to push this in the way it is and we can hack on it
until 8.1 goes out
>
>
>>> I still think they are redundant and should be entirely removed.
>>> We can
>>> do that afterwards though.
>>>
>>
>> If absolutely necessary, however I don't think setObject with a
>> different type is
>> in the critical path
>>
>
> I'll hack on it if I have time after this is all applied; don't worry
> about it for now since it's already in HEAD.
>
>
>>> Why the repackaging?
>>>
>>
>> can't remember now.
>>
>
> Can you avoid repackaging then? Less CVS churn..

Absolutely
>
> -O
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that
> your
> message can get through to the mailing list cleanly
>
>


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match