tightening up on use of oid 0

I am currently cleaning up a few places where OID 0 could get used as a
parameter type (causing the backend to try to infer a type). For
example, this occurs when setObject() is passed an object that it can't
classify (not any of Integer, Boolean, PGobject, etc), or when calling
setObject() with a non-PGobject and Types.OTHER.

I think this is a bad idea: we do not have any mechanism in place to
check that the type actually matches the expected type. It also seems
quite fragile to rely on this behaviour when everything else in JDBC is
fairly strongly typed.

Another place I am less sure about is setNull(x, Types.OTHER); this
passes the NULL with type OID 0. Without this, there is no other way to
set a nonstandard-typed parameter to null. I think this is still
problematic. Consider the case where you have two functions:

foo(line)
foo(box)

Executing "SELECT foo(?)" via PreparedStatement will work fine if you
pass a non-null PGline or PGbox argument to setObject, but if you try to
setNull() then you will get ambiguity between the two functions at
execution time.

I can't see a way to fix this without a postgresql extension of some
sort. Options I can think of are:

1) PGStatement.setNull(int parameterIndex,String databaseType): sets
parameter to null with the given type.

2) PGStatement.setTypeHint(int parameterIndex,String databaseType): sets
parameter's type, only, to the given type; this is then used by later
calls to setNull().

3) PGConnection.addDataType(String type, Class klass, int sqlType):
register a new java.sql.Types value along with the type handler.

(3) seems like the least intrusive approach, but it requires that
extension types somehow decide on a unique Types value. I'm not sure if
this is practical.

-O

On Fri, 8 Oct 2004, Oliver Jowett wrote:
>
> Executing "SELECT foo(?)" via PreparedStatement will work fine if you
> pass a non-null PGline or PGbox argument to setObject, but if you try to
> setNull() then you will get ambiguity between the two functions at
> execution time.
>
> I can't see a way to fix this without a postgresql extension of some
> sort. Options I can think of are:

Can't the existing PGobject interface handle this. You can create a
PGobject with the correct datatype and a null value and that should work.

Kris Jurka

Kris Jurka wrote:
>
> On Fri, 8 Oct 2004, Oliver Jowett wrote:
>
>>Executing "SELECT foo(?)" via PreparedStatement will work fine if you
>>pass a non-null PGline or PGbox argument to setObject, but if you try to
>>setNull() then you will get ambiguity between the two functions at
>>execution time.
>>
>>I can't see a way to fix this without a postgresql extension of some
>>sort. Options I can think of are:
>
>
> Can't the existing PGobject interface handle this. You can create a
> PGobject with the correct datatype and a null value and that should work.

OK, so getValue() and toString() return null?

This would still mean that setNull() would not work for Types.OTHER, though.

-O

On Sat, 9 Oct 2004, Oliver Jowett wrote:

> > Can't the existing PGobject interface handle this. You can create a
> > PGobject with the correct datatype and a null value and that should work.
>
> OK, so getValue() and toString() return null?

Right.

> This would still mean that setNull() would not work for Types.OTHER, though.
>

I don't think this is a very big deal. The only place this looks
important is for an overloaded function call. The previous driver didn't
have any typing at all for NULLs, and I can only recall one complaint
about. The other API additions you suggested didn't really appeal to me
and we have an "out" with PGobject, so I think we're OK here.

Kris Jurka

Oliver Jowett wrote:
> I am currently cleaning up a few places where OID 0 could get used as a
> parameter type (causing the backend to try to infer a type).

Here is a patch to do this, including the PGobject changes to handle SQL
NULLs.

The PGobject / geometric type changes are a bit ugly in places, mostly
because those types are mutable (unnecessarily, IMO).

-O

On Sat, 9 Oct 2004, Oliver Jowett wrote:

> Oliver Jowett wrote:
> > I am currently cleaning up a few places where OID 0 could get used as a
> > parameter type (causing the backend to try to infer a type).
>
> Here is a patch to do this, including the PGobject changes to handle SQL
> NULLs.
>

What I was suggesting before was a means to allow users to specify a pg
type for the Types.OTHER case, but not to require it. I don't see the
danger in allowing OID 0 in this case. I know you are after complete
strong typing, but I don't see the benefit while I do see the drawback.
Comments?

Kris Jurka

Kris Jurka wrote:
>
> On Sat, 9 Oct 2004, Oliver Jowett wrote:
>
>
>>Oliver Jowett wrote:
>>
>>>I am currently cleaning up a few places where OID 0 could get used as a
>>>parameter type (causing the backend to try to infer a type).
>>
>>Here is a patch to do this, including the PGobject changes to handle SQL
>>NULLs.
>>
>
>
> What I was suggesting before was a means to allow users to specify a pg
> type for the Types.OTHER case, but not to require it. I don't see the
> danger in allowing OID 0 in this case.

Ah, I thought you were OK with disallowing setNull(Types.OTHER)
entirely, I must have misunderstood what you said earlier.

I described one problem with allowing it in an earlier email:

>> Consider the case where you have two functions:
>>
>> foo(line)
>> foo(box)
>>
>> Executing "SELECT foo(?)" via PreparedStatement will work fine if you pass a non-null PGline or PGbox argument to setObject, but if you try to setNull() then you will get ambiguity between the two functions at execution time.

That's quite unpredictable behaviour.

> I know you are after complete
> strong typing, but I don't see the benefit while I do see the drawback.

What is the drawback? The only case that will change is the case that is
currently ambiguous. And there is a fairly simple mechanism for
disambiguating it via PGobject.

-O

On Wed, 13 Oct 2004, Oliver Jowett wrote:

> >> Consider the case where you have two functions:
> >>
> >> foo(line)
> >> foo(box)
> >>
> >> Executing "SELECT foo(?)" via PreparedStatement will work fine if you
> >> pass a non-null PGline or PGbox argument to setObject, but if you try
> >> to setNull() then you will get ambiguity between the two functions at
> >> execution time.
>

I was expecting to see this "ERROR: function f("unknown") is not unique"
in all ambiguous situations. Your approach has the benefit of being
fail-fast as adding a new function to the database suddently can't
produce an ambiguity for Java code. I don't think it's a common
situation to have overloaded functions that get called with non-typed
nulls, so I wanted to allow it to work as usual for non-ambiguous
cases.

I was testing this out a little and this doesn't produce the error I
expected:

CREATE FUNCTION g(int) RETURNS int AS 'SELECT 1;' LANGUAGE sql;
CREATE FUNCTION g(float) RETURNS int AS 'SELECT 2;' LANGUAGE sql;
SELECT g(NULL);

Instead it returns 2 indicating the float version was called. I don't
know if this is a bug and/or oddity of the type system, but if it's the
expected behavior then I definitely agree with you.

> What is the drawback? The only case that will change is the case that is
> currently ambiguous. And there is a fairly simple mechanism for
> disambiguating it via PGobject.

The case that will change is that all the non-ambiguous cases can
no longer be called with untyped nulls:
- non ambiguous functions
- INSERT/UPDATE statements that use nulls

Kris Jurka

Kris Jurka wrote:

> I was testing this out a little and this doesn't produce the error I
> expected:
>
> CREATE FUNCTION g(int) RETURNS int AS 'SELECT 1;' LANGUAGE sql;
> CREATE FUNCTION g(float) RETURNS int AS 'SELECT 2;' LANGUAGE sql;
> SELECT g(NULL);

I think implicit casting is interfering here somehow.

With types that don't have implicit casts, you get ambiguity:

>> test=> CREATE FUNCTION h(line) RETURNS int AS 'SELECT 1;' LANGUAGE sql;
>> CREATE FUNCTION
>> test=> CREATE FUNCTION h(point) RETURNS int AS 'SELECT 2;' LANGUAGE sql;
>> CREATE FUNCTION
>> test=> SELECT h(NULL);
>> ERROR: function h("unknown") is not unique
>> HINT: Could not choose a best candidate function. You may need to add explicit type casts.

-O

Kris Jurka wrote:

> Your approach has the benefit of being
> fail-fast as adding a new function to the database suddently can't
> produce an ambiguity for Java code.

Right. The driver cannot guarantee that setNull(i,Types.OTHER) will
always be able to infer the right type. It seems safer to me to
completely disallow it than have it sometimes succeed and sometimes fail
depending on the exact state of the database. It may break older apps,
but new apps would be more robust.

General question for the list: how much code is out there that performs
one of these (equivalent) calls?

PreparedStatement.setObject(i, null);
PreparedStatement.setObject(i, null, Types.OTHER);
PreparedStatement.setNull(i, Types.OTHER);

> I don't think it's a common
> situation to have overloaded functions that get called with non-typed
> nulls, so I wanted to allow it to work as usual for non-ambiguous
> cases.

After some experimentation, it's not just overloaded functions that are
affected.

- "? IS NULL" breaks if the parameter is an untyped NULL (this was the
original issue -- in an off-list email --that made me look at this area)

- Functions taking 'anyarray' or 'anyelement' don't like untyped NULLs,
even if they are STRICT (ERROR: could not determine anyarray/anyelement
type because input has type "unknown")

I'd like to catch those errors earlier than query execution if possible.
If nothing else, it'll encourage application developers to provide
correct type info to the driver in all cases..

-O

Hi,

For my part, I've never used any of those calls.

FWIW, I always use reference objects such as "Integer" as opposed to "int"
so I've never done it that way. All calls to setObject or set<Anything>
would always use a variable which is typed (and may be null) my assumption
is that I avoid all such ambiguities that way.

regards
iain

> Kris Jurka wrote:

> General question for the list: how much code is out there that performs
> one of these (equivalent) calls?
>
> PreparedStatement.setObject(i, null);
> PreparedStatement.setObject(i, null, Types.OTHER);
> PreparedStatement.setNull(i, Types.OTHER);
>

Oliver Jowett <oliver(at)opencloud(dot)com> writes:
> - "? IS NULL" breaks if the parameter is an untyped NULL (this was the
> original issue -- in an off-list email --that made me look at this area)

Hmm. The system doesn't complain if you do "select 'z' IS NULL". It
knows that it doesn't have a hard idea about the datatype of 'z', but it
also knows that it doesn't matter much. The reason that you are seeing
a failure is that exec_parse_message() explicitly fails if any parameter
datatypes remain UNKNOWN after parsing. I made it do that because I
expected that client code would be unhappy to get UNKNOWN back as a
"resolved" parameter datatype. Would you rather get that result or
a failure?

> - Functions taking 'anyarray' or 'anyelement' don't like untyped NULLs,
> even if they are STRICT (ERROR: could not determine anyarray/anyelement
> type because input has type "unknown")

This you're just stuck with. There has to be some way to determine the
actual datatype imputed to the function result, and if you supply an
untyped parameter then there isn't. It hasn't got anything to do with
whether the parameter is NULL or not.

regards, tom lane

On Wed, 13 Oct 2004, Oliver Jowett wrote:

> I'd like to catch those errors earlier than query execution if possible.
> If nothing else, it'll encourage application developers to provide
> correct type info to the driver in all cases..

I don't see the real benefit of catching this one statement earlier. It's
still a runtime failure.

Kris Jurka

Iain wrote:
> Hi,
>
> For my part, I've never used any of those calls.
>
> FWIW, I always use reference objects such as "Integer" as opposed to
> "int" so I've never done it that way. All calls to setObject or
> set<Anything> would always use a variable which is typed (and may be
> null) my assumption is that I avoid all such ambiguities that way.

Just to clarify.. these calls are typed:

setInt(i, 42);
setObject(i, new Integer(42));
setObject(i, new Integer(42), Types.INTEGER);
setObject(i, null, Types.INTEGER);
setNull(i, Types.INTEGER);
setObject(i, new PGline(...), Types.OTHER);

These calls are not (sufficiently) typed:

setObject(i, null);
setObject(i, (Integer)null); // (*)
setObject(i, null, Types.OTHER);
setNull(i, Types.OTHER);

Types.OTHER on its own is not specific enough to identify a particular
backend type, and Java nulls have no inherent type ('instanceof' will
always return false).

From your description it sounds like you may use the case marked (*) ?

-O

Kris Jurka wrote:
>
> On Wed, 13 Oct 2004, Oliver Jowett wrote:
>
>
>>I'd like to catch those errors earlier than query execution if possible.
>>If nothing else, it'll encourage application developers to provide
>>correct type info to the driver in all cases..
>
>
> I don't see the real benefit of catching this one statement earlier. It's
> still a runtime failure.

My point was that it is useful to generate an error on *all* uses of
untyped nulls, so that the developer sees the error in all cases
regardless of the database state or statement context.

Consider, for example, framework code that calls user-specified or
user-provided functions. If it uses untyped nulls, and the driver uses
Oid.UNKNOWN, you will only see the problem when the framework tries to
pass a NULL to an overloaded function. If the driver rejects the call
with an error in all cases where a NULL is used, then the framework
developer sees the error as soon as they exercise the NULL path,
regardless of whether their test environment has overloaded functions or
not.

====

The spec has this to say about the setObject case:

>> If setObject is called without a type parameter, the Java Object is
>> implicitly mapped using the default mapping for that object type.

There is no default mapping (in appendix B table B-4) for Java nulls.

Of course the spec then goes on to say:

>> If a Java null is passed to any of the setter methods that take a Java
>> object, the parameter will be set to JDBC NULL.

I don't know which takes precedence. The way that setNull() is organized
implies that JDBC requires NULLs to be typed, but there's nothing
explicit about it.

-O

Hi Oliver,

Just out of interest, is the case you marked,

> setObject(i, (Integer)null); // (*)

equivalent to

Integer someInteger = null;
setObject(i, someInteger);

?

From what I remember of my code I'd be surprised if I was doing either as
this case would use setInt instead of setObject. I don't think I use
setObject anywhere.

I would ask the question then, is there any situation where there is no
alternative to the insufficiantly typed calls you listed? From my limited
view of the situation, my feeling is that there isn't, so I would say that
such calls should produce errors rather than some kind of default behavour.

Cheers
Iain

Iain wrote:
> Hi Oliver,
>
> Just out of interest, is the case you marked,
>
>> setObject(i, (Integer)null); // (*)
>
> equivalent to
>
> Integer someInteger = null;
> setObject(i, someInteger);
>
> ?

Yes.

> I would ask the question then, is there any situation where there is no
> alternative to the insufficiantly typed calls you listed?

I think there is always an alternative.

For standard types you can use setNull or setObject with a type code:

setNull(i, Types.INTEGER);
setObject(i, null, Types.INTEGER);

For extension types (classed as Types.OTHER) you can use the singleton
NULL objects I introduced in my patch:

setObject(i, PGline.NULL);
setObject(i, PGline.NULL, Types.OTHER);

-O

Kris Jurka wrote:

> I was looking at the assorted changes to the PGobject extensions and I'm
> unclear on exactly how NULL is handled. Consider PGmoney has tests for
> NULL in equals, clone, and getValue, but PGbox does not. Is this simply
> an oversight or is there something more profound going on here.

I ended up with two approaches for this.

For those types where there was already a field I could hijack to
represent NULL -- e.g. PGbox's points array -- I used that to represent
null values. The singleton NULL is just a normal object that happens to
have a null value. You can have several different objects that all
represent null if you like, and you can mutate an object representing a
null value just like any other object of the type. This is consistent
with the way other instances of the type operate, but it's slightly
dangerous as it's possible to modify the NULL singleton so it no longer
has a null value (pity we don't have 'const'..)

For the other types that didn't have a suitable field, I'd have needed
to add a field to every instance of the type to indicate whether the
object was a null or not. Instead, I used the identity of the NULL
singleton to decide when an object is null. In that case, there is only
ever one object that represents a null, and the actual value it holds is
irrelevant -- getValue() etc. check the identity of 'this' before
looking at the actual value.

It's hardly ideal but it kept the changes to a minimum. If you don't
mind a more invasive set of changes, I can probably come up with
something better.

-O

On Thu, 14 Oct 2004, Oliver Jowett wrote:

> Kris Jurka wrote:
>
> > I was looking at the assorted changes to the PGobject extensions and I'm
> > unclear on exactly how NULL is handled. Consider PGmoney has tests for
> > NULL in equals, clone, and getValue, but PGbox does not. Is this simply
> > an oversight or is there something more profound going on here.
>
> I ended up with two approaches for this.

I don't like the lack of consistency here, "new PGbox()" is NULL, but "new
PGmoney()" is zero instead. I also don't like the ability to mutate away
NULLness. This means another application can break mine by modifying the
shared PGbox.NULL object.

> It's hardly ideal but it kept the changes to a minimum. If you don't
> mind a more invasive set of changes, I can probably come up with
> something better.

Yes, let's think about this a little more. I unfortunately don't have any
brilliant ideas, perhaps just adding a boolean everywhere is simplest.

Kris Jurka

Here's a merged version of the patch, if it helps:

http://www.ejurka.com/pgsql/patches/

Hi,

Interesting, thanks for your feedback.

regards
Iain
----- Original Message -----
From: "Oliver Jowett" <oliver(at)opencloud(dot)com>
To: "Iain" <iain(at)mst(dot)co(dot)jp>
Cc: <pgsql-jdbc(at)postgresql(dot)org>
Sent: Thursday, October 14, 2004 11:40 AM
Subject: Re: [JDBC] tightening up on use of oid 0

> Iain wrote:
>> Hi Oliver,
>>
>> Just out of interest, is the case you marked,
>>
>>> setObject(i, (Integer)null); // (*)
>>
>> equivalent to
>>
>> Integer someInteger = null;
>> setObject(i, someInteger);
>>
>> ?
>
> Yes.
>
>> I would ask the question then, is there any situation where there is no
>> alternative to the insufficiantly typed calls you listed?
>
> I think there is always an alternative.
>
> For standard types you can use setNull or setObject with a type code:
>
> setNull(i, Types.INTEGER);
> setObject(i, null, Types.INTEGER);
>
> For extension types (classed as Types.OTHER) you can use the singleton
> NULL objects I introduced in my patch:
>
> setObject(i, PGline.NULL);
> setObject(i, PGline.NULL, Types.OTHER);
>
> -O
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
> joining column's datatypes do not match

Kris Jurka wrote:
>
> On Thu, 14 Oct 2004, Oliver Jowett wrote:
> [... PGobject and NULL ...]
>>It's hardly ideal but it kept the changes to a minimum. If you don't
>>mind a more invasive set of changes, I can probably come up with
>>something better.
>
> Yes, let's think about this a little more. I unfortunately don't have any
> brilliant ideas, perhaps just adding a boolean everywhere is simplest.

I've applied the non-PGobject bits of this patch.

For PGobject it turned into a bit of a general overhaul. Currently I have:

PGobject layer:

- PGobject becomes an interface
- Implementations of PGobject should provide a ctor taking a single
String; this is called by the driver to construct non-null objects.
- PGobject.setType() and PGobject.setValue() go away entirely
- A new helper class, PGunknown, provides an immutable type+value
implementation of PGobject (i.e. 'new PGunknown("mytype","myvalue")').
This gives the functionality currently available via PGobject.setType()
/ setValue, and is used by the driver when it receives an unhandled type
in a resultset.

PGobject subclasses:

Mutability:
- Generally, classes become immutable where it's easy to do (PGmoney,
PGinterval, PGpoint, PGline, PGlseg, PGcircle, PGbox)
- PGpoint.translate() returns a new PGpoint rather than modifying the
existing point (I'm not sure why this method even exists really..)
- Constant-sized PGpoint[] arrays in PGline, PGlseg, PGbox become
separate fields; this makes immutability much easier.
- PGpolygon and PGpath remain mutable as it's hard to make them
immutable without incurring lots of array copies.

NULL-handling:
- No-arg ctors construct NULL objects.
- PGmoney and PGpoint get boolean isNull fields, the other types reuse
an existing reference field.
- equals(), hashCode(), getValue() take account of NULL-ness.
- Add per-class static NULL singleton fields for convenience; for
mutable types, I'm not sure whether to just omit the singletons
(slightly less convenient/readable), or to use PGunknown to get an
immutable NULL (works, but 'instanceof' on NULL becomes misleading).

Much of the mutability changes here are just personal preference,
they're not necessary to support NULLs but I thought I'd clean things up
while I was in the area.

When we come to do binary-format types, I'd expect we would have a
subinterface (PGbinaryobject?) that adds whatever methods are needed for
binary parameter formatting. Objects that implement PGbinaryobject
become candidates for binary transfer.

Any thoughts on this general approach?

-O

Oliver Jowett wrote:

> For PGobject it turned into a bit of a general overhaul. Currently I have: [...]

I've put my current changes up at:
http://visible.randomly.org/pgjdbc/pgjdbc-pgobject-changes.txt

-O

Hi, Oliver,

On Thu, 28 Oct 2004 15:18:13 +1300
Oliver Jowett <oliver(at)opencloud(dot)com> wrote:

> - Implementations of PGobject should provide a ctor taking a single
> String; this is called by the driver to construct non-null objects.

Is it possible to use a static factory function instead?

This would make it possible to produce different subclasses depending on
the String, which would be useful e. G. for PostGIS, als all the
geometry classes share the same postgres type "geometry".

> When we come to do binary-format types, I'd expect we would have a
> subinterface (PGbinaryobject?) that adds whatever methods are needed for
> binary parameter formatting. Objects that implement PGbinaryobject
> become candidates for binary transfer.

What do you think about the factory / handler object approach that AFAIR
was discussed here some days ago?

So the driver gets registered one PGfactory for every postgres type, and
this factory then has methods to transform objects to text and binary
representation and vice-versa.

This would allow us to read and write instances of third-party defined
classes that don't implement any postgres specific interface.

We still could have a default factory implementation that gets used
whenever any legacy application uses PGObject subclasses.

Markus

--
markus schaber | dipl. informatiker
logi-track ag | rennweg 14-16 | ch 8001 zürich
phone +41-43-888 62 52 | fax +41-43-888 62 53
mailto:schabios(at)logi-track(dot)com | www.logi-track.com

On Thu, 28 Oct 2004, Oliver Jowett wrote:

> For PGobject it turned into a bit of a general overhaul. Currently I have:

These changes are way too drastic for something as minor as preventing a
user from accidentally mutating a NULL PGobject. These API changes suck
for both developers and users. There's no way to make a PGobject
implementation compile against both 7.4 and 8.0 drivers. Altering the
PGline API means user code can't compile against both 7.4 and 8.0 drivers.

If we were providing exciting new features, then maybe, but for now we've
got to find a way to make this work without huge API changes or we should
abandon the whole idea and go back to your original patch. What
immediately comes to mind is making the PGobject interface an abstract
class with all abstract methods so that a developer can implement a type
that can work with both driver versions.

Kris Jurka

Kris Jurka wrote:
>
> On Thu, 28 Oct 2004, Oliver Jowett wrote:
>
>
>>For PGobject it turned into a bit of a general overhaul. Currently I have:
>
>
> These changes are way too drastic for something as minor as preventing a
> user from accidentally mutating a NULL PGobject.

That wasn't really my motivation. It was a general cleanup of PGobject
and subclasses. The current implementations leave something to be desired.

> These API changes suck
> for both developers and users. There's no way to make a PGobject
> implementation compile against both 7.4 and 8.0 drivers. Altering the
> PGline API means user code can't compile against both 7.4 and 8.0 drivers.

The feedback I got earlier was that changing the PGobject API wasn't a
big deal and that external users of the API (which seems to boil down to
PostGIS & co only) would just track the changes. Is this not true?

> If we were providing exciting new features, then maybe, but for now we've
> got to find a way to make this work without huge API changes or we should
> abandon the whole idea and go back to your original patch. What
> immediately comes to mind is making the PGobject interface an abstract
> class with all abstract methods so that a developer can implement a type
> that can work with both driver versions.

That's possible. I'd almost prefer doing a new interface and having
PGobject be a completely-abstract implementation of it. Having the
driver-required bit as an interface makes it much easier to integrate
into whatever representation of the data is convenient to the application.

This might be moot if we look at a mapping mechanism that is external to
the data objects themselves, as suggested by Markus.

Either way, I can't really justify spending much more time on this: we
don't use extension types in our code at all, and there is still an
(admittedly ugly) way to set NULL values for extension types in the
existing driver.

-O